fix(#594): detach query back-refs before transaction struct efree - #598
Merged
Conversation
php_fbird_free_query_rsrc reads fb_query->trans->open_cursor_count (_php_fbird_cursor_closed, #566 bookkeeping) at request shutdown. The fbird_transaction struct, however, is efree'd by three other owners: _php_fbird_commit_link (default tx on link close), _php_fbird_free_trans (le_trans dtor), and fbird_execute_auto's temp-trans error paths. Whichever ran first left every live query's ->trans dangling - ASAN heap-use-after-free in issue582/issue591 tests under php83-asan. Fix: intrusive per-transaction registry of live fbird_query back-refs, mirroring the #591 statement sweep one layer up: - fbird_transaction gains query_head; fbird_query gains trans_reg (list we are enrolled in) + trans_reg_next (intrusive link). trans_reg is kept separate from the semantic fb_query->trans backref, which fbird_execute_query may NULL to force a default-tx restart (#294/#566) - membership survives that reset. - Queries enroll at _php_fbird_prepare AND at both result-resource creation sites that copy the parent's trans pointer. - Every efree site calls _php_fbird_trans_detach_queries() first (9 sites: link-close default-tx efree, le_trans dtor incl. fork-child early return, execute_auto x5). - Default-tx restart reassignment unregs/reg around the swap. query_head init added at all 7 fbird_transaction creation sites (emalloc'd, not zeroed). Registry hangs off the request-owned structs themselves - no thread_local state, so no #593-class ZTS gap. ASAN negative control: pre-fix runs reproduced heap-use-after-free READ of open_cursor_count in zend_close_rsrc_list; post-fix clean. Known follow-up: the fix unmasks a pre-existing LSAN leak (drop_db orphans the fb::Transaction wrapper after attachment death) - filed as #597; the two tests scope LSAN off via --ENV-- while keeping UAF detection armed.
3 tasks
ponytail-review of PR #598: after the unconditional unreg, a failing _php_fbird_def_trans() could leave the live query unregistered while still holding ->trans (def_trans creates/finds the tx struct before its failure exits) - link-close would later efree it under our backref, the exact UAF class this PR kills. Register on the post-call pointer regardless of outcome. Also: relocate detach-behavior comment onto _php_fbird_trans_detach_ queries; shorten duplicated ASAN --ENV-- rationale in both tests. Follow-up filed: #599 (batch->trans same UAF class, out of scope here).
Closed
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
1 participant
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.
Closes #594.
Problem
Query resources keep a raw `fb_query->trans` backref used for cursor-count bookkeeping at dtor time. Three other owners efree the `fbird_transaction` struct (link close / le_trans dtor / execute_auto temps); whichever ran first left live queries with dangling backrefs -> ASAN heap-use-after-free at shutdown (issue582/issue591 under php83-asan).
Fix
Intrusive per-transaction query registry (the #591 sweep pattern one layer up): enroll at prepare + result-resource creation, unregister at query free, detach-and-efree at all 9 trans free sites. No thread_local state (no #593 gap). Restart-reassignment keeps membership in sync.
Verification