Skip to content

Fix GH-20404: crash when modifying weak references during destruction #22390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
khaledalam wants to merge 1 commit into
base: master
Choose a base branch
from
+70 −21
Open

Fix GH-20404: crash when modifying weak references during destruction #22390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions Zend/tests/weakrefs/gh20404.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--TEST--
GH-20404: Crash when adding object to WeakMap during destruction
--FILE--
<?php

$w = new WeakMap;

$o = new stdClass;
$w[$o] = new class($r) {
function __construct(public &$r) {}
function __destruct() {
global $refs;
$o = $this->r->get();
for ($i = 0; $i < 8; ++$i) {
$r = new WeakMap;
$r[$o] = 1;
$refs[] = $r;
}
}
};
$r = WeakReference::create($o);

echo "END OF SCRIPT\n";
?>
--EXPECT--
END OF SCRIPT
65 changes: 44 additions & 21 deletions Zend/zend_weakrefs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,22 +80,6 @@ static inline void zend_weakref_unref_single(
}
}

static void zend_weakref_unref(zend_object *object, void *tagged_ptr) {
void *ptr = ZEND_WEAKREF_GET_PTR(tagged_ptr);
uintptr_t tag = ZEND_WEAKREF_GET_TAG(tagged_ptr);
if (tag == ZEND_WEAKREF_TAG_HT) {
HashTable *ht = ptr;
ZEND_HASH_MAP_FOREACH_PTR(ht, tagged_ptr) {
zend_weakref_unref_single(
ZEND_WEAKREF_GET_PTR(tagged_ptr), ZEND_WEAKREF_GET_TAG(tagged_ptr), object);
} ZEND_HASH_FOREACH_END();
zend_hash_destroy(ht);
FREE_HASHTABLE(ht);
} else {
zend_weakref_unref_single(ptr, tag, object);
}
}

static void zend_weakref_register(zend_object *object, void *payload) {
GC_ADD_FLAGS(object, IS_OBJ_WEAKLY_REFERENCED);

Expand Down Expand Up @@ -213,13 +197,52 @@ void zend_weakrefs_notify(zend_object *object) {
/* Annoyingly we can't use the HT destructor here, because we need access to the key (which
* is the object address), which is not provided to the dtor. */
const zend_ulong obj_key = zend_object_to_weakref_key(object);
void *tagged_ptr = zend_hash_index_find_ptr(&EG(weakrefs), obj_key);
void *tagged_ptr;

#if ZEND_DEBUG
ZEND_ASSERT(tagged_ptr && "Tracking of the IS_OBJ_WEAKLY_REFERENCE flag should be precise");
ZEND_ASSERT(zend_hash_index_find_ptr(&EG(weakrefs), obj_key)
&& "Tracking of the IS_OBJ_WEAKLY_REFERENCE flag should be precise");
#endif
if (tagged_ptr) {
zend_weakref_unref(object, tagged_ptr);
zend_hash_index_del(&EG(weakrefs), obj_key);

/* Clearing a weak reference may run user code: destroying a WeakMap value can invoke a
* destructor, which may in turn register new -- or unregister still-pending -- weak
* references to this same object (see GH-20404). To remain correct under such reentrancy
* we never hold an iteration cursor across that user code. Instead we repeatedly detach a
* single pending entry from EG(weakrefs) and clear it, re-reading EG(weakrefs) each time,
* until nothing remains for this object. Detaching before clearing ensures a reentrant
* registration creates a fresh entry rather than mutating (and possibly reallocating or
* freeing) the structure we are about to use. */
while ((tagged_ptr = zend_hash_index_find_ptr(&EG(weakrefs), obj_key)) != NULL) {
uintptr_t tag = ZEND_WEAKREF_GET_TAG(tagged_ptr);
void *ptr = ZEND_WEAKREF_GET_PTR(tagged_ptr);

if (tag != ZEND_WEAKREF_TAG_HT) {
/* A single WeakReference, WeakMap or bare HashTable entry. */
zend_hash_index_del(&EG(weakrefs), obj_key);
zend_weakref_unref_single(ptr, tag, object);
continue;
}

/* Multiple entries for this object. Pop exactly one and detach it before clearing it,
* so that reentrancy which grows the HashTable (new registration) or removes entries
* from it (unregistration triggered by a destructor) cannot corrupt our traversal. */
HashTable *ht = ptr;
void *entry = NULL;
ZEND_HASH_MAP_FOREACH_PTR(ht, entry) {
break;
} ZEND_HASH_FOREACH_END();
ZEND_ASSERT(entry && "TAG_HT HashTable should never be empty");

zend_hash_index_del(ht, (zend_ulong) entry);
if (zend_hash_num_elements(ht) == 0) {
zend_hash_destroy(ht);
FREE_HASHTABLE(ht);
zend_hash_index_del(&EG(weakrefs), obj_key);
}

/* Do this last, as it may run a destructor and re-enter. */
zend_weakref_unref_single(
ZEND_WEAKREF_GET_PTR(entry), ZEND_WEAKREF_GET_TAG(entry), object);
}
}

Expand Down
Loading