Side effects after zend_fetch_property_address() / get_property_ptr_ptr() #15938
Labels
Comments
|
@arnaud-lb see also discussion of #13754 |
|
Indeed related, but cannot be solved in the same way as #13754 as no error handlers are involved. The same problem would generally exist for |
|
Very simple PoC. Details
diff --git a/Zend/tests/gh15938_001.phpt b/Zend/tests/gh15938_001.phpt
new file mode 100644
index 0000000000..c904c51b07
--- /dev/null
+++ b/Zend/tests/gh15938_001.phpt
@@ -0,0 +1,24 @@
+--TEST--
+GH-15938
+--FILE--
+<?php
+
+#[AllowDynamicProperties]
+class C {}
+
+$obj = new C();
+$obj->a = str_repeat('a', 10);
+$obj->a .= new class {
+ function __toString() {
+ global $obj;
+ unset($obj->a);
+ return str_repeat('c', 10);
+ }
+};
+
+var_dump($obj->a);
+
+?>
+--EXPECTF--
+Warning: Undefined property: C::$a in %s on line %d
+string(10) "cccccccccc"
diff --git a/Zend/tests/gh15938_002.phpt b/Zend/tests/gh15938_002.phpt
new file mode 100644
index 0000000000..3a98b9ae87
--- /dev/null
+++ b/Zend/tests/gh15938_002.phpt
@@ -0,0 +1,44 @@
+--TEST--
+GH-15938
+--FILE--
+<?php
+
+#[AllowDynamicProperties]
+class C {}
+
+$obj = new C();
+$obj->a = '';
+$obj->a .= new class {
+ function __toString() {
+ global $obj;
+ for ($i = 0; $i < 8; $i++) {
+ $obj->{$i} = 0;
+ }
+ return 'str';
+ }
+};
+
+var_dump($obj);
+
+?>
+--EXPECTF--
+object(C)#%d (9) {
+ ["a"]=>
+ string(3) "str"
+ ["0"]=>
+ int(0)
+ ["1"]=>
+ int(0)
+ ["2"]=>
+ int(0)
+ ["3"]=>
+ int(0)
+ ["4"]=>
+ int(0)
+ ["5"]=>
+ int(0)
+ ["6"]=>
+ int(0)
+ ["7"]=>
+ int(0)
+}
diff --git a/Zend/tests/gh15938_003.phpt b/Zend/tests/gh15938_003.phpt
new file mode 100644
index 0000000000..120697c1d8
--- /dev/null
+++ b/Zend/tests/gh15938_003.phpt
@@ -0,0 +1,25 @@
+--TEST--
+GH-15938
+--FILE--
+<?php
+
+class C {
+ public $a;
+}
+
+$obj = new C();
+$obj->a = '';
+$obj->a .= new class {
+ function __toString() {
+ global $obj;
+ $obj = null;
+ return 'str';
+ }
+};
+
+?>
+--EXPECTF--
+Fatal error: Uncaught Error: Attempt to assign property "a" on null in %s:%d
+Stack trace:
+#0 {main}
+ thrown in %s on line %d
diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 63787c902f..68ec9b9d43 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -3683,6 +3683,12 @@ static void zend_compile_compound_assign(znode *result, zend_ast *ast) /* {{{ */
offset = zend_delayed_compile_begin();
zend_delayed_compile_prop(result, var_ast, BP_VAR_RW);
zend_compile_expr(&expr_node, expr_ast);
+ /* If the expression may contain objects and cause side-effects, emit
+ * a cast before compiling the lhs W fetches. */
+ if (opcode == ZEND_CONCAT && expr_ast->kind != ZEND_AST_ZVAL) {
+ zend_op *cast_opline = zend_emit_op_tmp(&expr_node, ZEND_CAST, &expr_node, NULL);
+ cast_opline->extended_value = IS_STRING;
+ }
opline = zend_delayed_compile_end(offset);
cache_slot = opline->extended_value;The same is likely needed for DIM, STATIC_PROP and VAR. Each could at least leak memory, if not crash. |
iluuu1994
added a commit
to iluuu1994/php-src
that referenced
this issue
Sep 19, 2024
Neither STATIC_PROP nor VAR are susceptible. Static vars cannot be unset. Unsetting vars only sets them to IS_UNDEF, including dynamic properties (`zend_hash_del_ind()` in the `ZEND_UNSET_VAR` handler). Fixes phpGH-15938
iluuu1994
added a commit
to iluuu1994/php-src
that referenced
this issue
Sep 19, 2024
Neither STATIC_PROP nor VAR are susceptible. Static vars cannot be unset. Unsetting vars only sets them to IS_UNDEF, including dynamic properties (`zend_hash_del_ind()` in the `ZEND_UNSET_VAR` handler). Fixes phpGH-15938
iluuu1994
added a commit
to iluuu1994/php-src
that referenced
this issue
Sep 19, 2024
Neither STATIC_PROP nor VAR are susceptible. Static vars cannot be unset. Unsetting vars only sets them to IS_UNDEF, including dynamic properties (`zend_hash_del_ind()` in the `ZEND_UNSET_VAR` handler). Fixes phpGH-15938
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
In some cases it is possible to modify the object or release it after acquiring the address of a property with zend_fetch_property_address() / get_property_ptr_ptr(), leading to corruptions.
Here we remove the
bkey from the zend_object.properties hashtable, and then write a zval to the slot. This leaks the zval:ASAN
Here the zend_object.properties ht is resized, so the address points to a freed block:
ASAN
Here the object is released, so the address points to a freed block:
ASAN
Using
ReflectionClass::resetAsLazy*()in__toString()in these cases will also cause similar issues.This issue exists with ASSIGN_OBJ_OP, ASSIGN_OBJ_REF, maybe ASSIGN_(PRE|POST)_(INC|DEC). I think we are aware of similar issues, but I'm not sure we were aware of these cases particularly.
The 3rd case may be resolved with a addref()/delref() of the object like we do in
php-src/Zend/zend_object_handlers.c
Line 1002 in 21196ca
But the two others are trickier. If conversion of the value is the only way to trigger a side effect after acquisition of the address, maybe we can ensure this occurs before that. However we don't always know the required conversion before fetching the property. Or can we prevent any operation on the object, with a flag or by setting zend_object.handlers to error-only handlers temporarily?
PHP Version
master
Operating System
No response
The text was updated successfully, but these errors were encountered: