FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix GH-23385: Use-after-free in SplDoublyLinkedList::serialize() by Alb3e3 · Pull Request #23387 · php/php-src · GitHub

/ php-src Public

Fix GH-23385: Use-after-free in SplDoublyLinkedList::serialize() - #23387

Open
Alb3e3 wants to merge 2 commits into
php:PHP-8.4from
Alb3e3:fix-gh23385-dllist-serialize-uaf-84
Open

Fix GH-23385: Use-after-free in SplDoublyLinkedList::serialize()#23387
Alb3e3 wants to merge 2 commits into
php:PHP-8.4from
Alb3e3:fix-gh23385-dllist-serialize-uaf-84

Conversation

Alb3e3 commented Aug 19, 2026

Copy link
Copy Markdown

Fixes GH-23385.

The SPL side

SplDoublyLinkedList::serialize() walks the list and passes each element's zval straight to php_var_serialize():

	while (current) {
		next = current->next;
		SPL_LLIST_CHECK_ADDREF(next);
		php_var_serialize(&buf, &current->data, &var_hash);
		SPL_LLIST_CHECK_DELREF_EX(next, break;);
		current = next;
	}

Serializing an element can re-enter userland (__serialize, __sleep, Serializable::serialize), and that code can remove the element that is being serialized. The loop already anticipates this for the next element but not for the current one, so offsetUnset() drops the last reference and frees it while php_var_serialize() still uses &current->data as its struc argument.

The element is already built to outlive its removal from the list, see the "Keep consistency if element is kept alive" branch in offsetUnset(), so taking a reference for the duration of the call is all that is needed.

Why there are two commits

The reproducer in the issue frees two different things, and only one of them is the SPL bug.

offsetUnset() also destroys element->data, which drops the array's last reference while php_var_serialize_nested_data() is iterating it. That half was already fixed on master by cc8abaf (GH-22714), which holds a ref on the HashTable across the walk, but that commit never made it to PHP-8.4/8.5. So on this branch the issue's reproducer still crashes there even with the SPL fix applied.

The first commit is that backport, unchanged apart from the surrounding incomplete_class argument style on this branch, together with its test. Please drop it if you would rather merge cc8abaf up yourself; the second commit stands on its own and merges up cleanly.

Verification

Built PHP-8.4 (--disable-all --enable-debug) with ASan and USE_ZEND_ALLOC=0.

Before, the issue's reproducer:

==1803822==ERROR: AddressSanitizer: heap-use-after-free
READ of size 4 ... php_var_serialize_nested_data var.c:1009
freed by ... zim_SplDoublyLinkedList_offsetUnset spl_dllist.c:758

After, both that reproducer and a variant that hits only the SPL half (a nested array after the object, so the walk dereferences struc again once the element is gone) run clean, with no leaks reported.

ext/spl/ and ext/standard/tests/serialize/: 920 pass, same 4 pre-existing failures as an unpatched checkout of this branch in the same ASan build (RecursiveIteratorIterator_dtor_order, bug79710, bug67247, bug77751). Unpatched: 918 pass, so the delta is exactly the two added tests.

The new gh23385.phpt covers three shapes: an element removing itself, an element removing its successor, and an element clearing the whole list.

Note on AI use

I used Claude Code while working on this. I reproduced the crash, wrote and reviewed the change, and ran the test suites myself.

Alb3e3 added 2 commits August 19, 2026 21:56
The IS_ARRAY case of php_var_serialize_intern() walked the array's
HashTable without holding a reference across
php_var_serialize_nested_data(), which recurses into user hooks
(__serialize, __sleep, Serializable::serialize). A hook that grows the
same array through a by-reference alias reallocs the backing store
mid-walk, so the iterator reads freed memory. Hold a ref across the walk,
as the object path and var_dump/var_export already do, so the append
separates a copy instead of reallocating in place.

This is a backport of cc8abaf (phpGH-22714), which landed on master only.
phpGH-23385 needs it on this branch as well.
SplDoublyLinkedList::serialize() walks the list and hands each element's
zval to php_var_serialize(). Serializing an element can call back into
userland (__serialize, __sleep, Serializable::serialize), and that code
can remove the very element being serialized from the list.

The loop already takes a reference on the *next* element for exactly this
reason, but not on the current one. Once offsetUnset() drops the last
reference to it the element is freed, while php_var_serialize() still
holds a pointer into it as its `struc` argument and dereferences it for
every value it walks.

Take a reference on the current element as well, so it survives the call.
The element is already designed to outlive its removal from the list, see
the "Keep consistency if element is kept alive" path in offsetUnset().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL