#200: fix double-release and use-after-free of pooled PDO connections#16
Merged
Conversation
The scheduler promotes the running main flow into the main coroutine on the fly, so a connection acquired before that was keyed to a context that no longer matched afterwards. The statement destructor then saw it as orphaned and returned it to the pool while the coroutine binding still owned it and returned it a second time, and the pool destroyed the same connection twice. Two layers: - The main flow now maps to binding key 0 whether or not the scheduler has launched, so one execution context keeps one binding across the promotion. - A pooled slot carries an owner_binding back-pointer. Every release path detaches the owner first, which makes a double release impossible even if the key drifts (statement freed in another coroutine, binding already gone). A statement no longer reaches the pool through stmt->dbh at all: the slot holds its own reference on the pool event, so releasing works in any destruction order, including GC freeing the PDO before the statement. pdo_mysql's statement destructor had the same stale-dbh read and is guarded the same way as its neighbouring stmt-cache code. Tests: pool_006..011 cover the scheduler-launch transition, statements outliving their PDO and their coroutine, GC ordering, orphaned transactions and pre-launch bindings; pdo_pool_021 covers slot accounting on failed query(). The pool tests in ext/pdo/tests now resolve their helper through REDIR_TEST_DIR, so they run under the driver redirect instead of borking. PDO_DRIVER_API is bumped for the new pdo_dbh_t field.
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
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.
Fixes true-async/php-async#200.
The bug
async_scheduler_launch()promotes the already-running main flow into the main coroutine on the fly, the first time something needs the scheduler (in the report:file_put_contents). The PDO pool keys its per-context binding by coroutine identity, soZEND_ASYNC_CURRENT_COROUTINEgoingNULL -> mainmid-execution changed the key underneath a live connection.Traced with an instrumented build:
The statement destructor missed the binding, took the "orphaned connection" branch and returned the conn to the pool; the binding still owned it and returned it again at pool destroy. The idle list then held it twice and the pool destructor freed it twice — the
conn->methodsguard inpdo_pool_free_connpassed because the memory was non-NULL, not because it was valid.The fix
Key stability. The main flow maps to binding key 0 whether or not the scheduler has launched, so one execution context keeps one binding across the promotion. Object handle 0 is reserved by the objects store, so no coroutine collides with it.
Ownership. A pooled slot carries an
owner_bindingback-pointer. Every release path detaches the owner first, so a double release is impossible even when the key legitimately drifts — a statement destroyed in a different coroutine, or a binding freed at coroutine finalize while statements still borrow the connection.Destruction order. A statement no longer dereferences
stmt->dbhon the release path at all. The slot holds its own reference on the pool event and is released throughconn->pool, so it works in any order, including GC freeing the PDO before the statement.pdo_mysql_stmt_dtorhad the same stale-dbhread (pefree(..., stmt->dbh->is_persistent)) and is now guarded exactly like the stmt-cache code eight lines below it.PDO_DRIVER_APIis bumped for the newpdo_dbh_tfield.Tests
Seven new tests, all sqlite-based except one so they need no server:
pool_006pool_007pool_008pool_009pool_010pool_011pdo_pool_021query()does not burn a pool slotEach was verified to fail with its corresponding change reverted, not merely to pass —
pool_010andpool_011in particular, because the ownership layer masks the key-stability fix: without them, reverting the core change of this PR passed all 22 existing tests silently while losing a committed transaction and deadlocking a coroutine.The pool tests in
ext/pdo/testsalso resolve their helper throughREDIR_TEST_DIRinstead of__DIR__. Under--REDIRECTTEST--run-tests materialises the skip file in the driver's directory, so__DIR__pointed atext/pdo_sqlite/tests/and 11 tests borked out without running at all. BORKED went 22 -> 0.Verification
ext/pdo_sqlite+ext/pdo_mysql+ext/pdoReproducers checked under
USE_ZEND_ALLOC=0 valgrind: the original report, statement-outlives-coroutine, the GC-ordering case, and the failed-query()slot accounting. The GC case is worth calling out — it printedDoneand exited 0 while writing into freed memory, so it was only visible under valgrind.Not in this PR
Found while auditing this code path, filed separately: true-async/php-async#201, php#202, php#203. php#202 in particular is now precisely located (
pdo_pool.ccoroutine-finalize callback dereferencesbinding->dbhafter a release that suspended) but its fix belongs after this lands, since it touches the same paths.