[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.

The callback must be detached from the module-global FCC before destruction. When replacing an existing loader, the new callback is installed before destroying the old one so that re-entrant calls from the old callback's destructor are preserved and do not leak the callback installed during re-entry.

The same detach-before-destruction behavior is used during libxml request shutdown.

Regression coverage includes:

• A bailout during callback destruction, using zend_trigger_bailout() from zend_test.
• Re-entrant replacement of the entity loader from the old callback's destructor.

Validation performed on current master with an ASan debug build:

• Bailout regression test: PASS.
• Re-entrant replacement regression test: PASS.
• ext/libxml test suite: 27 passed, 0 failed, 8 skipped.
• ASan/LeakSanitizer re-entrancy reproducer: clean; no assertion, use-after-free, or memory leak detected.
• Final loader installed during destructor re-entry is preserved.
• 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.

[thediex]

Updated the PR based on the review feedback.

The PR now targets master. I kept the detach at the libxml holder rather than changing zend_fcc_dtor() globally.

For the replacement path, the new callback is installed before releasing the old FCC, so destructor re-entrancy is preserved without leaking or overwriting the callback installed during the destructor.

I also:

• changed the bailout regression test to use zend_trigger_bailout() from zend_test in-process;
• added a regression test for re-entrant loader replacement;
• added comments explaining the detach/destruction ordering.

Local validation on current master:

• both regression tests: PASS
• ext/libxml: 27 passed, 0 failed, 8 skipped
• ASan/LeakSanitizer re-entrancy reproducer: clean, with the re-entrant loader preserved

Thanks for the review and for the re-entrancy reproducer.

[ndossche]

Arnaud & co are working on a general solution to handle these kinds of issues, I think it's better to wait for a general solution rather than patch everything as that's just whack-a-mole.

[Girgias]

I'm once again seriously unconvinced that fixing this sort of nonsense userland code is a good use of our time. We are making code harder to read, reason about, and slower for PHP code nobody writes.

This requires a global solution and until that is found I'm vetoing such changes.