From d7a1492398318abd2cfb5931543f107f83445f50 Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 14:46:45 +0200 Subject: [PATCH 1/5] fix(#589,#597,#593): rollback_ret locks, drop_db tx orphan, ZTS-safe sweep registry #589: isc_rollback_retaining retains attachment-level relation locks exactly like isc_commit_retaining (#586) - probe: after a retaining rollback of DDL, a fresh SERIALIZABLE NOWAIT attachment blocked over metadata. Zero-open-cursor rollback_ret now hard-rolls-back + restarts with the stored TPB (_php_fbird_trans_rollback_restart), releasing the locks; cursor-bearing transactions keep true retaining semantics. #597: fbird_drop_db() frees live transaction handles BEFORE fbc_drop_database() while the attachment is still valid (rollbackNoThrow+delete via fbt_free) instead of orphaning the fb::Transaction wrappers afterwards (LSAN 40B each). issue591/issue582 run with full LSAN again (scoped detect_leaks=0 removed); only fbird_drop_db_003 keeps it for an unrelated residual init-path leak. #593: statement sweep registry moved from thread_local onto fb::Connection (stmt_head_) - whoever closes the connection sweeps its wrappers regardless of preparing thread (ZTS-safe by construction). fbs_prepare gains a connection_ptr parameter (service-API callers pass NULL - synchronous lifetimes). Service wrappers remain unregistered no-ops by design. New test: tests/issue589_rollback_ret_locks.phpt --- CHANGELOG.md | 2 + UPGRADING.md | 10 ++ fbird_connection.c | 12 +++ fbird_inspection.c | 3 + fbird_query_prepare.c | 1 + fbird_transaction.c | 56 +++++++++- firebird.c | 2 + firebird_utils.cpp | 104 ++++++++++--------- firebird_utils.h | 1 + php_fbird_includes.h | 1 + src/cpp/fb_connection.hpp | 15 ++- src/cpp/fb_statement.hpp | 9 +- tests/fbird_drop_db_003.phpt | 7 +- tests/issue582_plink_revitalization.phpt | 4 - tests/issue589_rollback_ret_locks.phpt | 67 ++++++++++++ tests/issue591_stmt_survives_conn_death.phpt | 4 - 16 files changed, 231 insertions(+), 67 deletions(-) create mode 100644 tests/issue589_rollback_ret_locks.phpt diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c0f7c25..e1a53134 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **#589: `fbird_rollback_ret()` on a transaction with **zero open cursors** is now a hard rollback + restart** (twin of the #586 commit_ret fix): isc_rollback_retaining retained attachment-level relation locks exactly like isc_commit_retaining - probe confirmed a fresh SERIALIZABLE NOWAIT attachment was blocked over metadata after a retaining rollback. With no cursors there is nothing to preserve; the hard rollback releases all locks and restarts with the stored TPB. Transactions WITH open cursors keep true retaining semantics. See `UPGRADING.md`. + - **`fbird_query(FBIRD_CREATE, ...)` return type (Layer 1)**: returns a `Firebird\Connection` object instead of a raw resource. Code using `is_resource()` on the result must switch to `instanceof Firebird\Connection` (or truthiness, which is unchanged). The FBIRD_CREATE first argument remains deprecated - use `fbird_create_database()`. - **Behavior change (Layer 1/2)**: `fbird_commit_ret()` on a transaction with **zero open cursors** is now a hard commit + restart. Besides releasing locks, a hard commit **invalidates open BLOB handles** on that transaction (`isc_commit_retaining` preserved them). The legacy chunked-blob-import pattern (read blob in a loop with periodic `commit_ret`) must keep a SELECT cursor open or use plain `fbird_commit()` boundaries. PDO (`pdo_fbird`) is unaffected - it calls retaining commit directly. See `UPGRADING.md`. diff --git a/UPGRADING.md b/UPGRADING.md index 0bb62293..1a5d5a22 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -15,6 +15,16 @@ must switch to `instanceof Firebird\Connection`. The object works everywhere the resource did (dual-accept: `fbird_query`, `fbird_close`, `fbird_drop_db`, transactions). Prefer `fbird_create_database()`, which is not deprecated. +## v13.2.x → Unreleased (#589) + +### `fbird_rollback_ret()` with no open cursors is now a hard rollback + +Same shape as the #586 commit_ret change: a retaining rollback with **zero +open cursors** performs a hard rollback + transparent restart (stored TPB), +releasing the attachment-level relation locks that isc_rollback_retaining +otherwise keeps until disconnect. Callers keeping cursors open retain true +retaining semantics. + ## v13.2.x → Unreleased (#595) ### `fbird_connect()`/`fbird_pconnect()` gain a documented `sync` parameter diff --git a/fbird_connection.c b/fbird_connection.c index 0e4c01b0..96f9169a 100644 --- a/fbird_connection.c +++ b/fbird_connection.c @@ -986,6 +986,18 @@ PHP_FUNCTION(fbird_drop_db) * disconnect(). (PR #590 review finding 3) */ void *dead_conn = fb_link->fbc_connection; fb_link->fbc_connection = NULL; + /* Issue #597: release live transaction handles BEFORE the drop - + * the attachment is still valid here, so rollbackNoThrow+delete + * fully frees every fb::Transaction wrapper. The old post-drop + * nulling orphaned them (LSAN 40B each, #597): freeing AFTER + * attachment death would touch dead interfaces. DROP DATABASE + * discards open work server-side regardless of drop outcome. */ + for (l = fb_link->tr_list; l != NULL; l = l->next) { + if (l->trans != NULL && l->trans->fbt_transaction != NULL) { + fbt_free(l->trans->fbt_transaction); + l->trans->fbt_transaction = NULL; + } + } drop_result = fbc_drop_database(dead_conn, status); if (drop_result != 0) { _php_fbird_error(status); diff --git a/fbird_inspection.c b/fbird_inspection.c index c41cb830..73724ba9 100755 --- a/fbird_inspection.c +++ b/fbird_inspection.c @@ -65,6 +65,7 @@ static int _fbird_exec_kill(fbird_db_link *link, fbird_transaction *trans, ISC_I FBG(master_instance), attachment, transaction, + NULL, /* #593: service attachment - no Connection registry */ sql, 0, /* null-terminated */ SQL_DIALECT_V6, @@ -166,6 +167,7 @@ static int _fbird_drop_table(fbird_db_link *link, fbird_transaction *trans, cons FBG(master_instance), attachment, transaction, + NULL, /* #593: service attachment - no Connection registry */ drop_sql, 0, /* null-terminated */ SQL_DIALECT_V6, @@ -335,6 +337,7 @@ PHP_FUNCTION(fbird_list_table_blockers) FBG(master_instance), attachment, transaction, + NULL, /* #593: service attachment - no Connection registry */ sql, 0, /* null-terminated */ SQL_DIALECT_V6, diff --git a/fbird_query_prepare.c b/fbird_query_prepare.c index 0e4278fc..9234d505 100755 --- a/fbird_query_prepare.c +++ b/fbird_query_prepare.c @@ -323,6 +323,7 @@ int _php_fbird_prepare(fbird_query **new_query, fbird_db_link *link, FBG(master_instance), attachment_ptr, transaction_ptr, + fb_query->link->fbc_connection, /* #593: owning connection for sweep */ query, 0, /* sql_length: 0 = null-terminated */ link->dialect, diff --git a/fbird_transaction.c b/fbird_transaction.c index fd0c6130..6ed978c8 100644 --- a/fbird_transaction.c +++ b/fbird_transaction.c @@ -558,6 +558,7 @@ static void _php_fbird_exec_savepoint(INTERNAL_FUNCTION_PARAMETERS, const char * /* Prepare the savepoint statement */ stmt = fbs_prepare(FBG(master_instance), attachment, transaction_ptr, + link->fbc_connection, /* #593: owning connection for sweep */ query, (unsigned)len, SQL_DIALECT_CURRENT, status); if (!stmt) { _php_fbird_error(status); @@ -843,6 +844,45 @@ PHP_FUNCTION(fbird_connection_info) * single-link transaction (link_cnt == 1). * Returns 0 on success, nonzero on failure (status zero-initialized then * populated on engine errors; isc_arg_end-terminated on local failures). */ +/* Issue #589: hard ROLLBACK + restart with the stored TPB - the rollback + * twin of _php_fbird_trans_commit_restart(). Releases retained relation + * locks (isc_rollback_retaining keeps them, same #586 class) while leaving + * the caller a live, empty transaction. */ +int _php_fbird_trans_rollback_restart(fbird_transaction *trans, ISC_STATUS_ARRAY status) +{ + fbird_db_link *fb_link = trans->db_link[0]; + + status[0] = (ISC_STATUS) isc_arg_end; + status[1] = 0; + + if (!fb_link || !fb_link->fbc_connection) { + return 1; + } + + /* Hard rollback: releases all locks including metadata */ + if (fbt_rollback(trans->fbt_transaction, status) != 0) { + return 1; + } + + fbt_free(trans->fbt_transaction); + trans->fbt_transaction = NULL; + + void *attachment = fbc_get_attachment(fb_link->fbc_connection); + if (attachment == NULL) { + return 1; + } + + trans->fbt_transaction = fbt_start( + FBG(master_instance), + attachment, + trans->stored_tpb_len, + trans->stored_tpb_len > 0 ? trans->stored_tpb : NULL, + status + ); + + return (trans->fbt_transaction == NULL) ? 1 : 0; +} + int _php_fbird_trans_commit_restart(fbird_transaction *trans, ISC_STATUS_ARRAY status) { fbird_db_link *fb_link = trans->db_link[0]; @@ -1402,7 +1442,21 @@ static void _php_fbird_trans_end(INTERNAL_FUNCTION_PARAMETERS, int commit) result = fbt_commit(trans->fbt_transaction, status); break; case (ROLLBACK | RETAIN): - result = fbt_rollback_retaining(trans->fbt_transaction, status); + /* Issue #589: isc_rollback_retaining retains attachment-level + * relation locks exactly like isc_commit_retaining (#586 probe: + * SERIALIZABLE NOWAIT over metadata blocked after a retaining + * rollback). With no open cursor there is nothing to preserve - + * hard rollback + restart releases the locks and is observably + * identical for the caller. Single-link + non-MSHUTDOWN only. */ + if (trans->open_cursor_count == 0 && + trans->link_cnt == 1 && + !FBG(in_mshutdown)) { + FBDEBUG("Issue #589: rollback_ret with no open cursors -> hard rollback + restart"); + result = (_php_fbird_trans_rollback_restart(trans, status) != 0) + ? -1 : 0; + } else { + result = fbt_rollback_retaining(trans->fbt_transaction, status); + } break; case (COMMIT | RETAIN): /* Issue #586: isc_commit_retaining retains all relation locks diff --git a/firebird.c b/firebird.c index eff49c93..58a4d8a3 100755 --- a/firebird.c +++ b/firebird.c @@ -1284,6 +1284,7 @@ PHP_FUNCTION(fbird_gen_id) /* Prepare the query via OO API */ stmt = fbs_prepare(FBG(master_instance), attachment, transaction_ptr, + NULL, /* #593 */ query, (unsigned)strlen(query), SQL_DIALECT_CURRENT, status); if (!stmt) { _php_fbird_error(status); @@ -1383,6 +1384,7 @@ PHP_FUNCTION(fbird_last_insert_id) } void *stmt = fbs_prepare(FBG(master_instance), attachment, transaction_ptr, + NULL, /* #593 */ query, (unsigned)strlen(query), SQL_DIALECT_CURRENT, status); if (!stmt) { _php_fbird_error(status); diff --git a/firebird_utils.cpp b/firebird_utils.cpp index 96d78f31..4b0d0b61 100755 --- a/firebird_utils.cpp +++ b/firebird_utils.cpp @@ -280,7 +280,7 @@ using fb::copy_status_to_sv; * section, after fb_statement.hpp include; fwd-declared for the connection * death sites below). */ namespace fb { class StatementWrapper; } -static void fb_statement_sweep_invalidate(void* attachment); +static void fb_statement_sweep_invalidate(fb::Connection* conn); namespace fb { @@ -606,14 +606,14 @@ extern "C" int fbc_disconnect(void* connection, ISC_STATUS* status_vector) { return 0; } - /* Issue #591: capture attachment identity BEFORE detach (Connection::get() - * may return null afterwards) and neutralize dependent statements - the - * RAII attachment release below invalidates their interfaces. */ - void* dead_attachment = reinterpret_cast(connection)->get(); + auto* conn = reinterpret_cast(connection); - try { - auto* conn = reinterpret_cast(connection); + /* Issue #591/#593: neutralize dependent statements BEFORE the RAII + * attachment release invalidates their interfaces. Per-connection + * registry: sweeps everything registered on it, any thread. */ + fb_statement_sweep_invalidate(conn); + try { // Try graceful disconnect bool success = conn->detachNoThrow(); @@ -622,14 +622,12 @@ extern "C" int fbc_disconnect(void* connection, ISC_STATUS* status_vector) { conn->copyLastStatus(status_vector, ISC_STATUS_LENGTH); } - fb_statement_sweep_invalidate(dead_attachment); delete conn; return success ? 0 : 1; } catch (...) { // Clean up even on exception - fb_statement_sweep_invalidate(dead_attachment); - delete reinterpret_cast(connection); + delete conn; return 1; } } @@ -639,32 +637,28 @@ extern "C" int fbc_drop_database(void* connection, ISC_STATUS* status_vector) { return -1; } - /* Issue #591: attachment identity for the statement sweep (all exit - * paths delete the Connection; dropDatabase() destroys the attachment - * server-side even when it reports an error afterwards). */ - void* dead_attachment = reinterpret_cast(connection)->get(); + auto* conn = reinterpret_cast(connection); - try { - auto* conn = reinterpret_cast(connection); + /* Issue #591/#593: all exit paths delete the Connection; dropDatabase() + * destroys the attachment server-side even when it reports an error + * afterwards. Sweep this connection's statements first. */ + fb_statement_sweep_invalidate(conn); + try { // dropDatabase() throws fb::Exception on failure conn->dropDatabase(); - fb_statement_sweep_invalidate(dead_attachment); delete conn; return 0; } catch (const fb::Exception&) { - auto* conn = reinterpret_cast(connection); if (status_vector) { conn->copyLastStatus(status_vector, ISC_STATUS_LENGTH); } - fb_statement_sweep_invalidate(dead_attachment); delete conn; return -1; } catch (...) { - fb_statement_sweep_invalidate(dead_attachment); - delete reinterpret_cast(connection); + delete conn; return -1; } } @@ -1372,48 +1366,55 @@ extern "C" int fbu_encode_timestamp_tz(void *master_ptr, ISC_TIMESTAMP_TZ* times * jane: BlobWrapper/batch wrappers have the same lifetime coupling but no * observed crash - extend the same pattern if one appears. * ======================================================================== */ -static thread_local fb::StatementWrapper* t_stmt_sweep_head = nullptr; +/* Issue #593: the sweep registry lives on each fb::Connection + * (Connection::stmt_head_), not in thread_local state - whoever closes the + * connection sweeps every wrapper registered against it, on any thread. + * Service-API wrappers (no Connection) are unregistered no-ops: their + * lifetime is synchronous within one call. */ -/* Remove a wrapper from the sweep registry (fbs_free only delete site). */ +/* Remove a wrapper from its connection's registry (fbs_free delete site). */ static void fb_statement_sweep_unregister(fb::StatementWrapper* wrapper) { - fb::StatementWrapper** curr = &t_stmt_sweep_head; + auto* conn = static_cast(wrapper->sweep_conn_); + if (!conn) { + wrapper->sweep_next_ = nullptr; + return; + } + fb::StatementWrapper** curr = &conn->stmt_head_; while (*curr) { if (*curr == wrapper) { *curr = wrapper->sweep_next_; wrapper->sweep_next_ = nullptr; + wrapper->sweep_conn_ = nullptr; return; } curr = &(*curr)->sweep_next_; } } -/* Neutralize every wrapper owned by a dying attachment (identity compare on - * the raw pointer value - the attachment itself may already be released). - * jane: O(n) single-pass unlink; statement counts per request are small - * (tens), no list compaction needed. */ -static void fb_statement_sweep_invalidate(void* attachment) { - if (attachment == nullptr) { +/* Neutralize every wrapper registered on this dying connection. + * jane: O(n) single-pass; statement counts per request are small (tens). */ +static void fb_statement_sweep_invalidate(fb::Connection* conn) { + if (!conn) { return; } - fb::StatementWrapper** link = &t_stmt_sweep_head; - while (*link) { - fb::StatementWrapper* w = *link; - if (w->sweep_owner_ == attachment) { - w->invalidate(); - /* Unlink: wrapper stays alive (freed later via fbs_free), but no - * point sweeping it again. */ - *link = w->sweep_next_; - w->sweep_next_ = nullptr; - } else { - link = &w->sweep_next_; - } + fb::StatementWrapper* w = conn->stmt_head_; + while (w) { + fb::StatementWrapper* next = w->sweep_next_; + w->invalidate(); + /* Unlink: wrapper stays alive (freed later via fbs_free), but no + * point sweeping it again. */ + w->sweep_conn_ = nullptr; + w->sweep_next_ = nullptr; + w = next; } + conn->stmt_head_ = nullptr; } extern "C" void* fbs_prepare( void* master_ptr, void* attachment_ptr, void* transaction_ptr, + void* connection_ptr, const char* sql, unsigned sql_length, unsigned dialect, @@ -1452,14 +1453,17 @@ extern "C" void* fbs_prepare( return nullptr; } - /* Issue #591: register with the connection-death sweep registry keyed by - * attachment identity, so fbc_disconnect()/fbc_drop_database() can - * invalidate this wrapper when the attachment dies. Per-thread: PHP - * resources are created and destroyed on the same request thread (ZTS - * workers get one list each). */ - wrapper->sweep_owner_ = attachment; - wrapper->sweep_next_ = t_stmt_sweep_head; - t_stmt_sweep_head = wrapper; + /* Issue #591/#593: register with the owning connection's sweep list so + * fbc_disconnect()/fbc_drop_database() invalidate this wrapper when the + * attachment dies - from ANY thread (ZTS). Service-API callers pass + * nullptr and free synchronously. */ + wrapper->sweep_conn_ = connection_ptr; + if (auto* conn = static_cast(connection_ptr)) { + wrapper->sweep_next_ = conn->stmt_head_; + conn->stmt_head_ = wrapper; + } else { + wrapper->sweep_next_ = nullptr; + } return wrapper; } diff --git a/firebird_utils.h b/firebird_utils.h index 6e4f25a5..8ed23e1b 100755 --- a/firebird_utils.h +++ b/firebird_utils.h @@ -393,6 +393,7 @@ void* fbs_prepare( void* master_ptr, void* attachment_ptr, void* transaction_ptr, + void* connection_ptr, const char* sql, unsigned sql_length, unsigned dialect, diff --git a/php_fbird_includes.h b/php_fbird_includes.h index 0f8e447c..8b9e3fdb 100755 --- a/php_fbird_includes.h +++ b/php_fbird_includes.h @@ -545,6 +545,7 @@ void _php_fbird_trans_unreg_batch(struct _fb_batch *b); * Shared by fbird_release_metadata_locks(), _php_fbird_trans_end() and * Firebird\Transaction::releaseMetadataLocks(). */ int _php_fbird_trans_commit_restart(fbird_transaction *trans, ISC_STATUS_ARRAY status); +int _php_fbird_trans_rollback_restart(fbird_transaction *trans, ISC_STATUS_ARRAY status); static inline void _php_fbird_cursor_opened(fbird_query *fb_query) { /* Issue #586: idempotent - the SELECT execute path historically calls * this twice on the child result (open + "inherited state" re-mark); diff --git a/src/cpp/fb_connection.hpp b/src/cpp/fb_connection.hpp index d0289d7b..8eac4b33 100755 --- a/src/cpp/fb_connection.hpp +++ b/src/cpp/fb_connection.hpp @@ -95,8 +95,17 @@ struct ConnectionParams { * } * @endcode */ +/* Issue #593: per-connection statement registry lives on Connection + * (stmt_head_ below) instead of thread_local state - ZTS-safe because the + * list lives on the shared Connection object, so whoever closes it sweeps + * everything registered against it regardless of preparing thread. + * Consumed only by firebird_utils.cpp's C bridge. */ +class StatementWrapper; + class Connection { public: + StatementWrapper* stmt_head_ = nullptr; + /** * Factory method with explicit IMaster (for testing/advanced use). * @@ -421,8 +430,10 @@ inline Connection::Connection(Connection&& other) noexcept version_(other.version_), last_status_(std::move(other.last_status_)), statement_timeout_ms_(other.statement_timeout_ms_), - idle_timeout_sec_(other.idle_timeout_sec_) { + idle_timeout_sec_(other.idle_timeout_sec_), + stmt_head_(other.stmt_head_) { other.master_ = nullptr; + other.stmt_head_ = nullptr; } inline Connection& Connection::operator=(Connection&& other) noexcept { @@ -434,7 +445,9 @@ inline Connection& Connection::operator=(Connection&& other) noexcept { last_status_ = std::move(other.last_status_); statement_timeout_ms_ = other.statement_timeout_ms_; idle_timeout_sec_ = other.idle_timeout_sec_; + stmt_head_ = other.stmt_head_; other.master_ = nullptr; + other.stmt_head_ = nullptr; } return *this; } diff --git a/src/cpp/fb_statement.hpp b/src/cpp/fb_statement.hpp index c350c72f..f50598e8 100755 --- a/src/cpp/fb_statement.hpp +++ b/src/cpp/fb_statement.hpp @@ -668,10 +668,11 @@ class StatementWrapper { prepared_ = false; } - /// Owning IAttachment identity (raw value, NOT refcounted) - set at - /// fbs_prepare() time; used by the connection-death sweep registry. - /// jane: identity key only; never dereferenced (attachment may be dead). - void* sweep_owner_{nullptr}; + /// Owning fb::Connection (raw void* to avoid header circularity; cast + /// at use in firebird_utils.cpp). Set at fbs_prepare() when the caller + /// has a Connection; service-API prepares pass nullptr and manage their + /// own synchronous lifetime. jane: never dereferenced here. + void* sweep_conn_{nullptr}; /// Intrusive per-thread registry link (firebird_utils.cpp owns the list). StatementWrapper* sweep_next_{nullptr}; diff --git a/tests/fbird_drop_db_003.phpt b/tests/fbird_drop_db_003.phpt index 6d3fe24c..c21d14ac 100755 --- a/tests/fbird_drop_db_003.phpt +++ b/tests/fbird_drop_db_003.phpt @@ -1,9 +1,10 @@ --TEST-- fbird_drop_db(): Make sure passing an integer to the function throws an error. --ENV-- -; jane: detect_leaks=0 - LSAN's exit check fatally conflicts with -; run-tests --set-timeout ptrace on leak-bearing paths (#596); ASAN UAF -; detection stays armed. +; jane: detect_leaks=0 - this test's init path still has a residual +; non-trans orphan (unrelated to the #597 drop_db tx-handle fix, which +; is verified by issue591/issue582 running with LSAN armed); tracked +; under #596/#597 investigation. ASAN_OPTIONS=detect_leaks=0 --SKIPIF-- --FILE-- diff --git a/tests/issue589_rollback_ret_locks.phpt b/tests/issue589_rollback_ret_locks.phpt new file mode 100644 index 00000000..66aeaa37 --- /dev/null +++ b/tests/issue589_rollback_ret_locks.phpt @@ -0,0 +1,67 @@ +--TEST-- +Issue #589: rollback_ret() must not retain metadata relation locks (#586 twin) +--SKIPIF-- + +--FILE-- + FBIRD_WRITE, + 'isolation' => FBIRD_COMMITTED | FBIRD_REC_VERSION, + 'lock_resolution' => FBIRD_WAIT, +]); + +$qA = fbird_prepare_ex($cA, 'CREATE TABLE T589_RR (id INTEGER)', $tA); +var_dump(fbird_execute($qA) === true); +fbird_free_query($qA); + +// Retaining rollback undoes the transactional DDL... +var_dump(fbird_rollback_ret($tA)); + +$chk = fbird_query($cA, "SELECT COUNT(*) FROM RDB\$RELATIONS WHERE RDB\$RELATION_NAME = 'T589_RR'"); +$row = $chk ? fbird_fetch_row($chk) : false; +echo "table-gone: "; +var_dump($row ? ((int)$row[0]) === 0 : false); +if ($chk) fbird_free_result($chk); + +// ...but must not keep the relation locks (the #589 defect). +$cB = fbird_connect($test_base, '', '', '', 0, 3, '', 0, FBIRD_CONNECT_FORCE_NEW); +$tB = fbird_trans_start($cB, [ + 'access_mode' => FBIRD_WRITE, + 'isolation' => FBIRD_CONSISTENCY, + 'lock_resolution' => FBIRD_NOWAIT, +]); +$ok = false; +if ($tB) { + $qB = @fbird_prepare_ex($cB, "SELECT COUNT(*) FROM RDB\$RELATION_FIELDS", $tB); + if ($qB && fbird_execute($qB)) { + $rB = fbird_fetch_row($qB); + $ok = ($rB !== false); + if ($qB) fbird_free_query($qB); + } +} +echo "serializable-nowait-after-rollback_retain: "; +var_dump($ok); + +fbird_close($cA); fbird_close($cB); +echo "done\n"; +?> +--EXPECT-- +bool(true) +bool(true) +bool(true) +table-gone: bool(true) +serializable-nowait-after-rollback_retain: bool(true) +done diff --git a/tests/issue591_stmt_survives_conn_death.phpt b/tests/issue591_stmt_survives_conn_death.phpt index 4421e144..b898f5a2 100644 --- a/tests/issue591_stmt_survives_conn_death.phpt +++ b/tests/issue591_stmt_survives_conn_death.phpt @@ -1,9 +1,5 @@ --TEST-- Query result resources outlive connection death: drop_db and disconnect with live result resources (#591) ---ENV-- -; 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-- --FILE-- From 54e43a71527cdb389b55cc446b5b4347840f25a1 Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 15:14:31 +0200 Subject: [PATCH 2/5] fix(pdo): sync fbs_prepare signature into pdo_fbird header + enroll statements PR #601 session-4 changed fbs_prepare's signature (connection_ptr for the #593 per-connection sweep registry) but pdo_fbird carries its OWN copies of firebird_utils.h/php_fbird_includes.h. PDO compiled against the stale 8-param declaration while the main ext exported 9 params -> garbage args (sql=0x1f) -> SIGSEGV in pdo_fbird_handle_preparer on every pdo test, across all CI matrix cells. Fix: sync the header and pass H->fbc_conn at all three call sites - which also enrolls PDO statements in the #593 sweep registry (bonus: PDO prepared statements now get connection-death invalidation too). Found via full-CI-matrix failure; reproduced locally on php83-fb3-dev; FB3 full suite 358/0 after fix. --- pdo_fbird/firebird_utils.h | 1 + pdo_fbird/pdo_fbird_driver.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pdo_fbird/firebird_utils.h b/pdo_fbird/firebird_utils.h index 6e4f25a5..8ed23e1b 100755 --- a/pdo_fbird/firebird_utils.h +++ b/pdo_fbird/firebird_utils.h @@ -393,6 +393,7 @@ void* fbs_prepare( void* master_ptr, void* attachment_ptr, void* transaction_ptr, + void* connection_ptr, const char* sql, unsigned sql_length, unsigned dialect, diff --git a/pdo_fbird/pdo_fbird_driver.c b/pdo_fbird/pdo_fbird_driver.c index 2b08282b..6c7e3bce 100644 --- a/pdo_fbird/pdo_fbird_driver.c +++ b/pdo_fbird/pdo_fbird_driver.c @@ -170,7 +170,7 @@ static bool pdo_fbird_handle_preparer(pdo_dbh_t *dbh, zend_string *sql, void *tr = fbt_get_handle(H->fbt_trans); S->fbs_stmt = fbs_prepare( - FBG(master_instance), att, tr, + FBG(master_instance), att, tr, H->fbc_conn, prepare_sql, prepare_len, H->dialect, S->status ); @@ -310,7 +310,7 @@ static zend_long pdo_fbird_handle_doer(pdo_dbh_t *dbh, const zend_string *sql) had_statements = 1; ISC_STATUS_ARRAY st = {0}; - void *fbs = fbs_prepare(FBG(master_instance), att, tr, + void *fbs = fbs_prepare(FBG(master_instance), att, tr, H->fbc_conn, p, (unsigned)len, H->dialect, st); if (!fbs) { memcpy(H->status, st, sizeof(ISC_STATUS_ARRAY)); @@ -1190,7 +1190,7 @@ static zend_string *pdo_fbird_handle_last_id(pdo_dbh_t *dbh, const zend_string * return NULL; } - void *stmt = fbs_prepare(FBG(master_instance), attachment, tr_handle, + void *stmt = fbs_prepare(FBG(master_instance), attachment, tr_handle, H->fbc_conn, query, (unsigned)strlen(query), H->dialect, H->status); if (!stmt) { return NULL; From e6164c0e8946cdd00b3b55ad3c46d5773cc0bc4e Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 15:22:23 +0200 Subject: [PATCH 3/5] chore(test): --CLEAN-- section for #589 test (quality gate) --- tests/issue589_rollback_ret_locks.phpt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/issue589_rollback_ret_locks.phpt b/tests/issue589_rollback_ret_locks.phpt index 66aeaa37..fbd1878e 100644 --- a/tests/issue589_rollback_ret_locks.phpt +++ b/tests/issue589_rollback_ret_locks.phpt @@ -58,6 +58,8 @@ var_dump($ok); fbird_close($cA); fbird_close($cB); echo "done\n"; ?> +--CLEAN-- + --EXPECT-- bool(true) bool(true) From 73c9f50deae6d0a59c7456c128a4d88763058d6b Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 16:07:27 +0200 Subject: [PATCH 4/5] fix(review): PR #601 review findings + #602 header-parity gate Review findings applied: - UPGRADING #589: BLOB-handle invalidation warning (same hazard as #586; rollback_ret blob-loops must keep a cursor open) - fbird_inspection.c x3: comment rationale corrected - NULL is safe because the statement is freed synchronously in-function, NOT because it is a 'service attachment' (these use the user's attachment; the wrong rationale invited unsafe copies of the pattern) - fbird_transaction.c: stale comment claiming fbt_* 'ALWAYS delete the wrapper' contradicted by implementations and load-bearing-adjacent for rollback_restart's failure path #602 acceptance criteria (review finding 3 - landed here per reviewer): - scripts/check-header-parity.sh: fails when firebird_utils.h (ABI- load-bearing prototypes) or php_fbird_includes.h (struct layouts) drift between main ext and pdo_fbird copies - pdo_fbird/php_fbird_includes.h reconciled to a byte-identical copy (was generations stale; safe because pdo touches only opaque handles - verified by FB3 full suite 358/0 after sync) - code-quality.yml runs the gate alongside check-stubs-sync Ponytail -30 twin-merge skipped deliberately: repo convention is documented per-issue twins (commit_restart/rollback_restart). --- .github/workflows/code-quality.yml | 5 ++ UPGRADING.md | 6 ++ fbird_inspection.c | 6 +- fbird_transaction.c | 5 +- pdo_fbird/php_fbird_includes.h | 108 +++++++++++++++++++++++++++-- scripts/check-header-parity.sh | 64 +++++++++++++++++ 6 files changed, 185 insertions(+), 9 deletions(-) create mode 100755 scripts/check-header-parity.sh diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index 016c39d1..3f0da8ad 100755 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -75,6 +75,11 @@ jobs: chmod +x scripts/check-stubs-sync.sh bash scripts/check-stubs-sync.sh + - name: Check pdo_fbird shared-header parity (#602) + run: | + chmod +x scripts/check-header-parity.sh + bash scripts/check-header-parity.sh + - name: Check --CLEAN-- sections for DDL tests run: | chmod +x scripts/check-clean-sections.sh diff --git a/UPGRADING.md b/UPGRADING.md index 1a5d5a22..231c23c1 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -25,6 +25,12 @@ releasing the attachment-level relation locks that isc_rollback_retaining otherwise keeps until disconnect. Callers keeping cursors open retain true retaining semantics. +**BLOB handles** (same hazard as #586): a hard rollback invalidates open +BLOB handles on that transaction; `isc_rollback_retaining` preserved them. +The legacy blob read-loop with periodic `fbird_rollback_ret()` must keep a +SELECT cursor open on the same transaction or restructure around plain +`fbird_rollback()` boundaries. + ## v13.2.x → Unreleased (#595) ### `fbird_connect()`/`fbird_pconnect()` gain a documented `sync` parameter diff --git a/fbird_inspection.c b/fbird_inspection.c index 73724ba9..6b73a3d3 100755 --- a/fbird_inspection.c +++ b/fbird_inspection.c @@ -65,7 +65,7 @@ static int _fbird_exec_kill(fbird_db_link *link, fbird_transaction *trans, ISC_I FBG(master_instance), attachment, transaction, - NULL, /* #593: service attachment - no Connection registry */ + NULL, /* #593: statement freed synchronously below - no registry enrollment needed */ sql, 0, /* null-terminated */ SQL_DIALECT_V6, @@ -167,7 +167,7 @@ static int _fbird_drop_table(fbird_db_link *link, fbird_transaction *trans, cons FBG(master_instance), attachment, transaction, - NULL, /* #593: service attachment - no Connection registry */ + NULL, /* #593: statement freed synchronously below - no registry enrollment needed */ drop_sql, 0, /* null-terminated */ SQL_DIALECT_V6, @@ -337,7 +337,7 @@ PHP_FUNCTION(fbird_list_table_blockers) FBG(master_instance), attachment, transaction, - NULL, /* #593: service attachment - no Connection registry */ + NULL, /* #593: statement freed synchronously below - no registry enrollment needed */ sql, 0, /* null-terminated */ SQL_DIALECT_V6, diff --git a/fbird_transaction.c b/fbird_transaction.c index 6ed978c8..2ff7612c 100644 --- a/fbird_transaction.c +++ b/fbird_transaction.c @@ -1482,8 +1482,9 @@ static void _php_fbird_trans_end(INTERNAL_FUNCTION_PARAMETERS, int commit) } /* Clear handle for non-retained operations BEFORE checking result. - * The fbt_* functions ALWAYS delete the wrapper (even on error), - * so we must clear our pointer to avoid dangling references. + * fbt_commit()/fbt_rollback() do NOT delete the wrapper (it is + * explicitly freed below via fbt_free), so we clear our pointer and + * free here to avoid dangling references. * Fixes: #9, #10 - SIGSEGV due to use-after-free of transaction wrapper */ if ((commit & RETAIN) == 0) { diff --git a/pdo_fbird/php_fbird_includes.h b/pdo_fbird/php_fbird_includes.h index 1a1b3d31..8b9e3fdb 100755 --- a/pdo_fbird/php_fbird_includes.h +++ b/pdo_fbird/php_fbird_includes.h @@ -28,6 +28,10 @@ #define SQLDA_CURRENT_VERSION SQLDA_VERSION1 #endif +/* Maximum TPB (Transaction Parameter Buffer) size. + * Used by fbird_transaction struct for stored_tpb field (#566). */ +#define TPB_MAX_SIZE 2048 + /* Metadata identifier length (bytes). FB 4.0+ supports 63 chars (UTF8 = 4 bytes/char) */ #ifndef METADATALENGTH # if FB_API_VER >= 40 @@ -95,6 +99,9 @@ ZEND_BEGIN_MODULE_GLOBALS(fbird) pid_t init_pid; /* PID at initialization for fork-safety detection */ int exception_mode; /* Exception mode: 0=SILENT (default), 1=THROW */ bool in_mshutdown; /* Flag: true during MSHUTDOWN to prevent EG() access */ + bool auto_ddl_commit; /* Issue #566: Transparent DDL commit+restart on explicit tx. + * Default false (BC). When true, fires on ALL DDL regardless + * of open cursors (v13.1.0 behavior for opt-in users). */ ZEND_END_MODULE_GLOBALS(fbird) ZEND_EXTERN_MODULE_GLOBALS(fbird) @@ -127,6 +134,38 @@ typedef struct { unsigned long affected_rows; /* OO API transaction wrapper (fb::Transaction* from fbt_start()) */ void *fbt_transaction; + /* Issue #554: True for the default (implicit) transaction created by + * _php_fbird_def_trans(). Intended to replace the positional i==0 + * sentinel head node convention once callers migrate (#554 follow-up). + * Until then, the positional convention remains load-bearing. + * Only one transaction per connection has this flag. */ + bool is_default; + /* Issue #566: Open cursor count for gating #540 transparent commit+restart. + * Incremented when a SELECT cursor is opened on this transaction, + * decremented when the cursor is closed/freed. The #540 commit+restart + * only fires when open_cursor_count > 0, preserving transactional DDL + * semantics when no cursors are holding metadata locks. */ + unsigned short open_cursor_count; + /* Issue #566/#540: Stored TPB for transaction restart after commit. + * Populated at creation time by fbird_trans_start() / _php_fbird_def_trans(). + * Used by #540 transparent restart and fbird_release_metadata_locks() + * to restore the original isolation level, access mode, etc. */ + unsigned short stored_tpb_len; + unsigned char stored_tpb[TPB_MAX_SIZE]; + /* Issue #586: set when the last COMMIT|RETAIN actually retained state + * (open cursors existed). When the last cursor closes afterwards, + * _php_fbird_trans_release_if_idle() transparently hard-commits + + * 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; + /* Issue #599: same registry for batch resources holding this tx. */ + struct _fb_batch *batch_head; fbird_db_link *db_link[1]; /* last member */ } fbird_transaction; @@ -230,6 +269,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 */ @@ -243,14 +288,17 @@ typedef struct _fb_query { unsigned in_msg_length; /* Input message buffer size */ } fbird_query; -#if FB_API_VER >= 40 /** * Batch operation wrapper for Firebird 4.0+ IBatch interface. * Provides high-performance bulk INSERT operations. + * (Declared unguarded so fbird_transaction can reference the type; only + * batch CODE is FB 4.0+.) */ -typedef struct { +typedef struct _fb_batch { void *fbbatch_wrapper; /* OO API batch wrapper (from fbbatch_create()) */ - fbird_transaction *trans; /* Associated transaction */ + fbird_transaction *trans; /* Associated transaction (Issue #599 registry) */ + struct _fb_batch *batch_reg_next; /* #599 intrusive registry link */ + fbird_transaction *trans_reg_on; /* #599 registry we are enrolled in */ fbird_query *query; /* Parent prepared statement */ zend_resource *query_res; /* Strong reference to query resource (Issue #185). * Prevents premature destruction of the IStatement* @@ -260,7 +308,6 @@ typedef struct { void *in_msg_buffer; /* Message buffer for row data */ unsigned in_msg_length; /* Message buffer size */ } fbird_batch; -#endif /* FB_API_VER >= 40 */ enum php_fbird_option { PHP_FBIRD_DEFAULT = 0, @@ -359,6 +406,9 @@ void _php_fbird_module_error(const char *, ...) } while (0) int _php_fbird_def_trans(fbird_db_link *fb_link, fbird_transaction **trans); +/* Issue #554: Find the default transaction in a connection's tr_list. + * Returns the fbird_transaction with is_default == true, or NULL if none exists. */ +fbird_transaction *_php_fbird_find_default_trans(fbird_db_link *link); void _php_fbird_get_link_trans(INTERNAL_FUNCTION_PARAMETERS, zval *link_id, fbird_db_link **fb_link, fbird_transaction **trans); @@ -471,6 +521,56 @@ const char *_fbird_res_type_name(int type); /* Issue #297: Exported for OOP Statement::execute() to call directly */ int _php_fbird_exec(INTERNAL_FUNCTION_PARAMETERS, fbird_query *fb_query, zval *args, int bind_n); +/* Issue #566: Cursor counter helpers for gating #540 transparent commit+restart. + * These centralize is_open management to keep open_cursor_count accurate. + * _php_fbird_cursor_opened: Call when a SELECT cursor is opened on a transaction. + * _php_fbird_cursor_closed: Call when a SELECT cursor is closed/freed. */ +/* Issue #586: lazy release of retained locks (defined in fbird_transaction.c). + * 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); +void _php_fbird_trans_unreg_batch(struct _fb_batch *b); +/* 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 + * flag never goes stale (PR #588 review finding: hand-rolled copies in the + * OOP layer missed the clear and later idle-released uncommitted DML). + * Shared by fbird_release_metadata_locks(), _php_fbird_trans_end() and + * Firebird\Transaction::releaseMetadataLocks(). */ +int _php_fbird_trans_commit_restart(fbird_transaction *trans, ISC_STATUS_ARRAY status); +int _php_fbird_trans_rollback_restart(fbird_transaction *trans, ISC_STATUS_ARRAY status); +static inline void _php_fbird_cursor_opened(fbird_query *fb_query) { + /* Issue #586: idempotent - the SELECT execute path historically calls + * this twice on the child result (open + "inherited state" re-mark); + * an unconditional increment double-counted and open_cursor_count + * never reached 0 again, breaking #566 gating and the #586 lazy + * release. is_open guards exactly like _php_fbird_cursor_closed. */ + if (!fb_query->is_open) { + fb_query->is_open = 1; + if (fb_query->trans) { + fb_query->trans->open_cursor_count++; + } + } +} +static inline void _php_fbird_cursor_closed(fbird_query *fb_query) { + if (fb_query->is_open && fb_query->trans && fb_query->trans->open_cursor_count > 0) { + fb_query->trans->open_cursor_count--; + /* Issue #586: last cursor gone - if a prior commit_ret retained + * locks for this cursor, release them now (transparent restart) */ + if (fb_query->trans->open_cursor_count == 0) { + _php_fbird_trans_release_if_idle(fb_query->trans); + } + } + fb_query->is_open = 0; +} + #define FBIRD_VALIDATE_QUERY_EX(zv, argnum, var) do { \ /* M3 object path: Firebird\ResultSet accepted alongside resources */ \ if (Z_TYPE_P(zv) == IS_OBJECT && \ diff --git a/scripts/check-header-parity.sh b/scripts/check-header-parity.sh new file mode 100755 index 00000000..f6681f77 --- /dev/null +++ b/scripts/check-header-parity.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# check-header-parity.sh — Fail when pdo_fbird's copies of shared headers +# drift from the main extension's (#602). +# +# pdo_fbird compiles against its OWN copies of firebird_utils.h and +# php_fbird_includes.h but LINKS against the main extension's exported +# functions. A signature or struct change in the main ext that is not +# mirrored in pdo's copies produces garbage-argument calls at runtime +# (incident 2026-08-24: fbs_prepare 8-param call into 9-param export -> +# SIGSEGV in pdo_fbird_handle_preparer, ~50 PDO test failures across the +# whole CI matrix - PR #601, hotfix 54e43a7). +# +# Exit codes: +# 0 — all shared headers in sync +# 1 — firebird_utils.h drifted (ABI-load-bearing: function prototypes) +# 2 — php_fbird_includes.h drifted (struct layouts; advisory while pdo +# only touches opaque handles, but must not diverge further) + +set -euo pipefail +source "$(dirname "$0")/lib/logging.sh" + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" + +UTILS_MAIN="${ROOT_DIR}/firebird_utils.h" +UTILS_PDO="${ROOT_DIR}/pdo_fbird/firebird_utils.h" +INCLUDES_MAIN="${ROOT_DIR}/php_fbird_includes.h" +INCLUDES_PDO="${ROOT_DIR}/pdo_fbird/php_fbird_includes.h" + +RC=0 + +if [[ ! -f "${UTILS_PDO}" ]]; then + error "pdo_fbird/firebird_utils.h not found at ${UTILS_PDO}" + exit 1 +fi + +if ! diff -q "${UTILS_MAIN}" "${UTILS_PDO}" >/dev/null; then + error "firebird_utils.h DRIFT between main ext and pdo_fbird (ABI-load-bearing!):" + diff "${UTILS_MAIN}" "${UTILS_PDO}" | head -30 || true + error "Any signature change must touch BOTH copies in the same commit (#602)." + RC=1 +else + log_info "firebird_utils.h: main and pdo_fbird copies identical" +fi + +if ! diff -q "${INCLUDES_MAIN}" "${INCLUDES_PDO}" >/dev/null; then + # Struct-layout drift: pdo currently touches only opaque handles, so + # this is advisory - but it must never grow. Fail the gate so the drift + # gets reconciled deliberately instead of silently. + warn "php_fbird_includes.h drifted between main ext and pdo_fbird:" + diff "${INCLUDES_MAIN}" "${INCLUDES_PDO}" | head -20 || true + warn "pdo_fbird must not touch fbird_transaction/fbird_query struct fields." + if [[ "${ALLOW_INCLUDES_DRIFT:-0}" != "1" ]]; then + error "Set ALLOW_INCLUDES_DRIFT=1 only with a reviewed reason; default is fail." + [[ $RC -eq 1 ]] || RC=2 + fi +else + log_info "php_fbird_includes.h: main and pdo_fbird copies identical" +fi + +if [[ $RC -ne 0 ]]; then + error "Header parity check FAILED (exit ${RC}) - see #602." +fi +exit ${RC} From 19e3a13d06f4b1027c5e0342ea12a413b0259247 Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 24 Aug 2026 16:50:32 +0200 Subject: [PATCH 5/5] fix(review): PR #601 round-2 findings - fbird_transaction.c: the #599 batch-walk order bug (batch_head cleared before the walk reads it - #600 init-regex collateral) is documented with a jane: comment and DEFERRED to #603: activating the walk crashes fbird_batch_multitype_001 at process exit (simple case passes; the complex lifecycle needs its own root-cause). Walk left in the known-good dead state. - firebird_utils.cpp + fb_connection.hpp: unified #591/#593 banner - stale per-thread description removed; thread-model claim corrected to 'safe because PHP never manipulates one connection's statements from multiple threads concurrently' (was: ZTS-safe-by-construction, which overstated) - UPGRADING.md: lockstep-upgrade note for the firebird.so/pdo_fbird.so ABI boundary (old pdo .so + new firebird .so = PDO::prepare SIGSEGV, #602 class) Full suite php83-dev: 312/0. --- UPGRADING.md | 12 ++++++++++++ fbird_transaction.c | 6 ++++++ firebird_utils.cpp | 31 ++++++++++++++++++------------- src/cpp/fb_connection.hpp | 7 ++++--- 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 231c23c1..7f110471 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -15,6 +15,18 @@ must switch to `instanceof Firebird\Connection`. The object works everywhere the resource did (dual-accept: `fbird_query`, `fbird_close`, `fbird_drop_db`, transactions). Prefer `fbird_create_database()`, which is not deprecated. +## v13.2.x → Unreleased (PR #601) + +### firebird.so and pdo_fbird.so must be upgraded in lockstep + +The internal C ABI between the two extensions changed (`fbs_prepare` +signature, #593). An old `pdo_fbird.so` loaded against the new +`firebird.so` crashes on the first `PDO::prepare()` (garbage arguments - +same failure class as the in-repo incident documented in #602). When +upgrading across this boundary, update BOTH packages in the same +deployment; the split packages (`pdo-fbird` PIE, `php-firebird`) share the +major version for this reason. + ## v13.2.x → Unreleased (#589) ### `fbird_rollback_ret()` with no open cursors is now a hard rollback diff --git a/fbird_transaction.c b/fbird_transaction.c index 2ff7612c..fce676c0 100644 --- a/fbird_transaction.c +++ b/fbird_transaction.c @@ -120,6 +120,12 @@ void _php_fbird_trans_detach_queries(fbird_transaction *trans) q = next; } trans->query_head = NULL; + /* jane: batch walk below is DISABLED - see follow-up issue: the + * batch_head clear must happen AFTER the walk (order bug from #600), + * but activating the walk crashes fbird_batch_multitype_001 at + * process exit (complex batch lifecycle). Simple case passes + * (issue599 test); root-causing the exit crash is tracked in the + * #599 follow-up issue. */ trans->batch_head = NULL; /* Issue #599 */ /* Issue #599: batches hold the same raw backref (dereferenced by diff --git a/firebird_utils.cpp b/firebird_utils.cpp index 4b0d0b61..c1d3f8cf 100755 --- a/firebird_utils.cpp +++ b/firebird_utils.cpp @@ -1352,25 +1352,30 @@ extern "C" int fbu_encode_timestamp_tz(void *master_ptr, ISC_TIMESTAMP_TZ* times #include "src/cpp/fb_statement.hpp" /* ======================================================================== - * Issue #591: connection-death statement sweep registry. + * Issue #591/#593: connection-death statement sweep registry. * * StatementWrapper objects outlive their fb::Connection in PHP semantics * (query result resources are userland-owned). When the connection dies * (drop_db / disconnect), the RAII attachment releases every dependent - * interface; calling through them later is a UAF. This per-thread intrusive - * list lets the connection-death sites neutralize dependent wrappers so - * their null-guarded closeCursor()/free() become no-ops. + * interface; calling through them later is a UAF. The sweep registry lives + * on each fb::Connection (Connection::stmt_head_, #593 - it was a + * thread_local list until #593): the connection-death sites neutralize + * every wrapper registered against that connection, so the null-guarded + * closeCursor()/free() become no-ops. + * + * Thread model: the list is NOT synchronized - it is safe because PHP + * never manipulates a connection's statements from multiple threads + * concurrently (ZTS workers own disjoint resource sets; a concurrent + * close-vs-prepare on one connection would already race on the wrappers + * themselves). Cross-thread SEQUENTIAL use (plink handed between workers) + * works because the list lives on the shared Connection object, which is + * what #593 fixed. * - * jane: thread_local - resources are created and destroyed on the same - * request thread; cross-thread statement use is not a PHP pattern. - * jane: BlobWrapper/batch wrappers have the same lifetime coupling but no - * observed crash - extend the same pattern if one appears. - * ======================================================================== */ -/* Issue #593: the sweep registry lives on each fb::Connection - * (Connection::stmt_head_), not in thread_local state - whoever closes the - * connection sweeps every wrapper registered against it, on any thread. * Service-API wrappers (no Connection) are unregistered no-ops: their - * lifetime is synchronous within one call. */ + * lifetime is synchronous within one call. jane: BlobWrapper/batch + * wrappers share the lifetime coupling - batches got their own registry + * in #599; extend to blobs if a crash appears. + * ======================================================================== */ /* Remove a wrapper from its connection's registry (fbs_free delete site). */ static void fb_statement_sweep_unregister(fb::StatementWrapper* wrapper) { diff --git a/src/cpp/fb_connection.hpp b/src/cpp/fb_connection.hpp index 8eac4b33..4e4f2a5f 100755 --- a/src/cpp/fb_connection.hpp +++ b/src/cpp/fb_connection.hpp @@ -96,9 +96,10 @@ struct ConnectionParams { * @endcode */ /* Issue #593: per-connection statement registry lives on Connection - * (stmt_head_ below) instead of thread_local state - ZTS-safe because the - * list lives on the shared Connection object, so whoever closes it sweeps - * everything registered against it regardless of preparing thread. + * (stmt_head_ below) instead of thread_local state. Unsynchronized by + * design: safe because PHP never manipulates one connection's statements + * from multiple threads concurrently; sequential cross-thread handoff + * (ZTS plink reuse) works because the list lives on the shared Connection. * Consumed only by firebird_utils.cpp's C bridge. */ class StatementWrapper;