Skip to content

Fix use-after-free when two Env objects share one environment - #228

Open
Lotu527 wants to merge 1 commit into
Venemo:masterfrom
Lotu527:fix-shared-env-refcount
Open

Lotu527 wants to merge 1 commit into
Venemo:masterfrom
Lotu527:fix-shared-env-refcount

Conversation

@Lotu527

@Lotu527 Lotu527 commented Aug 31, 2026

Copy link
Copy Markdown

The bug

EnvWrap::open() iterated the environment cache by value:

for (env_path_t envPath : envs) {   // copy
    char* existingPath = envPath.path;
    if (!strcmp(existingPath, *charPath)) {
        envPath.count++;            // increments the copy, not the cache entry
        ...

so count on the cached entry stays at 1 no matter how many Env objects
share the environment. EnvWrap::close() then decrements the real entry, sees
zero on the first close, and calls mdb_env_close() while another Env is
still using that MDB_env.

Every later use of the first Env dereferences freed memory. In our
application this showed up as a long-standing crash in mdb_txn_begin, where
the MDB_txn read out of the environment was garbage.

Reproducing it

const lmdb = require('node-lmdb');

const env1 = new lmdb.Env();
env1.open({ path: dir, maxDbs: 4 });

const env2 = new lmdb.Env();
env2.open({ path: dir, maxDbs: 4 });   // same path -> shares env1's MDB_env

env2.close();                          // frees the shared MDB_env

env1.beginTxn();                       // reads freed memory

Before the fix this dies with SIGSEGV (exit 139) on macOS and Linux; on
Windows 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.js plus a Shared environments case in
test/index.test.js. It follows the existing Cluster / Threads pattern and
runs 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 close handler checks signal before code, so a regression reports
child was killed by SIGSEGV instead of a confusing null exit code.

Verified both directions locally on macOS (arm64, Node 22):

  • with the src/env.cpp change reverted, the new test fails with
    child was killed by SIGSEGV: expected 'SIGSEGV' to equal null
  • with the fix applied, the full suite is green: 45 passing, 1 pending
    (44 passing before this PR, plus the new case)

I could not get a CI run to show up here: .github/workflows/test.yml triggers
on push only, with no pull_request trigger, so a fork PR produces no checks.
Happy to add whatever else is useful.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant