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/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..7f110471 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -15,6 +15,34 @@ 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 + +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. + +**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_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..6b73a3d3 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: statement freed synchronously below - no registry enrollment needed */ 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: statement freed synchronously below - no registry enrollment needed */ 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: statement freed synchronously below - no registry enrollment needed */ 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..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 @@ -558,6 +564,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 +850,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 +1448,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 @@ -1428,8 +1488,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/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..c1d3f8cf 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; } } @@ -1358,62 +1352,74 @@ 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. * - * 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. + * 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. + * + * Service-API wrappers (no Connection) are unregistered no-ops: their + * 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. * ======================================================================== */ -static thread_local fb::StatementWrapper* t_stmt_sweep_head = nullptr; -/* 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 +1458,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/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; 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/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/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} diff --git a/src/cpp/fb_connection.hpp b/src/cpp/fb_connection.hpp index d0289d7b..4e4f2a5f 100755 --- a/src/cpp/fb_connection.hpp +++ b/src/cpp/fb_connection.hpp @@ -95,8 +95,18 @@ struct ConnectionParams { * } * @endcode */ +/* Issue #593: per-connection statement registry lives on Connection + * (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; + class Connection { public: + StatementWrapper* stmt_head_ = nullptr; + /** * Factory method with explicit IMaster (for testing/advanced use). * @@ -421,8 +431,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 +446,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..fbd1878e --- /dev/null +++ b/tests/issue589_rollback_ret_locks.phpt @@ -0,0 +1,69 @@ +--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"; +?> +--CLEAN-- + +--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--