Fix GH-9618: drain __wakeup/__unserialize queue on unserialize failure - #21716
Open
iliaal wants to merge 1 commit into
Open
Fix GH-9618: drain __wakeup/__unserialize queue on unserialize failure#21716iliaal wants to merge 1 commit into
iliaal wants to merge 1 commit into
Conversation
Contributor
|
I think there's a bug when unserialize calls are nested. I vibe-coded the following code to show it: <?php
/*
* Reproduces a bug in the GH-9618 fix (commit 6c5407a).
*
* The fix calls var_invoke_delayed_calls() on the failure path of
* php_unserialize_with_options(). When a Serializable::unserialize()
* method calls unserialize() internally, the nested call shares the
* same php_unserialize_data_t with the outer call (BG(serialize_lock)==0,
* BG(unserialize).level>1). If the nested call fails,
* var_invoke_delayed_calls() drains ALL deferred __wakeup calls in the
* shared hash — including those belonging to the outer call's objects.
*
* Correct output (A::__wakeup runs during var_destroy, after all parsing):
* B::unserialize complete
* A::__wakeup
* bool(true)
*
* Buggy output (A::__wakeup drained prematurely by inner failure):
* A::__wakeup
* B::unserialize complete
* bool(true)
*/
class A
{
public function __wakeup()
{
echo "A::__wakeup\n";
}
}
class B implements Serializable
{
public $state;
public function serialize(): string
{
return 'x';
}
public function unserialize($data): void
{
@unserialize('O:1:"X":{BAD');
echo "B::unserialize complete\n";
$this->state = 'ready';
}
}
// Outer payload: array [A, B].
// A is parsed first (its __wakeup is queued in the dtor hash).
// B is parsed second (Serializable -> B::unserialize()).
// B::unserialize triggers a failing nested unserialize, which
// (incorrectly) drains A's __wakeup from the shared dtor hash.
$payload = 'a:2:{i:0;O:1:"A":0:{}i:1;C:1:"B":1:{x}}';
var_dump(@unserialize($payload)); |
iliaal
force-pushed
the
fix/gh9618-unserialize-wakeup-drain
branch
from
September 3, 2026 12:03
6c5407a to
67111fe
Compare
iliaal
added a commit
to iliaal/php-src
that referenced
this pull request
Sep 3, 2026
…lure unserialize() defers __wakeup and __unserialize until parsing ends and drains them in PHP_VAR_UNSERIALIZE_DESTROY, but the failure path runs zval_ptr_dtor(return_value) first, so a destructor of the partially-built value can reach a sibling object still in its pre-wakeup state and bypass whatever invariant that wakeup installs. Drain the queue before that dtor, scoped to the outermost call so a failing nested unserialize() leaves the outer call's queue alone. Fixes phpGH-9618 Closes phpGH-21716
…lure unserialize() defers __wakeup and __unserialize until parsing ends and drains them in PHP_VAR_UNSERIALIZE_DESTROY, but the failure path runs zval_ptr_dtor(return_value) first, so a destructor of the partially-built value can reach a sibling object still in its pre-wakeup state and bypass whatever invariant that wakeup installs. Drain before that dtor. The var dtor hash is shared with nested unserialize() calls and is append-only, so the drain starts from the tail position recorded before parsing and covers only the entries this call queued. Fixes phpGH-9618 Closes phpGH-21716
iliaal
force-pushed
the
fix/gh9618-unserialize-wakeup-drain
branch
from
September 3, 2026 12:20
67111fe to
bd5ef5a
Compare
Member
Author
|
Ah, indeed. Fixed, though not the way I first tried: scoping the drain to |
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9618
unserialize()defers__wakeup()and__unserialize()calls until the entire payload has been parsed, then drains them insidePHP_VAR_UNSERIALIZE_DESTROYat the end ofphp_unserialize_with_options. On the failure path,zval_ptr_dtor(return_value)atext/standard/var.c:1469runs first and triggers destructors of the partially-built return value. A destructor that touches a sibling object sees that sibling in its pre-wakeup state, bypassing any invariant installed in__wakeup()or__unserialize().The POC uses a malformed property key length to abort unserialization after constructing both
A(with a destructor) andB(whose__wakeup()overwrites a field thatA::__destructlater passes toeval()). Before this fix,A::__destructruns first, reachesBwith its pre-wakeup value, andeval()executes the attacker-controlled string.Drain the deferred-call queue on the failure path, before the
zval_ptr_dtor(return_value)that triggers destructors.__wakeup()and__unserialize()now run first, so sibling destructors see the post-wakeup state.Implementation:
var_drain_entry()invar_unserializer.refactors the per-slot drain out ofvar_destroy(). Bothvar_destroy()and the newvar_invoke_delayed_calls()share one implementation.var_invoke_delayed_calls()walks the var dtor hash and callsvar_drain_entry()on each slot, clearing theVAR_WAKEUP_FLAG/VAR_UNSERIALIZE_FLAGmarkers so a subsequentvar_destroy()skips already-drained entries.php_unserialize_with_optionscallsvar_invoke_delayed_calls()on the failure branch, beforezval_ptr_dtor(return_value).Success path is unchanged. Tests cover both the
__wakeupand__unserializedrain paths; both fail on unfixed master and pass after the fix.