fix: N-API use-after-free during worker-env teardown, plus two VT/transaction lifecycle races (#741) - #745
fix: N-API use-after-free during worker-env teardown, plus two VT/transaction lifecycle races (#741)#745kriszyp wants to merge 15 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses critical race conditions and use-after-free (UAF) vulnerabilities under worker-environment churn (#741). It introduces TransactionHandle::stateMutex to serialize transaction and verification table (VT) lock states, implements tryRegisterAsyncWork to safely track active operations, and adds a ParkedFlagRegistry to prevent wake-callback thread-safe functions (TSFNs) from outliving their parent environments. The review feedback highlights a potential null-pointer dereference in TransactionHandle::getCount if called on a closed transaction with an active snapshot, and suggests refactoring the manual erase-remove idiom in ParkedFlagRegistry to use C++20's cleaner std::erase_if helper.
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit 85676cd |
|
Update on the blocking finding: spent another round investigating per Kris's request, ruling out three hypotheses and confirming ThreadSanitizer isn't practical here without a from-source Node build. Full detail in Ruled out this round:
ThreadSanitizer: wired up ( Net: the regression traces to something in this fix's actual new logic (the 🤖 Generated with Claude Code |
|
Retraction of my earlier comment on this PR. The comment above claiming this fix converts the repro from "15/15 clean" to "14/15 crashing" is The baseline arm in that comparison was a false negative. It ran in a separate worktree where Re-measured in a single worktree (identical RocksDB prebuild, node_modules and build config; only
So the fix neither causes nor fixes that crash, and the crash matches the 7/12 rate #741 itself Lesson recorded in Apologies for the noise from the incorrect earlier analysis. 🤖 Generated with Claude Code |
kriszyp
changed the title
|
Pre-push review round (DLC step 10) — outcome and what changed.
Both non-Claude legs independently flagged one thing I had only noted rather than fixed, so I fixed it:
Findings I did not act on, with reasons:
Post-fix verification: 🤖 Generated with Claude Code |
…rdown DBDescriptor::finishClose() (worker-env teardown, e.g. via DBRegistry::Shutdown()) closes every TransactionHandle registered on a shared descriptor -- including ones owned by a different, still-live env that may be mid-commit, mid-put, or mid-abort at that exact moment. Two independent races there produced the reported heap corruption: 1. close()'s waitForAsyncWorkCompletion() only waited for async work already registered on activeAsyncWorkCount -- a TOCTOU let a racing Commit()/CommitSync/Abort register (or start touching txn) after close() had already observed zero in-flight work and moved on to delete this->txn and releaseIntent()'s vector mutation. Fixed by serializing txn/lockedVTSlots/heldTrackers access with a new TransactionHandle::stateMutex: close() flips `closed` under the mutex before waiting, so no new work can register once it starts waiting for what already did. 2. A coordinated-retry commit that loses (IsBusy) parks its RETRY_NOW resolution on the winning holder's LockTracker via a wake callback that captures a napi_threadsafe_function by value. LockTracker is process-global VT state, so that callback can fire on any thread an arbitrary time later -- including after the parking transaction's own env has torn down and Node has reclaimed the tsfn, making the callback's napi_release_threadsafe_function call a use-after-free (confirmed via gdb: uv_mutex_lock aborting inside it, matching the issue's crash trace exactly). Fixed with a ParkedFlagRegistry, mirroring DBDescriptor's per-env commitCompletions pattern: env cleanup invalidates that env's outstanding parked flags before Node frees its tsfns, and the wake callback checks the same flag under a shared mutex before touching the tsfn. Also adds holders/refcount underflow assertions in VerificationTable::releaseWriteIntent/unrefTracker as a hard invariant check, and a worker_threads churn regression test (VT materialized, coordinatedRetry, graceful env recycling) adapted from the investigation's proven repro scripts. Verified against the original repro-vt-stress.mjs / repro-crossthread.mjs scripts (graceful and abrupt teardown, up to 300k+ operations) and under AddressSanitizer: no crash, no ASan report, all settled. Fixes #741 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Findings from the Gemini + Grok legs of the pre-push review: - Upgrade the holders/refcount underflow checks from assert() (a no-op in NDEBUG/release builds -- exactly where the invariant matters most) to an always-on vtInvariant() that logs and aborts. A silent wraparound would strand a locked VT slot and mask the corruption this fix targets. - tryRegisterAsyncWork() now also atomically gates TransactionHandle::get() (async), closing the same TOCTOU already fixed for Commit/CommitSync/Abort. - Fix a leaked activeAsyncWorkCount registration when napi_queue_async_work fails after tryRegisterAsyncWork() succeeds in the legacy commit path -- would have hung close()'s waitForAsyncWorkCompletion() indefinitely. - close() no longer early-returns on a null txn before running waitForAsyncWorkCompletion()/releaseIntent() -- every step now self-guards instead of being skipped as a block, in case async work is still registered against a handle whose txn happens to be transiently null. - Amortize ParkedFlagRegistry::registerFlag()'s prune sweep (only run it when the per-env vector is about to reallocate) instead of an O(n) scan under the process-global lock on every single park. - Trim repeated inline "(#741)" citations down to the handful of canonical doc comments; the rest now explain the same constraints without repeating the issue number as narration. - Exercise CommitSync in the churn regression test (previously only Commit/Abort were covered). - Document two deliberately-deferred gaps in AGENTS.md: putSync/getSync/ removeSync still don't route through tryRegisterAsyncWork() (hot-path cost not accepted without dedicated review) and the pre-existing 5s waitForAsyncWorkCompletion() timeout can still let close() proceed past a genuinely slow (not parked) commit. Re-verified: native GoogleTest suite (104/104), full Vitest suite (710/710), and the original repro-vt-stress.mjs script (20s/4 workers) under both a plain build and AddressSanitizer -- clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…est, document critical unresolved teardown regression CommitSync() and getCount() are reverted to their pre-review-round-2 state (no tryRegisterAsyncWork()/AsyncWorkGuard): adding that registration reproduced a separate heap-corruption regression under worker-churn testing that was not root-caused in time; both are documented in AGENTS.md item 10a as deliberately deferred. The vt-lock-tracker-churn Vitest regression test is skipped by default: even tuned to a low-churn cadence it intermittently hits a severe, unrelated crash (see AGENTS.md item 12) and would make CI flaky for a reason outside this fix's scope. The fixture and worker script are kept for manual reproduction. AGENTS.md item 12 documents a CRITICAL, UNRESOLVED finding from this round's testing: comparing the pre-fix commit against every tested version of this fix on the project's own repro-crossthread.mjs (GRACEFUL=1, same settings) shows 15/15 clean at baseline vs. 14/15 crashing with the fix applied. Two gdb backtraces land in the pre-existing (unmodified by this fix) DBDescriptor::finishClose() closables-teardown loop, not in this fix's own new code paths. This needs dedicated follow-up (ThreadSanitizer is the likely next tool) before this fix should be merged. Refs #741 Co-Authored-By: Claude Opus <noreply@anthropic.com>
…up with ruled-out hypotheses Adds a ThreadSanitizer build toggle to binding.gyp (ROCKSDB_TSAN=1, mirroring the existing ROCKSDB_ASAN one) as diagnostic infrastructure for investigating AGENTS.md item 12. Updates item 12 with this round's findings: three hypotheses were tested and ruled out (writerMutex_ held during VT wake callbacks; un-closed/leaked transactions inflating the finishClose() closables backlog -- real, but not the cause; and raw TransactionHandle::close() latency, tested decisively via the existing ROCKSDB_JS_TXN_CLOSE_DELAY_MS seam against unmodified baseline code at 10ms and 100ms, both clean 20/20). Running the repro under TSan was attempted but is not currently practical: every race across several runs resolves to pure V8-internal GC/JIT machinery with zero rocksdb_js:: frames, a known limitation of running vanilla Node under TSan without Node's own build-time instrumentation. Refs #741 Co-Authored-By: Claude Opus <noreply@anthropic.com>
…false-negative baseline The earlier conclusion that this fix converts repro-crossthread.mjs from 15/15 clean to 14/15 crashing was wrong. The baseline arm ran in a separate worktree where pnpm install had run but pnpm build:bundle had not, so dist/index.mjs did not exist: every worker died on "Cannot find module", did zero work, and the harness still printed "RESULT: no stuck commit" and exited 0. All ~35 "clean baseline" data points -- including the ROCKSDB_JS_TXN_CLOSE_DELAY_MS runs used to rule out close() latency -- were no-op runs. Re-measured within one worktree (identical RocksDB prebuild, node_modules and build config, competing build paused, each run verified to have issued ~580k transactions): baseline ea83ff4 9/12 crashed, this fix 8/12 crashed. The fix neither causes nor fixes that crash, and the rate matches the 7/12 reported in the issue. Rewrites AGENTS.md item 12 accordingly: the real finding is that this fix closes two genuine races but does NOT resolve #741's headline repro. Also records the methodology warning (verify a repro run actually did work before trusting a green result; compare revisions in one worktree), the ASan blind spot from the non-instrumented prebuilt librocksdb.a, and that a from-source TSan Node needs -DTHREAD_SANITIZER but must leave V8_IS_TSAN off. Refs #741 Co-Authored-By: Claude Opus <noreply@anthropic.com>
…scripts
The repo's own test/fixtures/*.mts already fail correctly on a worker
startup error (worker.once('error', reject)); only the ad-hoc scripts
under ~/dev/tmp/harper-2001-repro/ swallow it and still exit 0.
Refs #741
Co-Authored-By: Claude Opus <noreply@anthropic.com>
… dominant crash Node's Environment::RunCleanup() runs principal_realm_->RunCleanup() *before* cleanup_queue_.Drain(), so by the time our module's env-cleanup hook runs, Realm cleanup has already destroyed the env's BaseObjects and freed N-API per-env state. TransactionHandle::close() guarded its napi_delete_reference call with "am I on the owning JS thread?" -- but env teardown runs on exactly that thread, so the guard passed and the call wrote through a freed env (napi_delete_reference -> napi_clear_last_error writes last_error). That corrupts glibc heap metadata; the abort then surfaces later and elsewhere, usually inside RocksDB's own allocators (~Arena / ~WriteBatchWithIndex / ~TransactionBaseImpl), which is why this read as a RocksDB/VT bug. Confirmed with ThreadSanitizer against a from-source TSan Node: three heap-use-after-free writes in napi_clear_last_error, reached from TransactionHandle::close() <- DBDescriptor::finishClose() <- DBRegistry::Shutdown() <- our cleanup hook. Adds napi/env_teardown.h (thread-local EnvTeardownScope + isEnvTearingDown()), scopes the cleanup hook with it, and makes the close path consult it. During teardown the reference is reclaimed with the env anyway, so skipping is correct. Effect on repro-crossthread.mjs (GRACEFUL=1 RECYCLE_MS=2000, 4 workers, 15s, ~1M transactions/run): 9/12 crashing -> 0/12. Still clean 8/8 at a harsher 800ms recycle. TSan goes from 3 races to 0. test/vt-lock-tracker-churn.test.ts stays skipped: it still fails ~25% via its tsx-transpiled-worker path with the same signature, which is a separate, not-yet-root-caused issue (documented in AGENTS.md item 12 along with the TSan-Node recipe and a methodology warning about false-negative baseline runs). Full suite 710 passed / 2 skipped; native 104/104. Refs #741 Co-Authored-By: Claude Opus <noreply@anthropic.com>
… teardown Second instance of the bug class fixed in the previous commit, flagged independently by the Grok review leg: DBHandle::close() releases its transaction-log JS references with napi_delete_reference, and is reachable from DBRegistry::Shutdown() inside the env cleanup hook -- so on last-env exit (or a worker unloading the addon) with open transaction-log handles it writes into freed N-API env state, the same delayed-heap-abort shape as the TransactionHandle path. Guarded with isEnvTearingDown(); the refs are reclaimed with the env. Only the close() loop is guarded -- unrefLog()/useLog() are called from live JS, not from teardown. Also trims the comments added in the previous commit down to the invariant they encode, per repo convention (no narration of mechanics or history) -- raised by both review legs. repro-crossthread.mjs still 0/10; full suite 709 passed / 3 skipped; native 104/104. Refs #741 Co-Authored-By: Claude Opus <noreply@anthropic.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
b3e8130 to
c740d2e
Compare
|
I have fixed the merge conflicts, but I have some reservations about this PR. The description states "N-API calls are unsafe inside", but that's false. This PR adds the env-teardown guard and mutexes, and that's cool. But the friction is this is a 947-line three-race fix with an unresolved residual crash, a skipped test, and its only real evidence living outside the repo. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| // (0 races over 3 aggressive runs -- TSan's ~15x slowdown likely closes the | ||
| // window). It is NOT root-caused, so enabling this would make CI flaky for | ||
| // a reason this PR does not address. See AGENTS.md item 12. | ||
| it.skip( |
There was a problem hiding this comment.
Medium: three teardown fixes ship with no runnable regression coverage
This is the only test the PR adds and it's it.skip, because the ~25% residual crash it exercises is not root-caused. The evidence cited for the dominant fix (repro-crossthread.mjs, "9/12 → 0/12") is not committed to the repo, so neither a reviewer nor CI can reproduce it. The stateMutex, ParkedFlag, and env-teardown fixes have no passing automated test — nothing here catches a regression.
Suggested fix: commit the repro-crossthread.mjs scenario as an opt-in/long-form spawned-child test. The existing ROCKSDB_JS_TXN_CLOSE_DELAY_MS / ROCKSDB_JS_COMMIT_DELAY_MS seams make the close-vs-commit race deterministic without relying on the flaky fixture — at minimum, don't leave all three fixes uncovered.
—
Generated by Barber AI
| /** | ||
| * True while the calling thread is inside its env's cleanup hook. | ||
| * | ||
| * Node runs `Realm::RunCleanup()` — which frees N-API per-env state — before |
There was a problem hiding this comment.
Medium: the documented root cause doesn't match Node's teardown ordering
Checked against Node source: Realm::RunCleanup() only resets binding-data and BaseObjects — it does not free the napi_env struct where last_error lives. The napi_env's own teardown (Unref→DeleteMe) is itself a cleanup-queue hook registered at env creation, and the queue drains in reverse (LIFO) order, so it runs after this module's hook. For the env being torn down, last_error is still valid memory when the hook runs.
A use-after-free write to last_error therefore implies the reference belongs to a different, already-destroyed worker env — i.e. the cross-env lifetime problem the stateMutex/ParkedFlag changes address, not "the realm freed my own env's N-API state." The isEnvTearingDown() guard is cheap and safe, so keep it — but please reconcile this explanation against the actual TSan/gdb trace (is the crashing env the current thread's, or foreign?) before enshrining it in AGENTS.md item 14, and don't treat #741 as closed on this basis.
—
Generated by Barber AI
| // Skip during env teardown: this is reachable from DBRegistry::Shutdown() | ||
| // inside the env cleanup hook, by which point Node has already freed the | ||
| // env's N-API state and the refs go with it (napi/env_teardown.h). | ||
| if (!isEnvTearingDown()) { |
There was a problem hiding this comment.
Medium: sibling napi_delete_reference sites left unguarded by this fix's own logic
This guard is correct, but napi_delete_reference is still called unconditionally in DBHandle::unrefLog() (line 371) and DBHandle::useLog() (line 392). The PR description itself notes db_handle.cpp "has further napi_delete_reference sites reachable from DBRegistry::Shutdown()" that were left alone — by this fix's own reasoning those are latent heap corruption during teardown.
Suggested fix: either wrap those sites in the same isEnvTearingDown() check, or confirm and document that they are unreachable from DBRegistry::Shutdown(). Shipping a self-documented reachable UAF defers the crash rather than fixing it.
—
Generated by Barber AI
|
Closing this PR in favor of #780. |
2 participants
Human-Review-Need: 4 @ c740d2e
Root cause found: N-API calls are unsafe inside
napi_add_env_cleanup_hookNode's
Environment::RunCleanup()runsprincipal_realm_->RunCleanup(); cleanup_queue_.Drain();in that order. So by the time our module's env-cleanup hook runs,
Realm::RunCleanup()hasalready destroyed the env's
BaseObjects and freed N-API per-env state.TransactionHandle::close()guarded itsnapi_delete_referencewith "am I on the owning JSthread?" — but env teardown runs on exactly that thread, so the guard passed and the call
wrote through a freed env (
napi_delete_reference→napi_clear_last_errorwriteslast_error).That corrupts glibc heap metadata; the abort then surfaces later and somewhere unrelated — usually
inside RocksDB's own allocators (
~Arena/~WriteBatchWithIndex/~TransactionBaseImpl), whichis why this read as a RocksDB/VT bug for so long.
Confirmed with ThreadSanitizer against a from-source TSan Node — three heap-use-after-free writes
in
napi_clear_last_error, reached fromTransactionHandle::close()←DBDescriptor::finishClose()←
DBRegistry::Shutdown()← our cleanup hook.Fix:
src/binding/napi/env_teardown.h— a thread-localEnvTeardownScopeset for the durationof the cleanup hook; close paths check
isEnvTearingDown()before making N-API calls. Duringteardown the reference is reclaimed with the env anyway, so skipping is correct.
Measured effect
repro-crossthread.mjs(GRACEFUL=1 RECYCLE_MS=2000, 4 workers, 15s, ~1M transactions/run), all inone worktree with runs verified to have actually done work:
ea83ff46Still clean 8/8 at a harsher 800ms recycle. TSan goes from 3 races to 0.
Also included: two VT/transaction lifecycle races
close()vs. in-flight async commit —close()could deletetxnwhilea commit that registered after
waitForAsyncWorkCompletion()saw zero work was still using it.Fixed with a
stateMutex, withclose()flippingclosedunder it before draining.ParkedFlagRegistry, mirroring the existingcommitCompletions/ReleaseCommitCompletionsByEnvper-env teardown pattern.
Both were confirmed against gdb backtraces matching the issue's signatures. Note these two do not
move the repro's crash rate on their own (bisected) — the UAF above was what that repro was hitting.
Known remaining (not root-caused)
test/vt-lock-tracker-churn.test.tsstill fails ~25% (1/4 reps at 4000ms, 1/6 at 1500ms) with thesame
corrupted size vs. prev_size, so it staysit.skip. It reproduces only through thatfixture's tsx-transpiled-worker path, never through the plain-
.mjsrepro, and TSan does not catchit. Also still open:
getSync/putSync/removeSync/CommitSync/getCountremain outsidetryRegisterAsyncWork()(AGENTS.md item 10a).Because of that residual, I'd suggest keeping #741 open after this merges, scoped to what's left.
Notes for reviewers
db_handle.cpphas furthernapi_delete_referencesites reachable fromDBRegistry::Shutdown()that this repro didn't exercise. They likely need the sameisEnvTearingDown()guard — I left them alone rather than change code I couldn't test.-DTHREAD_SANITIZER;V8_IS_TSANmust stay off or V8 won't compile underv8_enable_sandbox=0), why ASan is structurally blind here, and a methodology warning aboutfalse-negative baseline runs that cost me significant time this session.
binding.gypgains aROCKSDB_TSAN=1toggle alongside the existingROCKSDB_ASAN.Test coverage
Full Vitest suite 710 passed / 2 skipped; native GoogleTest 104/104.
Refs #741
🤖 Generated with Claude Code