Repository navigation
Conversation
WalkthroughFixes use-after-free in ChangesNapiEnv lifetime fix and external string finalizer tests
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:10 PM PT - Jul 9th, 2026
❌ @robobun, your commit 4ff5e80 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32277That installs a local version of the PR into your bun-32277 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi-app/external_string_addon.c`:
- Around line 42-43: The malloc calls at lines 42 and 55 in
external_string_addon.c are not guarded with null checks before the subsequent
memcpy operations. If malloc fails and returns NULL, the memcpy will dereference
the NULL pointer and crash the process. Add null checks immediately after each
malloc call to verify the buffer was successfully allocated, and if allocation
fails, return a controlled N-API error (such as napi_generic_failure or similar
error handling) instead of allowing the crash to occur. Apply this same
defensive programming pattern to both malloc/memcpy pairs at the anchor location
(lines 42-43) and the sibling location (lines 55-56).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69e20927-fdf5-405a-bc3e-9ffc08cb3019
📒 Files selected for processing (4)
src/jsc/bindings/napi.cpptest/napi/napi-app/binding.gyptest/napi/napi-app/external_string_addon.ctest/napi/napi-external-string-env.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/napi/napi-external-string-env.test.ts:119-120— Nit: per test/CLAUDE.md ("CRITICAL: Do not set a timeout on tests. Bun already has timeouts."), drop the60_000timeout argument here. The120_000onbeforeAllis fine — node-gyp builds are slow and other napi tests use the same pattern — but the per-test timeout should go.Extended reasoning...
What this is.
test/CLAUDE.md:120states:CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
The new test passes
60_000as the third argument totest.skipIf(isWindows).each([...])(name, fn, 60_000)at the end of the file. This is a direct violation of the documented test convention for this repository.Why the rule exists. Bun's test runner already applies a global per-test timeout, and CI further wraps test files with its own timeout budget. Sprinkling per-test numeric timeouts creates a second source of truth that drifts from the runner default, makes it harder to globally tune CI timing, and tends to either mask hangs (when set too high) or cause spurious flakes (when set too low). The repo convention is to leave timing to the harness.
Step-by-step.
test.skipIf(isWindows).each(["createLatin1", "createUtf16"])(title, async fn => { ... }, 60_000)— the trailing60_000is the per-test timeout in ms.- The test body spawns one child
bunprocess that starts a worker, creates 64 external strings, tears the worker down, and prints two lines. There is no node-gyp work in the test body (that's already done inbeforeAll), so this is a few-second operation at most — well under the runner default. - Therefore the explicit
60_000adds nothing functionally and only contradicts the style guide.
What about
beforeAll(..., 120_000)? That one is defensible: it shells out tobun installwhich runs the full node-gyp build oftest/napi/napi-app(a dozen-plus targets). Several existing napi tests use the samebeforeAll(..., 120_000)pattern for exactly this reason, so leaving it is consistent with the surrounding tree. Only the per-test60_000is the outlier.Precedent. There is one prior per-test timeout in
test/napi/napi.test.ts:757(25_000), so this isn't unprecedented — but the CLAUDE.md guidance is explicit and newer code should follow it rather than copy the exception.Fix. Delete the trailing
60_000,argument:}, - 60_000, );
This is a convention nit, not a correctness issue — the test passes either way.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi-external-string-env.test.ts`:
- Around line 111-114: The test assertion in napi-external-string-env.test.ts
uses a strict `stderr: ""` check which can cause false positives due to
sanitizer and runtime noise in ASAN-sensitive tests. Replace the exact empty
string assertion for stderr with a check that verifies stderr does NOT contain
sanitizer error signatures (such as ASAN error patterns or UAF indicators). Keep
the combined object assertion structure but modify the stderr property to use a
negation matcher instead of requiring exact emptiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 595e4fb7-dc44-4a77-92e2-6c2e2c62b46a
📒 Files selected for processing (1)
test/napi/napi-external-string-env.test.ts
There was a problem hiding this comment.
LGTM — the Ref capture matches the established pattern at napi.cpp:1167/2087, and all prior feedback has been addressed.
Extended reasoning...
Overview
The production change is two lines in src/jsc/bindings/napi.cpp: the ExternalStringImpl free-callback lambdas in node_api_create_external_string_latin1 and node_api_create_external_string_utf16 now capture env = WTF::Ref<NapiEnv>(*env) instead of the raw napi_env pointer. This is a mechanical pattern-match against the identical capture already used in the same file at napi_add_finalizer (line 1167) and napi_create_external_buffer (line 2087). The remaining changes are test scaffolding: a small C addon (external_string_addon.c), a binding.gyp target, and a worker-teardown test that exercises both finalizer paths under Malloc=1 + ASAN on Linux.
Security risks
None introduced. The change is a strict lifetime tightening: holding a Ref<NapiEnv> in the lambda can only extend the env's lifetime, never shorten it, so it cannot create a new UAF. The Ref is released when the ExternalStringImpl is destroyed, so there is no leak or cycle — the same ownership shape already runs in production via the two sibling call sites. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low-to-moderate. napi.cpp is core binding code, but this specific edit is a two-token capture change that brings two functions in line with an in-file precedent that has already been validated in production. The PR description gives a thorough analysis of why the UAF window is layout-dependent and hard to gate-prove, which is consistent with treating this as defensive hardening rather than a hot-path behavior change. Ref<T>::operator-> makes env->doFinalizer(...) compile identically, so there is no semantic drift in the call itself.
Other factors
I left four inline comments on earlier revisions; three were applied (dropped the stale collectContinuously comment + Windows skip in fe0bf0a, switched to the combined {stdout, stderr, exitCode} assertion in c20da88, moved to test.concurrent.each in 57562c7) and one was reasonably declined (the pre-existing copied out-param asymmetry is already tracked by a separate open PR). CodeRabbit's malloc null-check nit was declined consistently with the sibling isolate_finalizer_addon.c pattern. The bug-hunting pass on the current head found nothing. No CODEOWNERS cover these paths. Tests pass per the PR description, and the new test follows the project's subprocess-test conventions after iteration.
There was a problem hiding this comment.
LGTM — the two-line capture change matches the established Ref<NapiEnv> pattern at lines 1167/2087, and all prior review feedback has been addressed.
Extended reasoning...
Overview
The production change is two lines in src/jsc/bindings/napi.cpp: the ExternalStringImpl finalizer lambdas in node_api_create_external_string_latin1 and node_api_create_external_string_utf16 now capture env = WTF::Ref<NapiEnv>(*env) instead of the raw napi_env pointer. This is byte-for-byte the same pattern already used by napi_add_finalizer (line 1167) and napi_create_external_buffer (line 2087) in the same file. WTF::Ref::operator->() keeps env->doFinalizer(...) working unchanged. The remaining ~200 lines are test-only: a new C addon target in binding.gyp, the addon source, and a worker-teardown test.
Security risks
None introduced. The change is strictly defensive — it extends the lifetime of a RefCounted object across a callback that dereferences it, closing a theoretical use-after-free window. It cannot make memory safety worse; the only behavioral delta is one extra refcount inc/dec per external-string creation. No new attack surface, no auth/crypto/permission code touched.
Level of scrutiny
The src/ change is mechanical pattern-matching to two existing siblings in the same file and is verifiable by inspection. The test additions follow established test/napi/napi-app/ conventions (mirrors isolate_finalizer_addon.c) and the Malloc=1/ASAN subprocess pattern from websocket-server-upgrade-reentrant.test.ts. Low scrutiny is appropriate for the production change; the test code has already absorbed four rounds of review feedback.
Other factors
I previously left four inline comments on this PR. All were either applied (stale collectContinuously comment dropped in fe0bf0a, combined-object assertion in c20da88, test.concurrent.each in 57562c7) or reasonably declined (the pre-existing copied out-param asymmetry is deferred to PR #32259 to avoid a merge conflict; the new addon doesn't read copied). All review threads are resolved, the bug-hunting system found nothing, and there are no CODEOWNERS for these paths. The most recent commit (3f94689) is a CI retrigger with no code changes.
…izers The ExternalStringImpl free callbacks installed by node_api_create_external_string_latin1 and _utf16 captured the raw napi_env pointer by value. NapiEnv is RefCounted and its primary owner is Zig::GlobalObject::m_napiEnvs, dropped in ~GlobalObject(). If the env is released before the external string's StringImpl is dereferenced, the finalizer calls env->doFinalizer() on freed memory. Capture WTF::Ref<NapiEnv>(*env) instead, matching the existing pattern in napi_add_finalizer and napi_create_external_buffer. Add an addon and worker-teardown test that exercises both finalizer paths through env->doFinalizer() with Malloc=1 so ASAN instruments the TZONE-allocated NapiEnv. Whether ~GlobalObject precedes the string finalizer is heap-layout dependent (MarkedSpace::prepareForConservativeScan sorts m_preciseAllocations by address each full GC); under current allocation patterns the GlobalObject sorts last so the UAF does not fire, but the test makes any future reordering observable.
The Windows skip rationale referenced BUN_JSC_collectContinuously, which this test never sets; drop the skip so non-Linux lanes still assert the finalizers run (Malloc=1 remains Linux-only). Also remove the explicit 60s per-test timeout per test/CLAUDE.md.
Asserting stderr is exactly empty under Malloc=1 + ASAN can flake on benign sanitizer output across compiler-rt versions. The combined object keeps the empty-stderr check (so an ASAN heap-use-after-free report still fails the test) while surfacing all three values in one failure diff.
3f94689 to
6c99c80
Compare
There was a problem hiding this comment.
LGTM — the two-line native change matches the established env = WTF::Ref<NapiEnv>(*env) capture pattern already used by napi_add_finalizer and napi_create_external_buffer in the same file, and all prior review feedback has been addressed.
Extended reasoning...
Overview
The PR changes two lambda captures in src/jsc/bindings/napi.cpp (node_api_create_external_string_latin1 and node_api_create_external_string_utf16) from capturing the raw napi_env pointer to capturing env = WTF::Ref<NapiEnv>(*env), so the finalizer lambda holds a strong reference to the RefCounted NapiEnv for its lifetime. It also adds a new C test addon (external_string_addon.c), a binding.gyp target, and a worker-teardown test (napi-external-string-env.test.ts).
I verified the native change against the current tree: the identical capture form [env = WTF::Ref<NapiEnv>(*env), ...] with env->doFinalizer(...) in the body already appears at napi.cpp:1185 (napi_add_finalizer) and napi.cpp:2105 (napi_create_external_buffer). This PR brings the two remaining raw-pointer-capturing finalizer lambdas in line with those siblings. WTF::Ref::operator->() makes env->doFinalizer(...) work unchanged, and the ref is released when the ExternalStringImpl's stored free callback is destroyed.
Security risks
None. The change strictly increases safety — holding a Ref where a raw pointer was previously captured cannot introduce a UAF and closes a theoretical one. No user-controlled input handling, auth, crypto, or permissions are touched.
Level of scrutiny
This is memory-safety C++ in the JSC/N-API bindings, which normally warrants careful review. However, the actual native diff is two tokens per line and is a mechanical pattern-match to two existing sibling call sites in the same file that were written for exactly this reason. The change is defensively correct regardless of whether the sweep-ordering window ever opens (as the PR description honestly explains it currently does not under observed heap layouts). There is no plausible way this capture change regresses behavior.
Other factors
I have reviewed this PR across four prior rounds; every comment (stale skip rationale, combined-object stderr assertion, test.concurrent.each, canBuildNodeAddons() gate) has been applied, and the one declined suggestion (pre-existing copied out-param asymmetry) is correctly deferred to the separate open PR for it. The bug-hunting system found nothing. The test follows harness conventions (tempDir, bunEnv, concurrent subprocess drain, combined-object assertion, describe.skipIf(!canBuildNodeAddons())), and the addon mirrors the sibling isolate_finalizer_addon.c shape. The remaining risk is CI-only (whether the new node-gyp target builds cleanly on all lanes), which BuildKite will surface.
|
CI status: the new test ( Remaining red is unrelated to this diff:
The src/ change here is two lambda-capture tokens in |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-09, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Summary
node_api_create_external_string_latin1andnode_api_create_external_string_utf16(src/jsc/bindings/napi.cpp) install anExternalStringImplfree callback that callsenv->doFinalizer(...). The lambda capturedenv(the rawnapi_envpointer) by value:NapiEnvisRefCounted; its primary owner isZig::GlobalObject::m_napiEnvs, dropped in~GlobalObject(). If the env's refcount reaches zero before the external string'sStringImplis dereferenced, the finalizer lambda callsdoFinalizer(which readsm_napiModule.nm_version,m_vm, etc.) on freed memory.The sibling finalizers in
napi_add_finalizer(line 1167) andnapi_create_external_buffer(line 2087) already captureWTF::Ref<NapiEnv>(*env)for exactly this reason. This change brings the two external-string paths in line.Why this can't be gate-proven
I tried to construct a deterministic fail-before and couldn't. The ordering between
~GlobalObject(frees theNapiEnv) and the string finalizer depends on JSC heap layout:WebWorker__teardownJSCVMrunscollectNow(Sync, Full)while the Rustshutdownframe still has the global on its stack, so conservative roots keep the global (and anything rooted on it) marked through that collection.~VM→Heap::lastChanceToFinalize()→MarkedSpace::lastChanceToFinalize(), which runsforEachDirectory(block-directory cells, including the external-stringJSStrings) before iteratingm_preciseAllocations(where theGlobalObject's lower-tier precise allocation lives).m_preciseAllocationsis sorted by pointer address before each full GC (MarkedSpace::prepareForConservativeScan()), so even holders likeDOMExceptionthat keep aWTF::Stringref to theExternalStringImpland are themselves lower-tier precise depend on where the allocator placed them relative to the global.I ran 30+ iterations of a worker fixture that stashes external strings in
DOMException/Event/Fileholders (which share theStringImplviatoWTFString), withMalloc=1+ ASAN +BUN_JSC_stealEmptyBlocksFromOtherAllocators=0, and theGlobalObject'sPreciseAllocationconsistently sorted last, so~NapiEnvran after every string finalizer. Forcing the opposite order would require instrumentingsrc/to perturb allocation addresses, which the gate strips.The fix is still correct regardless of ordering and matches the established pattern. Any future change to sweep order,
PreciseAllocationlayout, orNapiEnvownership that exposes the window will be caught by the new test, which routes theNapiEnvallocation through the system heap on the Linux ASAN lane.Test
test/napi/napi-external-string-env.test.ts+ a small C addon (test/napi/napi-app/external_string_addon.c):latin1andutf16), roots them onglobalThisplus aDOMExceptionholder each, then exits.env->doFinalizer().Malloc=1so the TZONE-allocatedNapiEnvis ASAN-visible.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi-external-string-env.test.ts