fix: repair four OOM error-path bugs found by malloc-fault injection - #52
Merged
Conversation
…injection The new malloc-failure injection sweep (test/faultinject, PR #49) fails the Nth allocation across a workload and asserts every failure point returns cleanly. It found four genuine bugs, all on OOM error/cleanup paths; fixed here: 1. __db_env_destroy (src/env/env_method.c): on the early failure path in __env_create (the ENV calloc itself failed), dbenv->env is NULL, but the subsystem destructors and the ENV memset/free all dereference it -> NULL deref. Guard the whole ENV teardown on dbenv->env != NULL. 2. __lock_getlocker_int (src/lock/lock_id.c): the region locker-array grow loop used `if ((nlockers >> 1) == 0) break;` -- a no-op test with no assignment, so on allocation failure it either spun on the same size or broke with sh_locker still NULL while nlockers stayed >= 1; the following insert loop then walked a NULL sh_locker. Restore the intended halving (nlockers >>= 1) and move the __lock_nomem check ahead of the insert loop. 3. __db_pgin / __db_pgout (src/db/db_conv.c): a page swap dereferences the DB_PGINFO in cookie->data immediately. If a file was registered for page-in but its cookie was never set (an allocation failed during open), __memp_get_pgcookie hands back an empty DBT, so cookie->data is "" (or NULL) -> out-of-bounds / NULL read. Reject a missing/short cookie with EINVAL. 4. __txn_end (src/txn/txn.c): the transaction detail (td) was freed under TXN_SYSTEM_LOCK, then __lock_freelocker was called after the unlock -- __lock_freelocker_int reads td->si_ref (via the locker's td_off) to honor the SSI marker-lifetime protocol, dereferencing the freed td (heap-use-after-free, ASan). Free the locker BEFORE the detail, while td is still alive; the locker lives in its own LOCK_LOCKERS domain so ordering is unchanged. Verified: the malloc-injection sweep now PASSES (0 crash/hang/dirty over 506 failure points) under both plain debug and clang ASan; ssi001-006/009 (incl. multi-process concurrent writers) + txn001/002 + lock001 + test001 pass.
Coccinelle convention checksNo new violations. ✅ Resolved since baseline (2) -- update dist/cocci/baseline.txt to lock these in. |
ABI diff vs
|
This was referenced Jul 27, 2026
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.
The malloc-failure injection sweep (test/faultinject, #49) — fail the Nth allocation across a workload, assert every failure point returns cleanly — found 4 genuine bugs, all on OOM error/cleanup paths. All fixed here; the sweep now PASSES (0 crash/hang/dirty over 506 failure points) under both plain debug and clang ASan.
__db_env_destroy(env_method.c)dbenv->envcalloc failed__lock_getlocker_int(lock_id.c)>>typo (no>>=) → NULLsh_lockerwalked; nomem check too late__db_pgin/pgout(db_conv.c)__txn_end(txn.c)__lock_freelockerreadstd->si_refNotes
if ((nlockers >> 1) == 0) break;was meant to benlockers >>= 1— the retry loop never actually shrank the request. Fixed the halving and reordered the__lock_nomemcheck before the insert loop.__lock_freelocker_intreadstd->si_ref(the SIREAD-marker refcount) via the locker, but__txn_endfreedtdfirst. Fix: free the locker whiletdis still alive (the locker is in its ownLOCK_LOCKERSdomain, so ordering is preserved). Verified the full SSI suite incl. multi-processssi009still passes.Testing
ssi001–006,ssi009(multi-process concurrent writers),txn001/002,lock001,test001 btree— pass--enable-debug --enable-diagnostic)Found by / depends on #49 (malloc-fault injection harness).