[iliaal]

Fixes #9618

unserialize() defers __wakeup() and __unserialize() calls until the entire payload has been parsed, then drains them inside PHP_VAR_UNSERIALIZE_DESTROY at the end of php_unserialize_with_options. On the failure path, zval_ptr_dtor(return_value) at ext/standard/var.c:1469 runs 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) and B (whose __wakeup() overwrites a field that A::__destruct later passes to eval()). Before this fix, A::__destruct runs first, reaches B with its pre-wakeup value, and eval() 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:

• New static var_drain_entry() in var_unserializer.re factors the per-slot drain out of var_destroy(). Both var_destroy() and the new var_invoke_delayed_calls() share one implementation.
• var_invoke_delayed_calls() walks the var dtor hash and calls var_drain_entry() on each slot, clearing the VAR_WAKEUP_FLAG / VAR_UNSERIALIZE_FLAG markers so a subsequent var_destroy() skips already-drained entries.
• php_unserialize_with_options calls var_invoke_delayed_calls() on the failure branch, before zval_ptr_dtor(return_value).

Success path is unchanged. Tests cover both the __wakeup and __unserialize drain paths; both fail on unfixed master and pass after the fix.

[Sjord]

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]

Ah, indeed. Fixed, though not the way I first tried: scoping the drain to BG(unserialize).level <= 1 left the original bypass reachable one level down, since the nested root's destructor is deferred to the owner's var_destroy(), which drains and destructs per entry. The hash is append-only, so it now records the tail before parsing and drains from there. Your reproducer and the nested bypass added to tests

[bukka]

Looks like that latest fix introduced unsafe ordering:

1. Outer object A is fully parsed and its __unserialize() is queued.
2. A C: handler starts a failing nested parse which queues B::__unserialize().
3. B contains an r: reference to outer A.
4. It invokes only B, before A.
5. B::__unserialize() can access A in its pre-unserialize state, recreating the same primitive Fix GH-19860: Finalize nested unserialize callbacks #25055 fixes.

I think it should wait for GH-25055 and then use its global pending callback drain. The per call mark should only determine if the failure requires draining and not change the callback order.

[iliaal]

Wow, oldie forgot about this one, seems way out of date now, I'll need to review its relevance now.

[iliaal]

Confirmed, the tail drain runs B::__unserialize() first. I'll wait for #25055 and rebase onto its drain.