Lazy objects: foreach edge cases by arnaud-lb · Pull Request #15960 · php/php-src

arnaud-lb · 2024-09-19T15:30:36Z

This fixes a few edge cases around foreach on lazy objects of non-iterable classes:

Deny resetAsLazy() on objects being iterated, as it complicates things and it makes no sense to reset an object during iteration. Moreover I'm not sure we can have a consistent behavior between ghosts and proxies (e.g. we may conserve the iteration position after reset for ghosts, but for proxies it seems difficult).
foreach() doesn't always use the get_properties handler, so lazy objects with a properties ht would not be initialized.
Iterating over proxies whose real instance is also a proxy was broken

To detect that an object is being iterated I rely on the fact that FE_RESET creates a hash iterator, so object->properties->u.v.nIteratorsCount is non-zero. However the get_iterator implementation for hooks does not initialize object->properties, and does not immediately create a hash iterator. Here I change it so that it always initializes object->properties and immediately creates a hash iterator.

/cc @iluuu1994

Depends on GH-15825.

…t fails

- Fix foreach on a object whose initialization failed before - Do not allow to reset an object during iteration

arnaud-lb · 2024-09-19T15:42:34Z

Zend/zend_property_hooks.c

+	if (UNEXPECTED(Z_TYPE(bucket->val) == IS_UNDEF)) {
+		return;
+	}


This happens when a dynamic property is unset:

<?php #[AllowDynamicProperties] class C { public int $a { &get { $ref = 1; return $ref; } } } $c = new C(); $c->dyn0 = 0; $c->dyn1 = 1; unset($c->dyn0); foreach ($c as $k => &$v) { var_dump($k); }

arnaud-lb · 2024-09-19T15:46:00Z

Zend/zend_vm_def.h

+			if (UNEXPECTED(zend_object_is_lazy(zobj))) {
+				zobj = zend_lazy_object_init(zobj);
+				if (UNEXPECTED(EG(exception))) {
+					UNDEF_RESULT();
+					FREE_OP1_IF_VAR();
+					HANDLE_EXCEPTION();
+				}
+			}


Normally we would rely on ->get_properties() to do the right thing, but ZEND_FE_RESET_R proceeds to separate object->properties after that.

iluuu1994

Sorry for the delay.

iluuu1994 · 2024-09-26T15:08:55Z

Zend/zend_lazy_objects.c

+
+static bool zlo_is_iterating(zend_object *object)
+{
+	if (object->properties && object->properties->u.v.nIteratorsCount) {


Can you use HT_ITERATORS_COUNT()?

iluuu1994 · 2024-09-26T15:13:29Z

Zend/zend_lazy_objects.c

 		GC_DEL_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);

-		/* unset() dynamic properties */
-		zend_object_dtor_dynamic_properties(obj);


I don't understand why this change is necessary. Reverting it to the previous code doesn't hit a test failure.

This is not fully related to foreach, and I should probably commit this change separately, but I realized that setting properties to null here could break some assumptions. For instance, we have code like this that would break if we did it:

if (!obj->properties) { rebuild_properties(obj); } do_something(); obj->properties->..

So, just removing the dynamic properties from the ht is safer.

Wouldn't that also mean you're potentially accessing the wrong properties?

More or less. If do_something() resets the object, the last line could potentially access a property of the object after it has been reset (the right property in the same object, but it has been reset). In the worse case this would allow to observe the state of a non-initialized lazy object.

I've made this change just in case, but it should not happen in practice because this kind of code is unsafe (before lazy objects). Normally we use the result of rebuild_properties() / get_properties() immediately without effects in between (except in # 15938 and maybe in ArrayObject). Outside of the VM and object handlers we use get_properies_for(), which doesn't have this problem because it increases the refcount.

NB: For proxies, this kind of code would be worse:

ht = get_properties(object) do_something() ht->...

because ht may belong to an other object, which may be released during reset. However this code is unsafe before lazy objects because ht can be released due to CoW separation. E.g.:

ht = get_properties(object) ht' = convert_to_array(object) // ADDREF(ht), ht' === ht foreach(object); // separates ht, so object->ht is a duplicate now and ht is RC=1 dtor(ht') // releases ht/ht' ht->... // UB

So we shouldn't have code like this.

iluuu1994 · 2024-09-26T15:18:45Z

Zend/zend_lazy_objects.c

 		ZEND_ASSERT(info->flags & ZEND_LAZY_OBJECT_INITIALIZED);
+		if (zend_object_is_lazy(info->u.instance)) {
+			return zend_lazy_object_init(info->u.instance);
+		}


I find it a bit surprising that we allow nesting lazy proxies. I suppose this is not preventable? If some initialized lazy proxy should become lazy again, I would expect resetAsLazyProxy to be called on the proxy, rather than the backed object. If this doesn't cause any other issues, then I suppose it's fine.

Yes, this can happen when resetting the real instance, e.g.:

$real = new C(); $proxy = $r->newLazyProxy(function () use ($real) { return $real; }); $r->resetAsLazyProxy($real, ...);

It would be possible to prevent this, but there may be valid use-cases where the caller of resetAsLazy() ignores that the object is the real instance of some proxy.

One possible issue would be cycles of proxies, but this is prevented as we don't allow to return a proxy from the factory.

iluuu1994 · 2024-09-26T15:31:49Z

Zend/zend_property_hooks.c

@@ -252,14 +254,11 @@ static void zho_it_move_forward(zend_object_iterator *iter)
 		if (zend_hash_has_more_elements(properties) != SUCCESS) {
 			hooked_iter->declared_props_done = true;


Can we also move this check to stay consistent between declared and dynamic props?

I've tried, but it's not trivial. If zho_it_move_forward() doesn't mark declared_props_done immediately we will call zho_declared_it_fetch_current(), and zho_it_move_forward() again. But this time we will increment the dynamic position and skip the first dynamic property.

We would also get similar issues if the client called ->move_forward() a few times without calling ->get_current().

We could simplify how we chose between declared and dynamic props. E.g. use declared ones when declared_props.nInternalPointer < declared_props.nNumUsed, and dynamic ones otherwise. We can then remove declared_props_done / dynamic_props_done:

diff --git a/Zend/zend_property_hooks.c b/Zend/zend_property_hooks.c index d701abdc3c5..c9d52f18c9f 100644 --- a/Zend/zend_property_hooks.c +++ b/Zend/zend_property_hooks.c @@ -25,9 +25,7 @@ typedef struct { zend_object_iterator it; bool by_ref; - bool declared_props_done; zval declared_props; - bool dynamic_props_done; uint32_t dynamic_prop_it; zval current_key; zval current_data; @@ -100,7 +98,6 @@ static void zho_dynamic_it_init(zend_hooked_object_iterator *hooked_iter) { zend_object *zobj = Z_OBJ_P(&hooked_iter->it.data); zend_array *properties = zobj->handlers->get_properties(zobj); - hooked_iter->dynamic_props_done = false; hooked_iter->dynamic_prop_it = zend_hash_iterator_add(properties, zho_num_backed_props(zobj)); } @@ -161,7 +158,6 @@ static void zho_dynamic_it_fetch_current(zend_object_iterator *iter) HashPosition pos = zend_hash_iterator_pos(hooked_iter->dynamic_prop_it, properties); if (pos >= properties->nNumUsed) { - hooked_iter->dynamic_props_done = true; return; } @@ -191,10 +187,12 @@ static void zho_it_fetch_current(zend_object_iterator *iter) return; } + zend_array *properties = Z_ARR(hooked_iter->declared_props); + while (true) { - if (!hooked_iter->declared_props_done) { + if (properties->nInternalPointer < properties->nNumUsed) { zho_declared_it_fetch_current(iter); - } else if (!hooked_iter->dynamic_props_done) { + } else if (zend_hash_iterator_pos(hooked_iter->dynamic_prop_it, Z_OBJ(iter->data)->properties) < Z_OBJ(iter->data)->properties->nNumUsed) { zho_dynamic_it_fetch_current(iter); } else { break; @@ -248,13 +246,10 @@ static void zho_it_move_forward(zend_object_iterator *iter) zval_ptr_dtor_nogc(&hooked_iter->current_key); ZVAL_UNDEF(&hooked_iter->current_key); - if (!hooked_iter->declared_props_done) { - zend_array *properties = Z_ARR(hooked_iter->declared_props); - zend_hash_move_forward(properties); - if (zend_hash_has_more_elements(properties) != SUCCESS) { - hooked_iter->declared_props_done = true; - } - } else if (!hooked_iter->dynamic_props_done) { + zend_array *properties = Z_ARR(hooked_iter->declared_props); + if (properties->nInternalPointer < properties->nNumUsed) { + properties->nInternalPointer++; + } else { zend_array *properties = Z_OBJ(iter->data)->properties; HashPosition pos = zend_hash_iterator_pos(hooked_iter->dynamic_prop_it, properties); pos++; @@ -271,13 +266,9 @@ static void zho_it_rewind(zend_object_iterator *iter) zval_ptr_dtor_nogc(&hooked_iter->current_key); ZVAL_UNDEF(&hooked_iter->current_key); - hooked_iter->declared_props_done = false; zend_array *properties = Z_ARR(hooked_iter->declared_props); zend_hash_internal_pointer_reset(properties); - hooked_iter->dynamic_props_done = false; - if (hooked_iter->dynamic_prop_it != (uint32_t) -1) { - EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = zho_num_backed_props(Z_OBJ(iter->data)); - } + EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = zho_num_backed_props(Z_OBJ(iter->data)); } static HashTable *zho_it_get_gc(zend_object_iterator *iter, zval **table, int *n) @@ -318,7 +309,6 @@ ZEND_API zend_object_iterator *zend_hooked_object_get_iterator(zend_class_entry ZVAL_OBJ_COPY(&iterator->it.data, zobj); iterator->it.funcs = &zend_hooked_object_it_funcs; iterator->by_ref = by_ref; - iterator->declared_props_done = false; zend_array *properties = zho_build_properties_ex(zobj, true, false); ZVAL_ARR(&iterator->declared_props, properties); zho_dynamic_it_init(iterator);

WDYT? This may be for an other PR however.

We could simplify how we chose between declared and dynamic props. E.g. use declared ones when declared_props.nInternalPointer < declared_props.nNumUsed, and dynamic ones otherwise.

This would make more sense now that we always compute the properties hashtable. But we can do this in a separate PR.

iluuu1994 · 2024-09-26T15:35:11Z

Zend/zend_property_hooks.c

+	}
+
 	if (hooked_iter->by_ref && Z_TYPE(bucket->val) != IS_REFERENCE) {
 		ZEND_ASSERT(Z_TYPE(bucket->val) != IS_UNDEF);


This assert can be removed now.

iluuu1994 · 2024-09-27T11:37:44Z

Zend/zend_property_hooks.c

-	if (hooked_iter->dynamic_prop_it != (uint32_t) -1) {
-		EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = zho_num_backed_props(Z_OBJ(iter->data));
-	}
+	ZEND_ASSERT(hooked_iter->dynamic_prop_it != (uint32_t) -1);


This is no longer assigned -1 anywhere, so this assert is kinda useless.

* PHP-8.4: [ci skip] NEWS for GH-15960 Deny resetting an object as lazy during property iteration Ensure to initialize lazy object in foreach Do not null out obj->properties when resetting object Fix handling of undef property during foreach by ref on hooked class

Fix zend_lazy_object_get_properties for object with prop ht, when ini…

5cc8db8

…t fails

github-actions bot added the Category: Engine label Sep 19, 2024

arnaud-lb force-pushed the lazy-foreach-edge-cases branch from cf55481 to 75c65bf Compare September 19, 2024 15:40

Fix lazy object foreach edge cases

720ed56

- Fix foreach on a object whose initialization failed before - Do not allow to reset an object during iteration

arnaud-lb force-pushed the lazy-foreach-edge-cases branch from 75c65bf to 720ed56 Compare September 19, 2024 15:47

arnaud-lb marked this pull request as ready for review September 19, 2024 17:17

arnaud-lb requested review from dstogov and iluuu1994 as code owners September 19, 2024 17:17

arnaud-lb commented Sep 19, 2024

View reviewed changes

arnaud-lb changed the base branch from master to PHP-8.4 September 25, 2024 11:15

iluuu1994 reviewed Sep 26, 2024

View reviewed changes

Review

e6f22ba

github-actions bot added the ABI break label Sep 27, 2024

arnaud-lb removed the ABI break label Sep 27, 2024

iluuu1994 reviewed Sep 27, 2024

View reviewed changes

Remove useless checks

643aeaf

github-actions bot added the ABI break label Sep 27, 2024

arnaud-lb added a commit that referenced this pull request Oct 3, 2024

[ci skip] NEWS for GH-15960

e02e6be

arnaud-lb closed this in c9dfb77 Oct 3, 2024

		@@ -252,14 +254,11 @@ static void zho_it_move_forward(zend_object_iterator *iter)
		if (zend_hash_has_more_elements(properties) != SUCCESS) {
		hooked_iter->declared_props_done = true;

Conversation

arnaud-lb commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arnaud-lb commented Sep 19, 2024 •

edited

Loading

arnaud-lb Sep 26, 2024 •

edited

Loading