[thediex]

Fixes #24763.

A bailout during destruction of LIBXML(entity_loader_callback) can interrupt zend_fcc_dtor() before the stored FCC is cleared, leaving the module-global callback in a stale state.

This change detaches the FCC from LIBXML(entity_loader_callback) before destroying it, both when replacing the external entity loader and during libxml request shutdown.

A regression test is included.

Validation performed:

• PHP-8.4 unpatched: ASan heap-use-after-free reproduced.
• PHP-8.4 patched: 0/10 ASan UAF reproductions; libxml_get_external_entity_loader() returned null in all 10 runs.
• Regression test: FAIL on unpatched PHP-8.4, PASS with the fix.
• ext/libxml test suite on patched PHP-8.4: 23 passed, 0 failed, 8 skipped.
• git diff --check: clean.

[Girgias]

Surely it makes more sense to fix this once inside of zend_fcc_dtor() rather than every call site with confusing code with zero comment about why a copy is needed, especially as this function already sets the FCC to empty_fcall_info_cache.

I would appreciate a bit more global thinking.

[thediex]

Thanks, that makes sense. I’ll rework the fix so the FCC is cleared centrally in zend_fcc_dtor() before releasing the referenced objects, and keep the libxml regression test to verify the original issue.

[iliaal]

Moving the reset to the top of zend_fcc_dtor() breaks holders whose destructors can still read them during the release, since those readers don't check ZEND_FCC_INITIALIZED. This segfaults with that change and runs fine on master:

$ctx = stream_context_create();
class N {
    public function __invoke() {}
    public function __destruct() {
        global $ctx;
        echo count(stream_context_get_params($ctx)), "\n";
    }
}
stream_context_set_params($ctx, ['notification' => new N()]);
stream_context_set_params($ctx, ['notification' => function () {}]);

Same for spl_autoload_functions() from a loader's destructor during spl_autoload_unregister(). So either every such reader learns to handle an empty FCC, or the holder detaches before destroying, as this PR does.

[iliaal]

The replace path still leaks if the old loader's destructor registers a loader: after the detach, zend_fcc_dup() overwrites whatever the destructor installed. This leaks two allocations on a debug build:

class D { public function __destruct() { var_dump(libxml_get_external_entity_loader()); libxml_set_external_entity_loader(fn() => null); } }
$d = new D();
$f = function () use ($d) {};
unset($d);
libxml_set_external_entity_loader($f);
unset($f);
libxml_set_external_entity_loader(fn() => 1);

Installing the new callback before releasing the old one avoids it:

zend_fcall_info_cache old_fcc = LIBXML(entity_loader_callback);
LIBXML(entity_loader_callback) = empty_fcall_info_cache;
if (ZEND_FCI_INITIALIZED(fci)) { /* argument not null */
    zend_fcc_dup(&LIBXML(entity_loader_callback), &fcc);
}
if (ZEND_FCC_INITIALIZED(old_fcc)) {
    zend_fcc_dtor(&old_fcc);
}

This should target master, since bailouts from destructors aren't backported. The test can run in-process with zend_trigger_bailout() from zend_test and --EXPECTF-- instead of proc_open() and the deprecated E_USER_ERROR.