Fix GH-21687: array_walk creates references to readonly properties

[iliaal]

Fixes#21687

array_walk() wrapped object properties in IS_REFERENCE for the callback, corrupting readonly slots (enum singletons crashed on subsequent var_dump(); regular readonly properties were silently modifiable). By-ref callbacks now throw the same "Cannot acquire reference to readonly property" error as foreach; by-val callbacks copy the value into a temp so the slot stays intact.

[ndossche]

No. You're missing the point that it corrupts the enum and shm.

[iliaal]

Reworked. The fix is now in array_walk: check ZEND_ACC_READONLY before creating the reference, same check foreach already does. Enum properties are no longer corrupted, and regular readonly properties can no longer be silently modified through the callback.

[ndossche]

Just from looking at the code I think that you can violate the type of a typed property, e.g. put a string where an int is expected.

[iliaal]

No. You're missing the point that it corrupts the enum and shm.

Was a little to quick fixing the symptom as opposed to the cause 🤦 Here goes attempt #2

[ndossche]

I think the canonical message is Cannot indirectly modify readonly property Foo::$prop

E.g. from:

<?phpclass Foo {
publicreadonlyint$prop;
publicfunction__construct() {
$this->prop = 1;
}
}
$foo = newFoo;
$ref =& $foo->prop;

[iliaal]

foreach by-ref uses "Cannot acquire reference to readonly property", direct $ref =& $foo->prop uses "Cannot indirectly modify readonly property."

class Foo {
publicreadonlyint$prop;
publicfunction__construct() { $this->prop = 1; }
}
$foo = newFoo;
foreach ($fooas &$v) {}
// Error: Cannot acquire reference to readonly property Foo::$prop

I matched the foreach wording since array_walk iterates properties and both are array related, but "Cannot indirectly modify" works too. @ndossche I can update the message, if you prefer, but there is a bit of inconsistency here

[ndossche]

Ok for me

[iluuu1994]

This is missing the same handling for private and public private(set).

Edit: Thinking about it, the fact that this can access private properties at all is long-standing and probably shouldn't be changed without an RFC...

class C {
publicfunction__construct(
public private(set) int$a = 1,
privateint$b = 1,
) {}
}
$c = newC;
array_walk($c, function(&$v) { $v++; });
var_dump($c);

object(C)#1 (2) {
["a"]=>
int(2)
["b":"C":private]=>
int(2)
}

But also, this breaks by-val array_walk() for readonly:

class C {
publicfunction__construct(
publicreadonlyint$a = 1,
) {}
}
$c = newC;
array_walk($c, function($v) { $v++; });
var_dump($c);

Fatal error: Uncaught Error: Cannot acquire reference to readonly property C::$a

It might be possible not converting each zv into a ref and replace this with an addref on the container. But I'm not sure. The key comment here from Nikita:

Ensure the value is a reference. Otherwise the location of the value may be freed.

When object properties are iterated, we need to make sure the object doesn't go away or we'll write to freed memory.

[iliaal]

Agree, private/public private(set) deferred to RFC. Updated PR, by-val now copies the value into a temp before MAKE_REF, slot stays intact.

[Girgias]

I'm planning on deprecating the iteration on objects as it is not consistent, see https://wiki.php.net/rfc/deprecations_php_8_6#passing_objects_for_array_parameter_of_array_walk_and_array_walk_recursive

[iliaal]

I guess if that goes through this is not needed then