Php extension should check properties visibility

[anight]

I think php extension should respect php visibility rules for message properties.
Please note this patch is made on top of #7993

test.proto:

syntax = "proto3";
package Test;
message Test {
        int32 foo = 1;
}

test.php:

<?php
        dl('protobuf.so');
        require_once './vendor/autoload.php';
        $t = new Test\Test();
        print_r($t->foo);
?>

Output:

$ php test.php
0

Expected output:

$ php test.php
Fatal error: Uncaught Error: Cannot access protected property Test\Test::$foo

[haberman]

Thanks for this change. Is there any way to 100% prohibit any direct read/write of properties from outside of the class itself? If so I'd prefer to do that. We want the properties of the object to be totally encapsulated.

[anight]

@haberman To my understanding the patch does exactly this. The visibility level and a class from where access is performed are checked inside zend_get_property_info():

https://github.com/php/php-src/blob/60ece88c28494945c9067448e8f9c4eeb18f8aff/Zend/zend_object_handlers.c#L376

Are you talking about some specific case I have overlooked?

[haberman]

Yes, your change seems to have the desired behavior. I was wondering if we could deny access without having to look up the property info. Since we want to protect all properties, it seems unfortunate to do a lookup for it.

Perhaps we could use zend_get_executed_scope() to determine whether we are being called from generated code without having to look up the property?

[anight]

Yes, your change seems to have the desired behavior. I was wondering if we could deny access without having to look up the property info. Since we want to protect all properties, it seems unfortunate to do a lookup for it.

It seems to me that in php a parent class should always check if visibility was re-declared in the child class. I realise this may be a way too pedantic approach for protobuf case since php code generator should generate "protected" visibility for all properties. I just explained how I came to idea of my patch.

Yeah, I can re-implement zend_get_property_info() to adapt it to protobuf case, if you think this is a right approach.

[haberman]

Yeah, I can re-implement zend_get_property_info() to adapt it to protobuf case, if you think this is a right approach.

I am hoping this could be as simple as:

  if (zend_get_executed_scope() != intern->std.ce) {
    return EG(
    zend_throw_exception_ex(NULL, 0,
        "Protobuf fields may only be accessed through generated getters/setters.");
    return
  }

I guess we could return EG(&uninitialized_zval) if that also causes an error to be thrown. But will that throw an error in then unset() case?

[deannagarcia]

I'm going to close this request given no response to the last review comment, but feel free to reopen if you'd like!