Repository navigation
Conversation
node_api_create_external_string_utf16 never wrote its *copied out-parameter, leaving the caller's bool uninitialized. If the caller's stack value happened to be nonzero, the addon would assume the buffer was copied and free it while the ExternalStringImpl still referenced it, leading to a use-after-free on string read/finalize. The latin1 variant already sets *copied = false; mirror that here. Bun always uses ExternalStringImpl without copying, so copied is always false.
|
Updated 11:57 PM PT - Jun 14th, 2026
❌ @robobun, your commit d2c30cf has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32259That installs a local version of the PR into your bun-32259 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesExternal String
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward fix mirroring the latin1 path; the inline nit about the test's if (copied) free(buf) is non-blocking dead code.
Extended reasoning...
Overview
This PR adds 4 lines to node_api_create_external_string_utf16 in src/jsc/bindings/napi.cpp to write *copied = false before returning, fixing a bug where the out-parameter was left uninitialized. The change is an exact copy of lines 1471-1473 in the adjacent node_api_create_external_string_latin1 implementation. It also adds a regression test in standalone_tests.cpp and wires it into napi.test.ts via checkSameOutput.
Security risks
None. The change writes a single false to a caller-supplied bool* after a null check (if (copied)). There is no parsing, allocation, auth, or external input involved. If anything, this removes a memory-safety hazard for native addons that follow the Node-API contract and would otherwise free a still-referenced buffer based on an uninitialized read.
Level of scrutiny
Low. The production change is trivially verifiable by comparing it line-for-line against the latin1 variant directly above it in the same file — they are now symmetric. Bun always wraps external strings via ExternalStringImpl (never copies), so false is unconditionally correct. The Node-API docs and Node's own behavior (verified in the PR description) confirm this is the expected semantics.
Other factors
- No CODEOWNERS entry covers
napi.cppor the test files. - The test follows the established
checkSameOutputpattern used throughoutnapi.test.ts, comparing Bun's output against Node v24.3.0. - The one bug-hunter finding is a nit about dead
if (copied) free(buf)branches in the test that would double-free if a runtime ever returnedcopied=true. Since both Bun and Node returnfalsehere (and the test asserts on that), the branch never executes. It's a minor test-hygiene issue, not a correctness blocker, and is already posted as an inline comment for the author to optionally clean up.
The finalizer owns cleanup regardless of copied's value; calling free(buf) again when copied=true would be a double-free since the finalizer has already run synchronously.
There was a problem hiding this comment.
LGTM — the nit on the test's free blocks was addressed in d2c30cf.
Extended reasoning...
Overview
This PR adds a 3-line fix to node_api_create_external_string_utf16 in src/jsc/bindings/napi.cpp so it writes *copied = false when the out-parameter pointer is non-null. The latin1 variant immediately above it already does this; the utf16 variant was simply missing the assignment, leaving the caller's bool uninitialized. A new standalone C++ test (test_node_api_create_external_string_copied) and a checkSameOutput assertion in napi.test.ts verify both variants report copied=false.
Security risks
None introduced. The fix actually closes a potential use-after-free vector: per the Node-API contract, an addon that sees copied=true may free the original buffer, and an uninitialized nonzero value could trigger that while ExternalStringImpl still holds the pointer. Writing false unconditionally (Bun never copies) is the correct and safe behavior.
Level of scrutiny
Low. The production change is 3 lines that copy the exact pattern from the adjacent latin1 function. The test follows the established conventions in standalone_tests.cpp (heap-allocated buffer, NODE_API_CALL, printf, ok(env), REGISTER_FUNCTION) and uses checkSameOutput to cross-check against Node. No CODEOWNERS cover these files.
Other factors
I previously left a nit about if (copied) { free(buf); } blocks in the test that would double-free under a hypothetical copied=true runtime. The author removed both blocks in d2c30cf and added a clarifying comment that the finalizer owns cleanup in both cases — that thread is resolved and the current diff reflects the fix. The bug-hunting system found no issues on this revision.
There was a problem hiding this comment.
LGTM — the nit from the prior pass was addressed in d2c30cf.
Extended reasoning...
Overview
This PR adds a 4-line fix to node_api_create_external_string_utf16 in src/jsc/bindings/napi.cpp: it now writes *copied = false to the optional out-parameter, exactly mirroring what node_api_create_external_string_latin1 already does at lines 1471-1473. A new regression test in standalone_tests.cpp initializes copied = true for both variants and verifies via checkSameOutput that Bun matches Node's output (copied=false).
Security risks
None. This writes a constant false through a caller-supplied pointer that is null-checked first — identical to the existing latin1 path. The fix actually closes a latent UAF risk where addons reading an uninitialized nonzero copied might prematurely free a buffer the ExternalStringImpl still references.
Level of scrutiny
Low. The production change is mechanical and copy-pasted from the adjacent function; correctness is verified by direct comparison against the latin1 variant in the same file and against Node's behavior via the new checkSameOutput test. The test code follows established patterns in standalone_tests.cpp.
Other factors
My only prior comment (a nit about dead if (copied) free(buf) blocks that would double-free under a hypothetical copied=true runtime) was addressed in d2c30cf — those blocks are gone and the test now correctly lets the finalizer own cleanup unconditionally. No bugs were found by the bug-hunting system on this revision, and CodeRabbit had no actionable comments.
|
The diff is ready. Remaining CI failures across builds #62342 and #62355 are all GitHub API 5xx/504 errors unrelated to this change:
204 test-bun shards passed including the new The change itself is 4 lines in |
|
Stale PR review: closing. This PR has had no human activity since 2026-06-15 and conflicts with main. Its fix is already on main: #34146 (commit 8545a19, merged 2026-07-14) folded both external string variants into a shared Reopen if this evidence is wrong. |
1 participant
Problem
node_api_create_external_string_utf16never writes its*copiedout-parameter, leaving the caller's bool uninitialized. The latin1 variant correctly sets*copied = false.Per the Node-API docs,
copiedtells the addon whether the runtime copied the string. If it was copied, the finalizer has already been invoked and the addon should free the original buffer. If the uninitialized value happens to be nonzero, an addon following this contract frees a buffer that theExternalStringImplstill references, leading to a use-after-free on string read or finalize.Fix
Add
if (copied) { *copied = false; }before the return, mirroring the latin1 path. Bun always usesExternalStringImpl(no copy), socopiedis always false.Verification
New test
test_node_api_create_external_string_copiedinitializescopied = true, calls both the latin1 and utf16 variants, and prints the resulting value. The test compares Bun's output against Node's viacheckSameOutput.