diff --git a/NEWS b/NEWS index 8bd02dc6d6ac..286ffcda14ee 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ PHP NEWS ?? ??? ????, PHP 8.4.28 - Core: + . Fixed bug GH-24280 (Object foreach by reference bypasses asymmetric setter + visibility). (Xauele) . Fixed bug GH-24184 (System ID not being reinitialized after an Apache graceful restart, potentially causing incompatible OPcache file cache entries to be reused). (Levi Morrison) diff --git a/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref.phpt b/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref.phpt new file mode 100644 index 000000000000..8eb4379e2527 --- /dev/null +++ b/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref.phpt @@ -0,0 +1,132 @@ +--TEST-- +GH-24280: foreach by reference must respect asymmetric setter visibility +--FILE-- +value; + } +} + +class PrivateSetterChild extends PrivateSetter { + public function updateFromChild(): void { + foreach ($this as &$value) { + $value = 'child'; + } + unset($value); + } +} + +class ProtectedSetter { + public protected(set) int $value = 10; +} + +class ProtectedSetterChild extends ProtectedSetter { + public function updateInside(): void { + foreach ($this as &$value) { + ++$value; + } + unset($value); + } +} + +class PublicSetter { + public string $value = 'initial'; +} + +class ReadonlyProperty { + public readonly int $value; + + public function __construct() { + $this->value = 1; + } +} + +function acquireReference(object $object): void { + try { + foreach ($object as &$value) { + if (is_string($value)) { + $value = 'outside'; + } else { + ++$value; + } + } + unset($value); + echo "accepted\n"; + } catch (Error $error) { + echo $error->getMessage(), "\n"; + } +} + +$private = new PrivateSetter(); +echo "private outside: "; +acquireReference($private); +var_dump($private->value); +$private->updateInside(); +var_dump($private->value); + +$existingReference = new PrivateSetter(); +$existingReference->prepareReference(); +if (ReflectionReference::fromArrayElement((array) $existingReference, 'value') === null) { + throw new LogicException('Test setup failed: value is not a persistent reference'); +} +echo "existing reference outside: "; +acquireReference($existingReference); +var_dump($existingReference->value); +$existingReference->updateInside(); +var_dump($existingReference->value); + +$privateChild = new PrivateSetterChild(); +echo "private child: "; +try { + $privateChild->updateFromChild(); + echo "accepted\n"; +} catch (Error $error) { + echo $error->getMessage(), "\n"; +} +var_dump($privateChild->value); + +$protected = new ProtectedSetterChild(); +echo "protected outside: "; +acquireReference($protected); +var_dump($protected->value); +$protected->updateInside(); +var_dump($protected->value); + +$public = new PublicSetter(); +echo "public outside: "; +acquireReference($public); +var_dump($public->value); + +$readonly = new ReadonlyProperty(); +echo "readonly outside: "; +acquireReference($readonly); +var_dump($readonly->value); +?> +--EXPECT-- +private outside: Cannot indirectly modify private(set) property PrivateSetter::$value from global scope +string(8) "original" +string(6) "inside" +existing reference outside: Cannot indirectly modify private(set) property PrivateSetter::$value from global scope +string(8) "original" +string(6) "inside" +private child: Cannot indirectly modify private(set) property PrivateSetter::$value from scope PrivateSetterChild +string(8) "original" +protected outside: Cannot indirectly modify protected(set) property ProtectedSetter::$value from global scope +int(10) +int(11) +public outside: accepted +string(7) "outside" +readonly outside: Cannot acquire reference to readonly property ReadonlyProperty::$value +int(1) diff --git a/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref_controls.phpt b/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref_controls.phpt new file mode 100644 index 000000000000..b004ac2e4851 --- /dev/null +++ b/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref_controls.phpt @@ -0,0 +1,164 @@ +--TEST-- +GH-24280: foreach by-reference preserves legal writes, typed reference guards and hook errors +--FILE-- +hidden; + } +} + +#[AllowDynamicProperties] +class DynamicControl { + public string $regular = 'initial'; +} + +class TypedControl { + public int $value = 1; +} + +class HookedControl { + public string $value { + get { return 'hook'; } + set { } + } +} + +$private = new OwnedPrivateControl(); +$private->modify(); +echo "private own: {$private->value}\n"; + +$protected = new OwnedProtectedChildControl(); +$protected->modify(); +echo "protected child: {$protected->value}\n"; + +$invisible = new InvisibleControl(); +foreach ($invisible as &$entry) { + $entry = 'changed'; +} +unset($entry); +echo "private get hidden: {$invisible->getHidden()}\n"; +echo "public shown: {$invisible->visible}\n"; + +$dynamic = new DynamicControl(); +$dynamic->extra = 'created'; +foreach ($dynamic as &$entry) { + $entry = 'changed'; +} +unset($entry); +echo "dynamic: {$dynamic->regular}, {$dynamic->extra}\n"; + +$typed = new TypedControl(); +foreach ($typed as &$entry) { + try { + $entry = 'not-an-int'; + } catch (TypeError $exception) { + echo "typed-ref: rejected\n"; + } + $entry = 42; +} +unset($entry); +echo "typed: {$typed->value}\n"; + +$std = (object) ['value' => 'first']; +foreach ($std as &$entry) { + $entry = 'second'; +} +unset($entry); +echo "std: {$std->value}\n"; + +try { + $hooked = new HookedControl(); + foreach ($hooked as &$entry) { + $entry = 'nope'; + } + unset($entry); + echo "hooked: wrongly accepted\n"; +} catch (Error $exception) { + echo "hooked: rejected\n"; +} + +trait ScopedIterationControl { + public function modifyFromTrait(): void { + foreach ($this as &$value) { $value = 12; } + unset($value); + } +} +class ScopedOwnerControl { + use ScopedIterationControl; + public private(set) int $value = 1; + public function delegateToGlobal(): void { globalIterationControl($this); } +} +class ScopedOutsiderControl {} +function globalIterationControl(object $object): void { + foreach ($object as &$value) { $value = 13; } + unset($value); +} +$owner = new ScopedOwnerControl(); +$bound = Closure::bind(static function (ScopedOwnerControl $object): void { + foreach ($object as &$value) { $value = 12; } + unset($value); +}, null, ScopedOwnerControl::class); +$bound($owner); +echo "bound owner: {$owner->value}\n"; +$outsider = Closure::bind($bound, null, ScopedOutsiderControl::class); +try { $outsider($owner); throw new LogicException('Forbidden bound scope accepted'); } +catch (Error $error) { echo "bound outsider: rejected\n"; } +try { $owner->delegateToGlobal(); throw new LogicException('Global scope inherited caller permission'); } +catch (Error $error) { echo "global caller: rejected\n"; } +$owner->modifyFromTrait(); +echo "trait owner: {$owner->value}\n"; +$reflection = new ReflectionClass(ScopedOwnerControl::class); +$ghost = $reflection->newLazyGhost(static function (ScopedOwnerControl $object): void {}); +$proxy = $reflection->newLazyProxy(static fn (): ScopedOwnerControl => new ScopedOwnerControl()); +foreach (['ghost' => $ghost, 'proxy' => $proxy] as $name => $object) { + try { globalIterationControl($object); throw new LogicException('Forbidden lazy scope accepted'); } + catch (Error $error) { echo "lazy {$name}: rejected\n"; } + if ($object->value !== 1) { throw new LogicException('Lazy property was corrupted'); } + $bound($object); + if ($object->value !== 12) { throw new LogicException('Legal lazy write was rejected'); } +} +?> +--EXPECT-- +private own: after +protected child: 8 +private get hidden: secret +public shown: changed +dynamic: changed, changed +typed-ref: rejected +typed: 42 +std: second +hooked: rejected +bound owner: 12 +bound outsider: rejected +global caller: rejected +trait owner: 12 +lazy ghost: rejected +lazy proxy: rejected diff --git a/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref_stress.phpt b/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref_stress.phpt new file mode 100644 index 000000000000..59a15b2f7961 --- /dev/null +++ b/Zend/tests/asymmetric_visibility/gh24280_foreach_by_ref_stress.phpt @@ -0,0 +1,60 @@ +--TEST-- +GH-24280: repeatedly deny scoped property references while preserving legal partial writes +--FILE-- +restricted; + } + + public function updateInternally(): void { + foreach ($this as &$value) { + if (is_int($value)) { + $value = 8; + } + } + unset($value); + } +} + +function attemptExternalMutation(ReferenceStressObject $object): void { + try { + foreach ($object as &$value) { + if (is_string($value)) { + $value = 'after'; + } else { + $value = 99; + } + } + unset($value); + throw new LogicException('Forbidden reference accepted'); + } catch (Error $error) { + if (!str_contains($error->getMessage(), 'Cannot indirectly modify private(set) property ReferenceStressObject::$restricted')) { + throw $error; + } + unset($value); + } + if ($object->restricted !== 7 || $object->visible !== 'after') { + throw new LogicException('Incorrect property state after denied reference'); + } +} + +for ($i = 0; $i < 3000; $i++) { + $object = new ReferenceStressObject(); + if (($i & 1) === 0) { + $object->retain(); + } + attemptExternalMutation($object); + $object->updateInternally(); + if ($object->restricted !== 8) { + throw new LogicException('Legal in-class reference was rejected'); + } +} +echo "3000 scoped reference checks and legal writes: PASS\n"; +?> +--EXPECT-- +3000 scoped reference checks and legal writes: PASS diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index df255c5a868f..afb748c117d7 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -7296,22 +7296,30 @@ ZEND_VM_HANDLER(126, ZEND_FE_FETCH_RW, VAR, ANY, JMP_ADDR) value_type = Z_TYPE_INFO_P(value); if (EXPECTED(value_type != IS_UNDEF) && EXPECTED(zend_check_property_access(Z_OBJ_P(array), p->key, 0) == SUCCESS)) { - if ((value_type & Z_TYPE_MASK) != IS_REFERENCE) { - zend_property_info *prop_info = - zend_get_property_info_for_slot(Z_OBJ_P(array), value); - if (prop_info) { - if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { - zend_throw_error(NULL, - "Cannot acquire reference to readonly property %s::$%s", - ZSTR_VAL(prop_info->ce->name), ZSTR_VAL(p->key)); - UNDEF_RESULT(); - HANDLE_EXCEPTION(); - } - if (ZEND_TYPE_IS_SET(prop_info->type)) { - ZVAL_NEW_REF(value, value); - ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(value), prop_info); - value_type = IS_REFERENCE_EX; - } + zend_property_info *prop_info = + zend_get_property_info_for_slot(Z_OBJ_P(array), value); + if (prop_info) { + if ((value_type & Z_TYPE_MASK) != IS_REFERENCE + && UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { + zend_throw_error(NULL, + "Cannot acquire reference to readonly property %s::$%s", + ZSTR_VAL(prop_info->ce->name), ZSTR_VAL(p->key)); + UNDEF_RESULT(); + HANDLE_EXCEPTION(); + } + /* An existing reference must not bypass setter visibility. */ + if (UNEXPECTED(prop_info->flags & ZEND_ACC_PPP_SET_MASK) + && (UNEXPECTED(EG(fake_scope) != NULL) || prop_info->ce != EX(func)->common.scope) + && UNEXPECTED(!zend_asymmetric_property_has_set_access(prop_info))) { + zend_asymmetric_visibility_property_modification_error(prop_info, "indirectly modify"); + UNDEF_RESULT(); + HANDLE_EXCEPTION(); + } + if ((value_type & Z_TYPE_MASK) != IS_REFERENCE + && ZEND_TYPE_IS_SET(prop_info->type)) { + ZVAL_NEW_REF(value, value); + ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(value), prop_info); + value_type = IS_REFERENCE_EX; } } break; diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index 4b313f20cd39..ddda741c2a2b 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -23211,22 +23211,30 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_FE_FETCH_RW_SPEC_VAR_HANDLER(Z value_type = Z_TYPE_INFO_P(value); if (EXPECTED(value_type != IS_UNDEF) && EXPECTED(zend_check_property_access(Z_OBJ_P(array), p->key, 0) == SUCCESS)) { - if ((value_type & Z_TYPE_MASK) != IS_REFERENCE) { - zend_property_info *prop_info = - zend_get_property_info_for_slot(Z_OBJ_P(array), value); - if (prop_info) { - if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { - zend_throw_error(NULL, - "Cannot acquire reference to readonly property %s::$%s", - ZSTR_VAL(prop_info->ce->name), ZSTR_VAL(p->key)); - UNDEF_RESULT(); - HANDLE_EXCEPTION(); - } - if (ZEND_TYPE_IS_SET(prop_info->type)) { - ZVAL_NEW_REF(value, value); - ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(value), prop_info); - value_type = IS_REFERENCE_EX; - } + zend_property_info *prop_info = + zend_get_property_info_for_slot(Z_OBJ_P(array), value); + if (prop_info) { + if ((value_type & Z_TYPE_MASK) != IS_REFERENCE + && UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { + zend_throw_error(NULL, + "Cannot acquire reference to readonly property %s::$%s", + ZSTR_VAL(prop_info->ce->name), ZSTR_VAL(p->key)); + UNDEF_RESULT(); + HANDLE_EXCEPTION(); + } + /* An existing reference must not bypass setter visibility. */ + if (UNEXPECTED(prop_info->flags & ZEND_ACC_PPP_SET_MASK) + && (UNEXPECTED(EG(fake_scope) != NULL) || prop_info->ce != EX(func)->common.scope) + && UNEXPECTED(!zend_asymmetric_property_has_set_access(prop_info))) { + zend_asymmetric_visibility_property_modification_error(prop_info, "indirectly modify"); + UNDEF_RESULT(); + HANDLE_EXCEPTION(); + } + if ((value_type & Z_TYPE_MASK) != IS_REFERENCE + && ZEND_TYPE_IS_SET(prop_info->type)) { + ZVAL_NEW_REF(value, value); + ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(value), prop_info); + value_type = IS_REFERENCE_EX; } } break;