diff --git a/NEWS b/NEWS index 23212414d361..a63ee2c83dc9 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,8 @@ PHP NEWS . Deprecate specifying a nullable return type for __debugInfo(). (timwolla) . Fixed bug GH-22142 (Assertion failure in zendi_try_get_long() on IS_UNDEF). (David Carlier) + . Fixed GH-12695 (Wrong magic methods sequence with ?? operator). + (nicolas-grekas) - BCMath: . Added NUL-byte validation to BCMath functions. (jorgsowa) diff --git a/UPGRADING b/UPGRADING index 3d6c89e90e05..fd8de8eb45dc 100644 --- a/UPGRADING +++ b/UPGRADING @@ -19,6 +19,11 @@ PHP 8.6 UPGRADE NOTES 1. Backward Incompatible Changes ======================================== +- Core: + . ??/empty() on a magic property no longer call __get() when __isset() + has materialised the property by writing into the property table. + The freshly-written value is returned directly. isset() is unaffected. + - DOM: . Properties previously documented as @readonly (e.g. DOMNode::$nodeType, DOMDocument::$xmlEncoding, DOMEntity::$actualEncoding, ::$encoding, diff --git a/Zend/tests/magic_methods/gh12695.phpt b/Zend/tests/magic_methods/gh12695.phpt new file mode 100644 index 000000000000..6a4f9d94ee71 --- /dev/null +++ b/Zend/tests/magic_methods/gh12695.phpt @@ -0,0 +1,65 @@ +--TEST-- +GH-12695: __get() is not called under `??` when __isset() materialised the property +--FILE-- +$n = 123; + return true; + } +} + +echo "Dynamic property materialised in __isset, then `??`:\n"; +$a = new A; +var_dump($a->foo ?? 'fallback'); + +echo "\nSame on a declared (unset) property:\n"; +class B { + public int $x = 99; + public function __get($n) { + throw new Exception("__get must not be called when __isset materialised the property"); + } + public function __isset($n) { + echo " __isset($n)\n"; + $this->$n = 7; + return true; + } +} +$b = new B; +unset($b->x); +var_dump($b->x ?? 'fallback'); + +echo "\nWhen __isset() materialises the property to null, `??` falls back:\n"; +#[AllowDynamicProperties] +class D { + public function __get($n) { + throw new Exception("__get must not be called when __isset materialised the property"); + } + public function __isset($n) { + echo " __isset($n)\n"; + $this->$n = null; + return true; + } +} +$d = new D; +var_dump($d->foo ?? 'fallback'); + +?> +--EXPECT-- +Dynamic property materialised in __isset, then `??`: + __isset(foo) +int(123) + +Same on a declared (unset) property: + __isset(x) +int(7) + +When __isset() materialises the property to null, `??` falls back: + __isset(foo) +string(8) "fallback" diff --git a/Zend/tests/magic_methods/gh12695_empty.phpt b/Zend/tests/magic_methods/gh12695_empty.phpt new file mode 100644 index 000000000000..064ea02e20e8 --- /dev/null +++ b/Zend/tests/magic_methods/gh12695_empty.phpt @@ -0,0 +1,55 @@ +--TEST-- +GH-12695: empty() also benefits from the post-__isset() materialization re-check +--FILE-- +$n = $GLOBALS['next_value']; + return true; + } +} + +echo "1) empty() when __isset materialised a truthy value: __get is not called, empty=false:\n"; +$GLOBALS['next_value'] = 7; +$a = new A; +var_dump(empty($a->foo)); + +echo "\n2) empty() when __isset materialised a falsy value: __get is not called, empty=true:\n"; +$GLOBALS['next_value'] = 0; +$a = new A; +var_dump(empty($a->bar)); + +echo "\n3) empty() with no materialization: __get is still called (legacy path preserved):\n"; +class B { + public function __get($n) { + echo " __get($n)\n"; + return 'value'; + } + public function __isset($n) { + echo " __isset($n)\n"; + return true; + } +} +$b = new B; +var_dump(empty($b->any)); + +?> +--EXPECT-- +1) empty() when __isset materialised a truthy value: __get is not called, empty=false: + __isset(foo) +bool(false) + +2) empty() when __isset materialised a falsy value: __get is not called, empty=true: + __isset(bar) +bool(true) + +3) empty() with no materialization: __get is still called (legacy path preserved): + __isset(any) + __get(any) +bool(false) diff --git a/Zend/tests/magic_methods/gh12695_isset_unchanged.phpt b/Zend/tests/magic_methods/gh12695_isset_unchanged.phpt new file mode 100644 index 000000000000..41b933d5a74a --- /dev/null +++ b/Zend/tests/magic_methods/gh12695_isset_unchanged.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-12695: isset() itself is unchanged (still does not consult __get) +--FILE-- +any)); + +?> +--EXPECT-- + __isset(any) +bool(true) diff --git a/Zend/tests/magic_methods/gh12695_no_materialization.phpt b/Zend/tests/magic_methods/gh12695_no_materialization.phpt new file mode 100644 index 000000000000..0cd12b5cac87 --- /dev/null +++ b/Zend/tests/magic_methods/gh12695_no_materialization.phpt @@ -0,0 +1,65 @@ +--TEST-- +GH-12695: when __isset() does not materialise the property, __get() is still called +--FILE-- +any ?? 'fallback'); + +echo "\n2) `??` when __isset=true and __get returns null: __get is called and fallback is used:\n"; +class D { + public function __get($n) { + echo " __get($n)\n"; + return null; + } + public function __isset($n) { + echo " __isset($n)\n"; + return true; + } +} +$d = new D; +var_dump($d->any ?? 'fallback'); + +echo "\n3) `??` when __isset returns false: __get is not called:\n"; +class E { + public function __get($n) { + throw new Exception("__get must not be called when __isset returned false"); + } + public function __isset($n) { + echo " __isset($n)\n"; + return false; + } +} +$e = new E; +var_dump($e->any ?? 'fallback'); + +?> +--EXPECT-- +1) `??` when __isset=true and __get returns a value: __get is called: + __isset(any) + __get(any) +string(8) "from-get" + +2) `??` when __isset=true and __get returns null: __get is called and fallback is used: + __isset(any) + __get(any) +string(8) "fallback" + +3) `??` when __isset returns false: __get is not called: + __isset(any) +string(8) "fallback" diff --git a/Zend/tests/magic_methods/gh12695_object_released_in_isset.phpt b/Zend/tests/magic_methods/gh12695_object_released_in_isset.phpt new file mode 100644 index 000000000000..ed5bc1a39aa3 --- /dev/null +++ b/Zend/tests/magic_methods/gh12695_object_released_in_isset.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-12695: object freed by __isset() during materialization +--FILE-- +prop = 'materialised'; + return true; + } +} + +$obj = new C(); +unset($obj->prop); +var_dump($obj->prop ?? 'fb'); + +?> +--EXPECT-- +string(12) "materialised" diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index e7ddd466a51a..373a93315cc5 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -934,6 +934,35 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int } zval_ptr_dtor(&tmp_result); + + /* __isset() may have materialised the property by writing into + * the property table. Re-check it before deferring to __get(), + * so the freshly-written value is returned directly without a + * redundant __get() call (GH-12695). The value is copied into + * `rv` because the property table can be freed by the OBJ_RELEASE + * below (e.g. when __isset() drops the last external reference + * to the object). */ + if (IS_VALID_PROPERTY_OFFSET(property_offset)) { + retval = OBJ_PROP(zobj, property_offset); + if (Z_TYPE_P(retval) != IS_UNDEF) { + ZVAL_COPY(rv, retval); + retval = rv; + OBJ_RELEASE(zobj); + goto exit; + } + } else if (IS_DYNAMIC_PROPERTY_OFFSET(property_offset)) { + if (zobj->properties != NULL) { + retval = zend_hash_find(zobj->properties, name); + if (retval) { + ZVAL_COPY(rv, retval); + retval = rv; + OBJ_RELEASE(zobj); + goto exit; + } + } + } + retval = &EG(uninitialized_zval); + if (zobj->ce->__get && !((*guard) & IN_GET)) { goto call_getter; } @@ -2487,7 +2516,20 @@ ZEND_API int zend_std_has_property(zend_object *zobj, zend_string *name, int has result = zend_is_true(&rv); zval_ptr_dtor(&rv); if (has_set_exists == ZEND_PROPERTY_NOT_EMPTY && result) { - if (EXPECTED(!EG(exception)) && zobj->ce->__get && !((*guard) & IN_GET)) { + /* GH-12695, see above. */ + zval *prop = NULL; + if (IS_VALID_PROPERTY_OFFSET(property_offset)) { + prop = OBJ_PROP(zobj, property_offset); + if (Z_TYPE_P(prop) == IS_UNDEF) { + prop = NULL; + } + } else if (IS_DYNAMIC_PROPERTY_OFFSET(property_offset) + && zobj->properties != NULL) { + prop = zend_hash_find(zobj->properties, name); + } + if (prop) { + result = i_zend_is_true(prop); + } else if (EXPECTED(!EG(exception)) && zobj->ce->__get && !((*guard) & IN_GET)) { (*guard) |= IN_GET; zend_std_call_getter(zobj, name, &rv); (*guard) &= ~IN_GET;