Implement mechanism for finding prop_info for property slot

[nikic]

Currently there is no efficient way of finding the property_info
which corresponds to a given property slot. This patch implements
such a mechanism, by storing an array of property_infos in offset
order on the class. This structure is lazily initialized when it
is needed, though we could also compute it during inheritance.

This patch only uses it to optimize visibility checks during
foreach (and get_object_vars etc). We avoid having to look up the
property by name and can directly check the accessibility.

[nikic]

@bwoebi This would be my solution for #3313 (comment).

@dstogov What do you think about this? Part of the motivation here is to fix a tricky edge case for typed properties, but I think that this will also be useful for other things. (E.g. we could implement object iteration without forcing creation of properties HT, which can be a big memory usage problem.)

[dstogov]

I think, this idea may make sense for typed properties, but I don't see benefits for foreach, in current state. This new table is going to be created and accessed even for objects with public properties only, that should be more expensive than additional check for public/private properties.

If this is really necessary, probably, better to combine this patch with typed properties.
I'll try to think about this tomorrow once again.

[dstogov]

There is no reason to inline this function. Better, merge it with zend_check_property_info_access().

[dstogov]

Cached classes with ZEND_ACC_LINKED may build this table once and keep it in SHM, instead of rebuilding on each request.

[dstogov]

opcache might need special handling for this field. At least, add an assertion that it's NULL.

[dstogov]

We, probably, may get property name from property_info->name, instead of unmangling.

[bwoebi]

property_info->name is a mangled name though. Unmangling is unavoidable here.

[bwoebi]

I like the approach (in particular as I cannot imagine a much better approach to this issue), though I'm questioning whether it's worth delaying the initialization at all. At least, like Dmitry hinted at, classes with ZEND_ACC_LINKED will want to persist that information.

@dstogov I had that same assumption than you, but as it turns out, you can redeclare a non-static property with the same name in a child class. And we cannot just optimize that redeclaration away as we'd then loose docblock info for reflection. Thus just checking for protected won't work.

[nikic]

Unfortunately there is an ugly test failure relating to ArrayObject that I'm not sure what to do about. The problem is basically that ArrayObject get_properties (depending on flags) may directly return the underlying HT, which may be an object HT, which may contain INDIRECTs. However, these INDIRECTs will point into the properties_table of the inner object, not the ArrayObject.

This is not a problem for the typed properties use case (where we'd be directly working on the properties_table, not going through properties HT), but it is a problem for implementing visibility checks.

[nikic]

This behavior also causes issues without this patch in some other cases. For example:

class Test {
    public $test;
    public $test2;
}

class AO extends ArrayObject {
    private $test;

    public function getObjectVars() {
        return get_object_vars($this);
    }
}

$ao = new AO(new Test);
var_dump(get_object_vars($ao));
var_dump($ao->getObjectVars());

This gives:

array(1) {
  ["test2"]=>
  NULL
}
php: /home/nikic/php-src/Zend/zend_object_handlers.c:570: zend_check_property_access: Assertion `property_info->flags & (1 << 0)' failed.
Aborted (core dumped)

First, the array only contains the test2 property, because visibility ends up being checked against the AO class, which has a private test property (even though it's completely unrelated to the inner object).

In the second case we get an assertion failure because https://github.com/php/php-src/blob/master/Zend/zend_object_handlers.c#L570 does not hold.

I think we need to change get_properties for ArrayObject to always return its own object properties, and additionally overload (array) to return the inner contents -- I think controlling array casts is the actual motivation for the current get_properties behavior.

[dstogov]

@nikic could you, please, release your ArrayObject related proposal as a bug fix PR and mention compatibility breaks.

[nikic]

@dstogov I've submitted a PR at #3574.

[nikic]

I just noticed a new issue (again, also existing prior to this patch):

<?php
  
class Test {
    private $prop = "Test";

    function run() {
        foreach ($this as $k => $v) {
            echo "$k => $v\n";
        }
        var_dump(get_object_vars($this));
    }
}
class Test2 extends Test {
    public $prop = "Test2";
}

(new Test2)->run();

This currently prints:

prop => Test2
prop => Test
array(2) {
  ["prop"]=>
  string(5) "Test2"
  ["prop"]=>
  string(4) "Test"
}

So both the private and the public property are shown. The get_object_vars() case returns a corrupted array with duplicate keys.

We can't simply assume that because a property is public that is visible from the current scope, because there might be a more specific private property.

[nikic]

I have fixed the above issue with 4d5d779. It was actually throwing an assertion failure on master.

However, @bwoebi pointed out another, larger problem:

<?php
  
class Test {
    private $prop = "Test";

    function run() {
        foreach ($this as $k => $v) {
            echo "$k => $v\n";
        }
        var_dump(get_object_vars($this));
    }
}
class Test2 extends Test {
}

$test2 = new Test2;
$test2->prop = "Test2";
$test2->run();

This also currently prints:

prop => Test
prop => Test2
array(2) {
  ["prop"]=>
  string(4) "Test"
  ["prop"]=>
  string(5) "Test2"
}

In this case we would have to hide the dynamic property, as the private one is in scope. This goes against our usual assumption that dynamic properties are always visible.

This means we must perform visibility checks for dynamic properties as well :( This is maybe less terrible than it sounds, because we can short-circuit this for the case where the class defines no properties, so at least stdClass and similar will not need checks.

[php-pulls]

Comment on behalf of petk at php.net:

Labelling

[nikic]

I've included this in the typed properties patch at #3313 (without the foreach/iteration changes -- only used for typed props for now).