fix(txn,lock): reclaim SSI reader bookkeeping (three resource leaks) - #146
Merged
Conversation
__txn_reap_si_details removed a committed reader's TXN_DETAIL from the
mvcc_txn list and freed it without releasing td->mvcc_mtx, while the two
other detail-free paths (__txn_end, __txn_remove_buffer) both do. Every
reaped detail therefore leaked one mutex slot for the life of the
environment; repetition shrank the mutex region until a later valid
operation returned ENOMEM ("BDB2034 unable to allocate memory for mutex").
Reached whenever a snapshot transaction both reads a multiversion database
(leaving a SIREAD marker, so si_ref > 0) and writes one (mvcc_ref > 0):
__txn_end parks the detail on mvcc_txn, the last MVCC buffer is evicted
while the marker is still live, and the reaper -- not __txn_remove_buffer
-- performs the final free.
Free the mutex before the detail, under TXN_SYSTEM_LOCK, exactly as
__txn_end does, so no new lock ordering is introduced.
Both free paths test the same predicate, so make TXN_DTL_SNAPSHOT an
explicit single-owner claim taken under td->mvcc_mtx (the latch every
writer of that flag already holds). Previously __txn_remove_buffer
dropped mvcc_mtx, then took the region lock and freed unconditionally;
with the reaper now also freeing the mutex, that window would be a
double free of the mutex slot.
Fixes #138
…rker goes
SIREAD cleanup removed a committed reader's obsolete markers but never
reclaimed the DB_LOCKER_FREED locker whose reclamation those markers had
deferred, nor its logical mutex. Sequential read-only DB_TXN_SNAPSHOT
transactions therefore accumulated one locker (and one mutex slot) each,
with only one transaction ever active, until DB_ENV->txn_begin returned
ENOMEM.
The deferral chain was correct up to the last step: __lock_sicommit
detaches the markers and flags the locker DB_LOCKER_FREED,
__lock_freelocker_int defers while the detail's si_ref is nonzero, and
__lock_siclean_obj later drops each obsolete marker -- but nothing then
completed the deferred free.
__lock_siclean_obj now uses atomic_dec's return value to notice it removed
the LAST marker of a DB_LOCKER_FREED locker, and marks the locker
reclaimable by clearing its td_off. The new __lock_sireap_lockers frees
those lockers from __lock_sicleanup after the object partition mutexes are
released.
Ordering, and why this cannot use-after-free:
* Freeing a locker needs LOCK_LOCKERS. The established nesting is
LOCK_LOCKERS -> partition (__lock_sicommit, the deadlock detector), so
the reclaim runs outside every partition mutex rather than taking
LOCK_LOCKERS under one. The M2 note's "do not free lockers while
holding a partition mutex" rule is preserved.
* The reclaim pass deliberately dereferences no TXN_DETAIL. mpool may
free the detail the instant si_ref reaches zero, so the marker count is
observed only where it is safe -- as atomic_dec's return value in
__lock_siclean_obj, under the partition mutex -- and the reclaim then
tests only locker-local state (flag, td_off, empty heldby). This is
also why the marked locker records INVALID_ROFF instead of leaving
td_off pointing at a detail that may already be gone.
* Lockers are reclaimed before __txn_reap_si_details, so no surviving
locker can name a freed detail.
* The (DB_LOCKER_FREED && td_off == INVALID_ROFF) pair is set only by
__lock_siclean_obj: a live locker never carries DB_LOCKER_FREED and a
still-deferred one keeps its td_off, so ordinary lockers are untouched.
Fixes #137
Both #137 and #138 are resource-exhaustion bugs: nothing crashes, no page is corrupt, no sanitizer fires -- slot counts just grow once per transaction until a later valid operation returns ENOMEM. Only mechanical accounting catches that shape, so each driver reads the counts back through the public statistics APIs (DB_ENV->lock_stat, ->mutex_stat, ->mutex_stat_print) and fails if they track the transaction count. leak_si_locker (#137): 2500 sequential read-only DB_TXN_SNAPSHOT transactions in one long-lived environment, one active at a time, no checkpoint. leak_si_mvcc_mtx (#138): snapshot transactions that read one multiversion database and write another, with cache churn to force MVCC eviction, so the final detail free lands in __txn_reap_si_details. Each has a control mode that creates no SIREAD marker (plain transaction / no read); the controls pass both before and after the fix, so a control failure indicts the harness rather than the library. The marker sweep is best-effort and fires on a lock-region threshold, so the fixed steady state is a sawtooth rather than a flat line. The assertion is therefore peak-per-half-of-run: equal peaks pass, and a per-transaction leak makes the second half's peak strictly larger. That is independent of both the transaction count and the sawtooth phase. Measured (2500 txns / 700 cycles, release -O2): #137 before: lockers 2 -> 1410, mutexes 213 -> 1621, ENOMEM from txn_begin after 1408 txns after: lockers peak 198/193 per half, mutexes 409/404, all 2500 complete #138 before: "txn mvcc" 57 -> 651, in_use 372 -> 1619, ENOMEM at cycle 653 after: "txn mvcc" peak 104/103 per half, ends at 4, all 700 complete Run with test/c/leak-run.sh (timeout-bounded, deterministic, single thread); `make leak_tests` builds the drivers alone.
Third leak in the same chain, found while validating #137/#138. The 'upgrading our own SIREAD to WRITE' branch in __lock_get_internal removes the marker from sh_obj->sireaders but never decrements the owning detail's si_ref, unlike the other two removal sites (__lock_sicommit, __lock_siclean_obj). si_ref therefore stays permanently above the true marker count, so the detail -- and the locker deferring its free on it -- can never be reclaimed: a snapshot transaction that reads then writes the same key leaks a detail, a locker and their mutex slots every iteration until txn_begin returns ENOMEM. Safe by construction: this is the reader's OWN marker (sh_off == holder), so the detail is its live running transaction; si_ref cannot reach zero here and no reclaim can trigger underneath us. Guarded with the same td_off != INVALID_ROFF test the grant path uses. Measured: growth 672 -> 23 slots per 1000 txns and the ENOMEM is gone.
The soak tier (PR #143) marked ro_snapshot and mvcc_retained expect_leak=1. With #137/#138 fixed those now pass, which the harness correctly reports as UNEXPECTED PASS -> exit 1. Clear both flags. mvcc_retained needed more than a flag change. Its residual growth is a BOUNDED SAWTOOTH, not a leak: markers accumulate until the GC trigger fires (live count past half the allocated lock objects) and then collapse. Measured over 30k txns the locker count runs 175 -> 112 -> 43 -> 178 with no ENOMEM -- it oscillates, it does not climb. A least-squares slope cannot express that, and a 2000-txn window lands mid-rise and reads as a leak. Two changes so the check states the honest property: - soak_peak_grew(): a counter over tolerance is only a leak if the SECOND half also peaks >10% higher than the first. A leak climbs; a sawtooth returns. - per-workload min_txns: mvcc_retained asserts over >=25000 txns so the window covers a full collect cycle (its period is ~19k). The run reports when it raises the count. Tier now: 5 workloads, 0 unexpected outcomes, exit 0. The three controls (rw_plain, aborted, cursor_churn) still pass, so the check has not been defanged.
|
ABI diff produced no report (build skipped or no base tag). Advisory: libabigail/nm is the authoritative binary-ABI check; Coccinelle is complementary source-level early warning. See dist/cocci/README.md. |
Coccinelle convention checksNo new violations. ✅ Resolved since baseline (2) -- update dist/cocci/baseline.txt to lock these in. |
The tiers were deliberately advisory while the bugs they reproduce were open, with their own comments saying to flip them in the fixing PR. Both conditions are now met: - B3 (lock-mode matrix under ASan) reproduced the #140 heap overflow in __lock_vec; #145 fixed the lock-list sizing and the matrix passes, so any ASan fault in the lock list is now a real regression. - B2 (resource-accounting soak) reproduced #137/#138; this PR fixes them and the tier reports 5 workloads / 0 unexpected outcomes. Dropping continue-on-error from both. B1 was already a hard gate. Verified on this PR: B1 pass, B3 pass.
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.
Fixes #137. Fixes #138.
Three leaks in the same SSI cleanup chain — the two reported, plus a third found while validating.
#138 — MVCC mutex slot leaked
__txn_reap_si_detailsremoved the detail and called__env_alloc_freewith no__mutex_free(&td->mvcc_mtx), while__txn_end(txn.c:1851) and__txn_remove_bufferboth free it. Fix frees the mutex before the detail, underTXN_SYSTEM_LOCK— the same ordering__txn_endalready uses, so no new lock ordering is introduced.Because the reaper and
__txn_remove_buffertest the same predicate, and the latter droppedmvcc_mtxbefore taking the region lock, naively adding the free opened a double-free window.TXN_DTL_SNAPSHOTis now a single-owner claim taken undertd->mvcc_mtx— the latch every writer of that flag already holds.#137 — committed-reader locker never reclaimed
The deferral chain was correct until the last step: nothing ever completed the deferred free.
__lock_siclean_objnow usesatomic_dec's return value to detect that it removed the last marker of aDB_LOCKER_FREEDlocker, and the new__lock_sireap_lockersfrees those from__lock_sicleanupafter all partition mutexes are released.Why this can't UAF: locker frees need
LOCK_LOCKERS, and the established nesting isLOCK_LOCKERS -> partition, so the reclaim runs outside every partition mutex (preserving the M2 rule). The reclaim pass dereferences noTXN_DETAIL— mpool may free the detail the instantsi_refhits 0, so the count is observed only asatomic_dec's return under the partition mutex, and the pass tests only locker-local state. That's also why the marker recordsINVALID_ROFFinstead of leavingtd_offpointing at possibly-freed memory. Lockers are reclaimed before__txn_reap_si_details, so no surviving locker names a freed detail.Third leak —
si_refnot decremented on SIREAD→WRITE upgrade__lock_get_internal's "upgrade our own SIREAD to WRITE" branch removed the marker but never decrementedsi_ref, unlike the other two removal sites.si_refstayed permanently above the true marker count, so the detail and the locker deferring on it could never be reclaimed: a snapshot txn that reads then writes the same key leaked on every iteration. Safe by construction — it's the reader's own marker on its live transaction, sosi_refcan't reach zero there.Measured (release -O2)
txn mvcc57→651, in_use 372→1619, ENOMEM at cycle 653Soak tier reconciliation
The tier from #143 marked both shapes
expect_leak=1; they now pass, so the flags are cleared.mvcc_retainedneeded more: its residual is a bounded sawtooth, not a leak — over 30k txns lockers run 175 → 112 → 43 → 178 with no ENOMEM. Addedsoak_peak_grew()(a counter over tolerance is only a leak if the second half also peaks higher) and a per-workloadmin_txnsso that shape asserts over ≥25k txns, covering a full collect cycle. Tier: 5 workloads, 0 unexpected outcomes, exit 0 — with all three controls still passing, so the check isn't defanged.Validation
Builds clean: debug+diagnostic, release -O2, ASan, TSan, DST. ssi001/002/003/009, txn001/2/3, lock001/2/3, recd001, test001 btree+hash — all pass.
ssi009under ASan: 0 faults (the UAF gate for the reclaim change). TSanssi009: same 3 pre-existing base-BDB mpool warnings as unfixed master — no new race. Fuzz gate 9/9;test_sim_crash_recoverpasses.