Conversation
EnvWrap::open() iterated the environment cache by value, so the reference count was incremented on a copy and the cached entry stayed at 1. When a second Env opened an already-open path and then closed it, the count dropped to zero, mdb_env_close() ran and the entry was erased, leaving the first Env with a dangling MDB_env pointer. Its next transaction read freed memory, which crashed as a segmentation fault on Linux and macOS and as EXCEPTION_ACCESS_VIOLATION_READ on Windows. Iterating by reference makes the increment land on the cached entry, so the environment is only closed once its last user is gone.
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.
The bug
EnvWrap::open()iterated the environment cache by value:so
counton the cached entry stays at1no matter how manyEnvobjectsshare the environment.
EnvWrap::close()then decrements the real entry, seeszero on the first close, and calls
mdb_env_close()while anotherEnvisstill using that
MDB_env.Every later use of the first
Envdereferences freed memory. In ourapplication this showed up as a long-standing crash in
mdb_txn_begin, wherethe
MDB_txnread out of the environment was garbage.Reproducing it
Before the fix this dies with
SIGSEGV(exit 139) on macOS and Linux; onWindows the same code faults as
EXCEPTION_ACCESS_VIOLATION_READ.The fix
Iterate by reference so the increment lands on the cached entry. The
environment is then closed only when its last user calls
close().Test
Added
test/sharedenv.jsplus aShared environmentscase intest/index.test.js. It follows the existingCluster/Threadspattern andruns the scenario in a child process, because the failure mode is a fatal
signal rather than a catchable exception — running it inline would abort the
mocha process and silently skip every later suite.
The
closehandler checkssignalbeforecode, so a regression reportschild was killed by SIGSEGVinstead of a confusingnullexit code.Verified both directions locally on macOS (arm64, Node 22):
src/env.cppchange reverted, the new test fails withchild was killed by SIGSEGV: expected 'SIGSEGV' to equal null(44 passing before this PR, plus the new case)
I could not get a CI run to show up here:
.github/workflows/test.ymltriggerson
pushonly, with nopull_requesttrigger, so a fork PR produces no checks.Happy to add whatever else is useful.