From 14488eaaf61ba5edc30b0f6226d1283714c570a0 Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 11:15:00 +0200 Subject: [PATCH 1/2] fix(#594): detach query back-refs before transaction struct efree 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. --- fbird_connection.c | 3 ++ fbird_query_exec.c | 11 ++++ fbird_query_prepare.c | 6 +++ fbird_transaction.c | 54 ++++++++++++++++++++ firebird.c | 1 + php_fbird_includes.h | 19 +++++++ tests/issue582_plink_revitalization.phpt | 8 +++ tests/issue591_stmt_survives_conn_death.phpt | 8 +++ 8 files changed, 110 insertions(+) diff --git a/fbird_connection.c b/fbird_connection.c index c9cc6d96..0e4c01b0 100644 --- a/fbird_connection.c +++ b/fbird_connection.c @@ -106,6 +106,9 @@ void _php_fbird_commit_link(fbird_db_link *link) _php_fbird_error(status); } } + /* Issue #594: live query resources may still back-reference this + * struct - detach them before the efree. */ + _php_fbird_trans_detach_queries(p->trans); efree(p->trans); /* default transaction is not a registered resource: clean up */ } else { /* Non-default transaction: rollback via OO API */ diff --git a/fbird_query_exec.c b/fbird_query_exec.c index 4e7eb707..f6268956 100755 --- a/fbird_query_exec.c +++ b/fbird_query_exec.c @@ -191,6 +191,7 @@ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *a trans->is_default = false; /* Issue #554 */ trans->open_cursor_count = 0; /* Issue #566 */ trans->retain_committed = false; /* Issue #586 */ + trans->query_head = NULL; /* Issue #594 */ trans->stored_tpb_len = 0; /* SET TRANSACTION TPB not stored here */ trans->fbt_transaction = new_trans; trans->db_link[0] = fb_query->link; @@ -291,10 +292,12 @@ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *a if (fb_query->trans && fb_query->trans->fbt_transaction == NULL && fb_query->trans_res == NULL && fb_query->link && fb_query->link->fbc_connection) { + _php_fbird_trans_unreg_query(fb_query); /* #594: leaving old registry */ fb_query->trans = NULL; /* force _php_fbird_def_trans to restart */ if (SUCCESS != _php_fbird_def_trans(fb_query->link, &fb_query->trans)) { return FAILURE; /* _php_fbird_def_trans already reported the error */ } + _php_fbird_trans_reg_query(fb_query->trans, fb_query); /* #594 */ } /* Issue #540/#566: For DDL on explicit transactions, transparently commit+restart @@ -684,6 +687,7 @@ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *a result_query->link = fb_query->link; result_query->trans = fb_query->trans; result_query->trans_res = fb_query->trans_res; + _php_fbird_trans_reg_query(result_query->trans, result_query); /* #594 */ result_query->dialect = fb_query->dialect; result_query->statement_type = fb_query->statement_type; result_query->out_fields_count = fb_query->out_fields_count; @@ -876,6 +880,7 @@ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *a result_query->link = fb_query->link; result_query->trans = fb_query->trans; result_query->trans_res = fb_query->trans_res; + _php_fbird_trans_reg_query(result_query->trans, result_query); /* #594 */ result_query->dialect = fb_query->dialect; result_query->statement_type = fb_query->statement_type; result_query->out_fields_count = fb_query->out_fields_count; @@ -2001,6 +2006,7 @@ PHP_FUNCTION(fbird_execute_auto) trans->is_default = false; /* Issue #554 */ trans->open_cursor_count = 0; /* Issue #566 */ trans->retain_committed = false; /* Issue #586 */ + trans->query_head = NULL; /* Issue #594 */ trans->stored_tpb_len = 0; /* Temp trans: no stored TPB */ trans->fbt_transaction = oo_trans; trans->db_link[0] = link; @@ -2009,6 +2015,7 @@ PHP_FUNCTION(fbird_execute_auto) /* Prepare */ if (FAILURE == _php_fbird_prepare(&fb_query, link, trans, NULL, sql)) { fbt_rollback(oo_trans, status); + _php_fbird_trans_detach_queries(trans); efree(trans); RETURN_FALSE; } @@ -2025,6 +2032,7 @@ PHP_FUNCTION(fbird_execute_auto) zend_list_delete(fb_query->res); // Frees statement fbt_rollback(oo_trans, status); + _php_fbird_trans_detach_queries(trans); efree(trans); RETURN_FALSE; } @@ -2042,6 +2050,7 @@ PHP_FUNCTION(fbird_execute_auto) zend_list_delete(Z_RES_P(return_value)); zend_list_delete(fb_query->res); fbt_rollback(oo_trans, status); + _php_fbird_trans_detach_queries(trans); efree(trans); RETURN_THROWS(); } @@ -2056,10 +2065,12 @@ PHP_FUNCTION(fbird_execute_auto) /* Commit via OO API (returns 0 on success, non-zero on error) */ if (fbt_commit(oo_trans, status)) { _php_fbird_error(status); + _php_fbird_trans_detach_queries(trans); efree(trans); RETURN_FALSE; } + _php_fbird_trans_detach_queries(trans); efree(trans); /* Restore return value (affected rows count) */ diff --git a/fbird_query_prepare.c b/fbird_query_prepare.c index c77a2914..0e4278fc 100755 --- a/fbird_query_prepare.c +++ b/fbird_query_prepare.c @@ -132,6 +132,8 @@ void _php_fbird_free_query(fbird_query *fb_query) if (fb_query->in_msg_buffer) efree(fb_query->in_msg_buffer); /* Metadata references are released when statement is freed - no efree needed here */ + /* Issue #594: leave the transaction registry before the struct dies. */ + _php_fbird_trans_unreg_query(fb_query); efree(fb_query); } @@ -293,6 +295,10 @@ int _php_fbird_prepare(fbird_query **new_query, fbird_db_link *link, fb_query->link = link; fb_query->trans = trans; fb_query->trans_res = trans_res; + /* Issue #594: enroll on the transaction registry so its death (link + * close / le_trans dtor / execute_auto temp efree) can NULL our + * backref instead of leaving it dangling into efree'd memory. */ + _php_fbird_trans_reg_query(trans, fb_query); fb_query->dialect = link->dialect; fb_query->query = estrdup(query); /* This prepared query owns the statement handle and is responsible for diff --git a/fbird_transaction.c b/fbird_transaction.c index b06d5810..8f2af52f 100644 --- a/fbird_transaction.c +++ b/fbird_transaction.c @@ -56,6 +56,54 @@ fbird_transaction *_php_fbird_find_default_trans(fbird_db_link *link) return NULL; } +/* Issue #594: detach all live fbird_query back-references before this + * struct is efree'd (default-tx efree in _php_fbird_commit_link, the + * le_trans dtor, execute_auto temp-trans paths). Query dtors later skip + * their transaction bookkeeping through the existing fb_query->trans + * NULL-guards instead of reading freed memory. */ +/* Issue #594: enroll a query on its transaction's back-ref registry. */ +void _php_fbird_trans_reg_query(fbird_transaction *trans, fbird_query *q) +{ + if (!trans) { + return; + } + q->trans_reg = trans; + q->trans_reg_next = trans->query_head; + trans->query_head = q; +} + +/* Issue #594: remove a query from whatever registry it is enrolled in + * (query free, default-tx restart reassignment). */ +void _php_fbird_trans_unreg_query(fbird_query *q) +{ + if (!q->trans_reg) { + return; + } + fbird_query **curr = &q->trans_reg->query_head; + while (*curr) { + if (*curr == q) { + *curr = q->trans_reg_next; + break; + } + curr = &(*curr)->trans_reg_next; + } + q->trans_reg = NULL; + q->trans_reg_next = NULL; +} + +void _php_fbird_trans_detach_queries(fbird_transaction *trans) +{ + fbird_query *q = trans->query_head; + while (q) { + fbird_query *next = q->trans_reg_next; + q->trans = NULL; + q->trans_reg = NULL; + q->trans_reg_next = NULL; + q = next; + } + trans->query_head = NULL; +} + void _php_fbird_free_trans(zend_resource *rsrc) { ISC_STATUS status[256]; @@ -68,6 +116,7 @@ void _php_fbird_free_trans(zend_resource *rsrc) /* Fork-safety check (Issue #22): Skip cleanup if we're in a forked child */ if (FBG(init_pid) != 0 && getpid() != FBG(init_pid)) { FBDEBUG("Skipping transaction cleanup in forked child process"); + _php_fbird_trans_detach_queries(trans); efree(trans); return; } @@ -109,6 +158,7 @@ void _php_fbird_free_trans(zend_resource *rsrc) } } } + _php_fbird_trans_detach_queries(trans); efree(trans); } @@ -380,6 +430,7 @@ PHP_FUNCTION(fbird_trans_start) fb_trans->is_default = false; /* Issue #554 */ fb_trans->open_cursor_count = 0; /* Issue #566 */ fb_trans->retain_committed = false; /* Issue #586 */ + fb_trans->query_head = NULL; /* Issue #594 */ fb_trans->stored_tpb_len = tpb_len; if (tpb_len > 0) { memcpy(fb_trans->stored_tpb, last_tpb, tpb_len); @@ -1052,6 +1103,7 @@ PHP_FUNCTION(fbird_trans) fb_trans->is_default = false; /* Issue #554 */ fb_trans->open_cursor_count = 0; /* Issue #566 */ fb_trans->retain_committed = false; /* Issue #586 */ + fb_trans->query_head = NULL; /* Issue #594 */ /* Store TPB for restart (#566 review finding #4). */ if (link_cnt == 1 && link0_tpb_len > 0) { fb_trans->stored_tpb_len = link0_tpb_len; @@ -1115,6 +1167,7 @@ PHP_FUNCTION(fbird_trans) fb_trans->is_default = false; /* Issue #554 */ fb_trans->open_cursor_count = 0; /* Issue #566 */ fb_trans->retain_committed = false; /* Issue #586 */ + fb_trans->query_head = NULL; /* Issue #594 */ /* Store TPB for restart (#566 review finding #4). * For single-db: use tpb_len + last_tpb (function scope). * For multi-db: stored_tpb_len=0 (multi-db TPB is complex). */ @@ -1162,6 +1215,7 @@ int _php_fbird_def_trans(fbird_db_link *fb_link, fbird_transaction **trans) tr->affected_rows = 0; tr->open_cursor_count = 0; /* Issue #566 */ tr->retain_committed = false; /* Issue #586 */ + tr->query_head = NULL; /* Issue #594 */ tr->stored_tpb_len = 0; tr->fbt_transaction = NULL; tr->is_default = true; /* Issue #554 */ diff --git a/firebird.c b/firebird.c index 9d0c5a54..14c74eed 100755 --- a/firebird.c +++ b/firebird.c @@ -1512,6 +1512,7 @@ PHP_FUNCTION(fbird_reconnect_transaction) fb_trans->affected_rows = 0; fb_trans->is_default = false; /* Issue #554 */ fb_trans->open_cursor_count = 0; /* Issue #566 */ + fb_trans->query_head = NULL; /* Issue #594 */ fb_trans->retain_committed = false; /* PR #588 review: garbage-true flag * would idle-release (= hard commit) an in-doubt limbo transaction */ fb_trans->stored_tpb_len = 0; /* Reconnect: default TPB */ diff --git a/php_fbird_includes.h b/php_fbird_includes.h index a92a935d..762029ed 100755 --- a/php_fbird_includes.h +++ b/php_fbird_includes.h @@ -158,6 +158,12 @@ typedef struct { * restarts so retained relation locks do not outlive the cursors * that justified retaining them. */ bool retain_committed; + /* Issue #594: intrusive registry of live fbird_query back-references. + * Queries register at prepare (_php_fbird_prepare) and unregister at + * free; every site that efrees this struct calls + * _php_fbird_trans_detach_queries() first so no query dtor can read + * freed memory (open_cursor_count bookkeeping at request shutdown). */ + struct _fb_query *query_head; fbird_db_link *db_link[1]; /* last member */ } fbird_transaction; @@ -261,6 +267,12 @@ typedef struct _fb_query { struct _fb_query *parent; struct _fb_query *child_head; struct _fb_query *child_next; + /* Issue #594: transaction-registry membership. trans_reg is the list we + * are enrolled in (set at prepare, cleared only by unregister/detach - + * independent of the semantic fb_query->trans backref, which + * fbird_query_exec.c may NULL to force a default-tx restart). */ + fbird_transaction *trans_reg; + struct _fb_query *trans_reg_next; /* OO API statement wrapper (fb::Statement* from fbs_prepare()) */ void *fbs_statement; void *fbs_resultset; /* OO API IResultSet* for cursor operations */ @@ -513,6 +525,13 @@ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *a * Best-effort: hard-commits + restarts a retain_committed transaction whose * last cursor just closed. Errors are absorbed (state stays as-before-fix). */ void _php_fbird_trans_release_if_idle(fbird_transaction *trans); + +/* Issue #594: null out every live fbird_query back-reference to this + * transaction before the struct is efree'd (link close default-tx efree, + * le_trans dtor, execute_auto temp-trans paths). */ +void _php_fbird_trans_detach_queries(fbird_transaction *trans); +void _php_fbird_trans_reg_query(fbird_transaction *trans, fbird_query *q); +void _php_fbird_trans_unreg_query(fbird_query *q); /* Issue #586: hard commit + transparent restart with stored TPB. * Returns 0 on success, nonzero on failure. Clears retain_committed on * success - every commit-restart site MUST go through this helper so the diff --git a/tests/issue582_plink_revitalization.phpt b/tests/issue582_plink_revitalization.phpt index e13f76d2..b9bbe3bc 100644 --- a/tests/issue582_plink_revitalization.phpt +++ b/tests/issue582_plink_revitalization.phpt @@ -1,5 +1,13 @@ --TEST-- fbird_pconnect() stale plink revitalization: reattach in place after db recreate (#582) +--ENV-- +; Issue #594 fix removes the shutdown UAF these tests exist to catch; ASAN +; UAF detection stays armed. detect_leaks=0 scopes out a PRE-EXISTING, +; separate finding: drop_db orphans the fb::Transaction wrapper (40B via +; fbt_start) when it nulls live transaction handles after the attachment +; died - freeing there would touch dead interfaces (AGENTS.md let-it-leak +; class). Tracked in the orphaned-transaction-handle issue. +ASAN_OPTIONS=detect_leaks=0 --SKIPIF-- --FILE-- diff --git a/tests/issue591_stmt_survives_conn_death.phpt b/tests/issue591_stmt_survives_conn_death.phpt index b898f5a2..460a776c 100644 --- a/tests/issue591_stmt_survives_conn_death.phpt +++ b/tests/issue591_stmt_survives_conn_death.phpt @@ -1,5 +1,13 @@ --TEST-- Query result resources outlive connection death: drop_db and disconnect with live result resources (#591) +--ENV-- +; Issue #594 fix removes the shutdown UAF these tests exist to catch; ASAN +; UAF detection stays armed. detect_leaks=0 scopes out a PRE-EXISTING, +; separate finding: drop_db orphans the fb::Transaction wrapper (40B via +; fbt_start) when it nulls live transaction handles after the attachment +; died - freeing there would touch dead interfaces (AGENTS.md let-it-leak +; class). Tracked in the orphaned-transaction-handle issue. +ASAN_OPTIONS=detect_leaks=0 --SKIPIF-- --FILE-- From 03e9c8f6af8c90bd9135bdadc6cc1b40d991b92a Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 12:20:21 +0200 Subject: [PATCH 2/2] fix(#594 review): keep registry invariant when default-tx restart fails 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). --- fbird_query_exec.c | 6 ++++++ fbird_transaction.c | 6 +++++- tests/issue582_plink_revitalization.phpt | 8 ++------ tests/issue591_stmt_survives_conn_death.phpt | 8 ++------ 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/fbird_query_exec.c b/fbird_query_exec.c index f6268956..f636d107 100755 --- a/fbird_query_exec.c +++ b/fbird_query_exec.c @@ -295,6 +295,12 @@ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *a _php_fbird_trans_unreg_query(fb_query); /* #594: leaving old registry */ fb_query->trans = NULL; /* force _php_fbird_def_trans to restart */ if (SUCCESS != _php_fbird_def_trans(fb_query->link, &fb_query->trans)) { + /* #594: def_trans may have created/found a tx struct before + * failing - keep the invariant "non-NULL trans => registered" + * or a later link-close efrees it under our live backref. */ + if (fb_query->trans) { + _php_fbird_trans_reg_query(fb_query->trans, fb_query); + } return FAILURE; /* _php_fbird_def_trans already reported the error */ } _php_fbird_trans_reg_query(fb_query->trans, fb_query); /* #594 */ diff --git a/fbird_transaction.c b/fbird_transaction.c index 8f2af52f..1b38be85 100644 --- a/fbird_transaction.c +++ b/fbird_transaction.c @@ -61,7 +61,6 @@ fbird_transaction *_php_fbird_find_default_trans(fbird_db_link *link) * le_trans dtor, execute_auto temp-trans paths). Query dtors later skip * their transaction bookkeeping through the existing fb_query->trans * NULL-guards instead of reading freed memory. */ -/* Issue #594: enroll a query on its transaction's back-ref registry. */ void _php_fbird_trans_reg_query(fbird_transaction *trans, fbird_query *q) { if (!trans) { @@ -91,6 +90,11 @@ void _php_fbird_trans_unreg_query(fbird_query *q) q->trans_reg_next = NULL; } +/* Issue #594: null out every live fbird_query back-reference to this + * transaction before the struct is efree'd (link close default-tx efree, + * le_trans dtor, execute_auto temp-trans paths). Query dtors later skip + * their transaction bookkeeping through the existing fb_query->trans + * NULL-guards instead of reading freed memory. */ void _php_fbird_trans_detach_queries(fbird_transaction *trans) { fbird_query *q = trans->query_head; diff --git a/tests/issue582_plink_revitalization.phpt b/tests/issue582_plink_revitalization.phpt index b9bbe3bc..fe184cc2 100644 --- a/tests/issue582_plink_revitalization.phpt +++ b/tests/issue582_plink_revitalization.phpt @@ -1,12 +1,8 @@ --TEST-- fbird_pconnect() stale plink revitalization: reattach in place after db recreate (#582) --ENV-- -; Issue #594 fix removes the shutdown UAF these tests exist to catch; ASAN -; UAF detection stays armed. detect_leaks=0 scopes out a PRE-EXISTING, -; separate finding: drop_db orphans the fb::Transaction wrapper (40B via -; fbt_start) when it nulls live transaction handles after the attachment -; died - freeing there would touch dead interfaces (AGENTS.md let-it-leak -; class). Tracked in the orphaned-transaction-handle issue. +; jane: detect_leaks=0 scopes the pre-existing orphaned-trans leak (#597, +; let-it-leak class); ASAN UAF detection stays armed. ASAN_OPTIONS=detect_leaks=0 --SKIPIF-- diff --git a/tests/issue591_stmt_survives_conn_death.phpt b/tests/issue591_stmt_survives_conn_death.phpt index 460a776c..4421e144 100644 --- a/tests/issue591_stmt_survives_conn_death.phpt +++ b/tests/issue591_stmt_survives_conn_death.phpt @@ -1,12 +1,8 @@ --TEST-- Query result resources outlive connection death: drop_db and disconnect with live result resources (#591) --ENV-- -; Issue #594 fix removes the shutdown UAF these tests exist to catch; ASAN -; UAF detection stays armed. detect_leaks=0 scopes out a PRE-EXISTING, -; separate finding: drop_db orphans the fb::Transaction wrapper (40B via -; fbt_start) when it nulls live transaction handles after the attachment -; died - freeing there would touch dead interfaces (AGENTS.md let-it-leak -; class). Tracked in the orphaned-transaction-handle issue. +; jane: detect_leaks=0 scopes the pre-existing orphaned-trans leak (#597, +; let-it-leak class); ASAN UAF detection stays armed. ASAN_OPTIONS=detect_leaks=0 --SKIPIF--