Fix GH-21687: array_walk creates references to readonly properties#21690
Fix GH-21687: array_walk creates references to readonly properties#21690iliaal wants to merge 1 commit into
Conversation
ndossche
left a comment
There was a problem hiding this comment.
No. You're missing the point that it corrupts the enum and shm.
56b2b9f to
1cbea32
Compare
|
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. |
|
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. |
Was a little to quick fixing the symptom as opposed to the cause 🤦 Here goes attempt #2 |
| ZVAL_NEW_REF(zv, zv); | ||
| ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(zv), prop_info); | ||
| if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { | ||
| zend_throw_error(NULL, |
There was a problem hiding this comment.
I think the canonical message is Cannot indirectly modify readonly property Foo::$prop
E.g. from:
<?php
class Foo {
public readonly int $prop;
public function __construct() {
$this->prop = 1;
}
}
$foo = new Foo;
$ref =& $foo->prop;There was a problem hiding this comment.
foreach by-ref uses "Cannot acquire reference to readonly property", direct $ref =& $foo->prop uses "Cannot indirectly modify readonly property."
class Foo {
public readonly int $prop;
public function __construct() { $this->prop = 1; }
}
$foo = new Foo;
foreach ($foo as &$v) {}
// Error: Cannot acquire reference to readonly property Foo::$propI 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
There was a problem hiding this comment.
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 {
public function __construct(
public private(set) int $a = 1,
private int $b = 1,
) {}
}
$c = new C;
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 {
public function __construct(
public readonly int $a = 1,
) {}
}
$c = new C;
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.
By-ref callbacks throw the same "Cannot acquire reference to readonly property" error foreach uses. By-val callbacks copy the value into a temp so MAKE_REF does not corrupt the slot. Closes phpGH-21687
1cbea32 to
c2c6730
Compare
|
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. |
|
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 |
|
I guess if that goes through this is not needed then |
Fixes #21687
array_walk()wrapped object properties inIS_REFERENCEfor the callback, corrupting readonly slots (enum singletons crashed on subsequentvar_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.