fix(thread): deep-copy nested closures per worker to stop a cross-thread run_time_cache UAF (#176)#180
Merged
Merged
Conversation
…ead run_time_cache UAF (#176) A closure transferred to a worker thread got a per-worker copy of its top-level op_array, but its nested closures (op_array.dynamic_func_defs) stayed shared with the persistent snapshot: the load-path memcpy only reset the top-level run_time_cache. Instantiated on more than one worker, zend_create_closure writes a per-thread arena run_time_cache pointer into that shared op_array — read from another worker, or after a retired worker's arena is freed (a long-lived coroutine spawned in a ThreadPool bootloader across reload()) — a cross-thread use-after-free that SEGVs. async_thread_create_closure now self-contains the whole op_array via op_array_to_emalloc whenever it carries nested definitions, so every nested def becomes worker-local (immutable cleared, fresh run_time_cache). op_array_to_emalloc additionally duplicates the nested static_variables template — the snapshot stores it as a persistent immutable array (refcount 2), which destroy_op_array cannot free — into a worker-local mutable copy. The transfer path (closure_transfer_obj) now only detaches the top-level when there are no nested defs, since the load path already did the full copy. Closures without nested definitions keep the cheap borrow and are unaffected. Adds tests/thread_pool/078 (bootloader nested-closure coroutine across reload) and 079 (submitted tasks declaring nested closures per worker); both SEGV without the fix and pass clean under ASAN.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 #176.
Problem
A closure transferred to a worker thread got a per-worker copy of its top-level op_array, but its nested closures (
op_array.dynamic_func_defs) stayed shared with the persistent snapshot — the load-pathmemcpyonly reset the top-levelrun_time_cache. Instantiated on more than one worker,zend_create_closurewrites a per-thread arenarun_time_cachepointer into that shared op_array, which is then read from another worker — or after a retired worker's arena is freed (a long-lived coroutine spawned in aThreadPoolbootloader acrossreload()) — a cross-thread use-after-free that SEGVs.Confirmed with ASAN + gdb: the top-level handler/bootloader op_arrays are distinct per worker, the nested op_array is the same address on every worker.
Fix (
thread.c)async_thread_create_closure(shared load path — bootloader / entry / submit): self-contain the whole op_array viaop_array_to_emalloconly when it carries nested defs, so every nested def becomes worker-local (IMMUTABLE cleared, freshrun_time_cache). Closures without nested definitions keep the cheap borrow — no cost on the hot task path.op_array_to_emalloc: duplicate the nestedstatic_variables— the snapshot stores it as a persistent immutable array (refcount 2) thatdestroy_op_arraycan't free — into a worker-local mutable copy.closure_transfer_obj: itsop_array_to_emallocis now gated onnum_dynamic_func_defs == 0to avoid a double-copy, since the load path already does the full copy for nested-def closures.The
refcount != NULLearly-return indestroy_op_arrayis why localizing nested defs requires full self-containization; the coroutine-outlives-bootloader lifecycle is handled correctly by refcounting (verified under ASAN).Verification
thread_pool/078(bootloader nested-closure coroutine acrossreload()) and079(submitted tasks declaring nested closures per worker) — both SEGV without the fix, pass clean under ASAN.thread+thread_poolsuite: 153 tests, 0 failed (no regressions).