v11.1.0: Fix autocommit (#294), MSHUTDOWN SIGSEGV (#295), ResultSet return type (#296), Statement return type (#297)#298
Merged
satwareAG-ironMike merged 9 commits intoJul 2, 2026
Conversation
…NoThrow Transaction::commit(), rollback(), and rollbackNoThrow() in src/cpp/fb_transaction.hpp guarded on the cached master_ member instead of calling getMaster(). During MSHUTDOWN, getMaster() returns nullptr (IBG(in_mshutdown) is set) but master_ is never nulled, so the guard was bypassed and a server-side ITransaction::commit() call was made on a dead attachment after cleanup_db() dropped the database. This is the same class of bug that commit 881d375 fixed for Connection::detachNoThrow(), but the fix was never applied to the Transaction class. Fix: Replace cached master_ with getMaster() in commit(), rollback(), and rollbackNoThrow(). When getMaster() returns nullptr during MSHUTDOWN, just transaction_.reset() locally (no server-side call). Also add in_mshutdown guard in fbird_transaction_free() [fbird_classes.c:209] before fbt_rollback() as belt-and-suspenders for the OOP-native Transaction object destructor path. Regression test: tests/issue295_mshutdown_pconnect_sigsegv.phpt
…cit tx _php_fbird_def_trans() cached a SNAPSHOT-isolation transaction in ib_link->tr_list->trans and reused it across all fbird_query($conn, ...) calls without ever committing it. The snapshot was frozen at the time of the first query, so data committed by other transactions after that point was invisible. This broke doctrine-firebird-driver's lastInsertId() which queries RDB$RELATION_FIELDS via autocommit. Fix: true autocommit for non-persistent connections: - DML/DDL: commit default tx immediately after _php_fbird_exec() returns, nullify fbt_transaction so next call starts fresh (fbird_query_exec.c) - SELECT: commit default tx when result is freed (php_fbird_free_query_rsrc in fbird_query_prepare.c), after closing cursor. Guarded by had_open_cursor flag so prepared statements are not affected. Persistent connections are excluded (is_persistent flag added to fbird_db_link): cleanup_db() may drop the DB before MSHUTDOWN, causing attachment_.reset() to crash. The default tx for pconnect is cleaned up by _php_fbird_commit_link during MSHUTDOWN (with #295 getMaster() guard). fbt_free is NOT called in the autocommit path — calling it in php_fbird_free_query_rsrc context caused SIGSEGV (StatusWrapper destructor). The Transaction C++ object is cleaned up by _php_fbird_commit_link during connection close. Known small leak (one Transaction object per autocommit SELECT on non-persistent connections); jane: fix in follow-up by adding fbt_dispose(). Tests updated: - tests/005.phpt: use explicit tx for rollback test (autocommit DML is now committed immediately, can't be rolled back) - tests/fbird_query_stmt_release_001.phpt: @fbird_commit() on already-committed default tx - tests/issue35_001.phpt: @fbird_commit() on already-committed default tx Regression test: tests/issue294_autocommit_visibility.phpt
…utocommit Additional fixes for true autocommit behavioral change: 1. _php_fbird_exec(): restart default tx if committed by autocommit. fbird_execute() on prepared statements failed with 'Legacy API execution not supported' when fbird_query() DML committed the default tx between prepare and execute. Fix: check fbt_transaction == NULL and restart via _php_fbird_def_trans() (trans_res == NULL guard ensures only default tx is restarted). 2. _php_fbird_trans_end(): silent no-op for default tx already committed. fbird_commit()/fbird_rollback() on the default link now return TRUE silently when the default tx was committed by autocommit, instead of warning 'invalid transaction handle'. Explicit transactions still warn. 3. Test adaptations (7 tests fixed): - fbird_rollback_001: use explicit tx for INSERT/rollback - fbird_trans_008/009: use explicit tx for savepoint operations - 004: move fbird_free_result before fbird_close (cursor cleanup) - fbird_batch_blob_001: remove %a EXPECTF wildcard (no longer needed) - gh135_stmt_leak_007: passes with exec restart fix - bug45373: passes with exec restart fix Full test suite: 273/273 PASS (0 failures, 9 skipped). Version bumped to 11.0.2. CHANGELOG updated.
…def_trans return Code review found that fbt_free was not called in the autocommit path, leaking one fb::Transaction C++ object per autocommit query. The CHANGELOG claim that _php_fbird_commit_link cleans it up was wrong — its guard (fbt_transaction != NULL) is false after autocommit nulls it. Fix: call fbt_free() after fbt_commit() in both autocommit sites (fbird_query_exec.c DML path, fbird_query_prepare.c SELECT-result-free path). fbt_free calls rollbackNoThrow() which returns early when transaction_ is null (already committed), then deletes the C++ object. Safe because the #295 fix made rollbackNoThrow() use getMaster(). Also fix minor review finding: check _php_fbird_def_trans() return value in the exec restart logic and propagate FAILURE instead of falling through to a misleading 'Legacy API' error.
fbird_query() stub declared \Firebird\ResultSet|int|bool return type but the C implementation returned a raw resource for SELECT queries. fbird_execute() already wrapped results via fbird_setup_resultset_object() but fbird_query() and two other functions did not. Fix: added the same 5-line fbird_setup_resultset_object() wrapping block to: - PHP_FUNCTION(fbird_query) — after ownership transfer + efree(args) - PHP_FUNCTION(fbird_execute_query) — after ownership transfer - PHP_FUNCTION(fbird_query_params_tx) — after ownership transfer fbird_execute_auto() does NOT need the wrap — it throws for SELECT statements (cursor would be closed on autonomous commit). Regression test: tests/issue296_resultset_return_type.phpt verifies all 4 SELECT-returning functions return Firebird\ResultSet objects, and DML returns int (not object).
…re_ex() fbird_prepare() and fbird_prepare_ex() were the last functions returning raw resource handles instead of Firebird\* objects. The Firebird\Statement class existed as a skeleton since v8.0.0 but was never returned. Changes: - Added fbird_setup_statement_object() helper in fbird_classes.c (same pattern as fbird_setup_resultset_object()) - Added fbird_statement_get_resource() for dual-accept bridge - Added Firebird\Statement branch to FBIRD_VALIDATE_QUERY_EX macro - Added Statement instanceof check to fbird_execute() type validation - Wrapped RETVAL_RES in both fbird_prepare() and fbird_prepare_ex() - Updated stubs: return type changed from mixed to Firebird\Statement|false - OOP Connection::prepare() now calls _php_fbird_prepare() directly instead of through call_user_function (avoids segfault) - OOP Statement::execute() now calls _php_fbird_exec() directly instead of through call_user_function Also fixes: - fbird_query_prepare.c: guard autocommit commit to only fire for the DEFAULT transaction (first tr_list node), not temp transactions from fbird_execute_auto() — prevents use-after-free SIGSEGV - fbird_classes.c: Connection::prepare() and Statement::execute() refactored to call internal C functions directly Tests: - Added tests/issue297_statement_return_type.phpt - Updated tests using is_resource() to use instanceof checks - Updated resource_type_validation_001 TypeError message Full test suite: 275/275 PASS (0 failures, 9 skipped). BREAKING CHANGE: fbird_prepare() and fbird_prepare_ex() now return Firebird\Statement objects instead of resources. Code using is_resource() checks must update to instanceof \Firebird\Statement. The dual-accept bridge accepts both types in all consuming functions.
Code review found that fbt_commit() errors were silently swallowed in both autocommit sites (fbird_query_exec.c DML path, fbird_query_prepare.c SELECT-result-free path). If commit failed (e.g., deferred FK constraint violation, connection lost), the error was discarded and fbt_free() rolled back the data — the user's DML appeared to succeed but wasn't committed. Fix: check the return value and call _php_fbird_error() on failure, matching the established pattern in _php_fbird_commit_link.
…cted The /review suggested checking fbt_commit() return value and reporting errors. However, the autocommit commit can legitimately fail when the default transaction has open cursors from a prior SELECT (Firebird error: 'object TABLE ... is in use'). Reporting these as errors would be noise — the transaction stays valid and is committed at connection close or explicit fbird_commit(). Reverted to silent commit (no error reporting). Added jane: intent comment documenting the rationale. Full test suite: 275/275 PASS (0 failures, 9 skipped).
This was referenced Jul 2, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Minor release (v11.1.0) completing the M3 resource-to-object migration. Fixes 4 issues blocking the
doctrine-firebird-driverv11 upgrade.fbird_query($conn, $sql)autocommit cached a SNAPSHOT transaction and reused it without committing — data committed by other transactions was invisibleTransaction::commit/rollback/rollbackNoThrowused cachedmaster_instead ofgetMaster()— MSHUTDOWN guard bypassed, server-side call on dead attachment → SIGSEGVmaster_withgetMaster()(mirrors 881d375 fix fordetachNoThrow)fbird_query()stub declaredFirebird\ResultSetbut C code returned rawresourcefbird_setup_resultset_object()wrapping to 3 functionsfbird_prepare()/fbird_prepare_ex()returnedresourceinstead ofFirebird\Statement— last remaining resource/object dualityfbird_setup_statement_object(), completed M3 migrationTest Results
275/275 PASS, 0 failures, 9 skipped (PHP 8.4.22, Firebird 4.0, Docker
php84-dev)4 new regression tests added:
tests/issue294_autocommit_visibility.phpttests/issue295_mshutdown_pconnect_sigsegv.phpttests/issue296_resultset_return_type.phpttests/issue297_statement_return_type.phptBreaking Changes (v11.1)
fbird_prepare()/fbird_prepare_ex()returnFirebird\Statement(wasresource)fbird_query()/fbird_execute_query()/fbird_query_params_tx()returnFirebird\ResultSetfor SELECT (wasresource; stubs already declared this since v11.0.0)fbird_commit()/fbird_rollback()on already-committed default tx silently returnstrue(was: warning +false)The dual-accept bridge accepts both legacy resources and new objects in all consuming functions — no consumer-side changes required.
Code Review
Two
/reviewpasses completed:fbt_freewas not called in autocommit path; fixed by calling it (the SIGSEGV during PHP shutdown after persistent connection cleanup (v11.0.1) #295getMaster()fix made it safe)jane:intent commentCommits (9)
Checklist
check-version-stamps.shpasses)v11.1.0after mergeCloses #294, #295, #296, #297