Copy the message string when constructing Error/AggregateError from a ZigString - #31451
Copy the message string when constructing Error/AggregateError from a ZigString#31451Jarred-Sumner wants to merge 2 commits into
Conversation
… ZigString Zig::getErrorInstance built the Error message with Zig::toString, which for an untagged ZigString wraps the caller's bytes via StringImpl::createWithoutCopying — no copy, no ownership, no liveness link. Several callers pass stack-local buffers (AsyncModule's resolve_error/download_error format their message into a local Vec), so the JSString backing error.message aliases freed memory by the time user code reads it. The TypeError/SyntaxError/RangeError siblings already use toStringCopy; do the same here and in JSC__JSGlobalObject__createAggregateError, whose message has the same lifetime requirement.
|
Updated 11:07 PM PT - May 27th, 2026
✅ @Jarred-Sumner, your commit ae94dddce664e8d2adda2892ddd53d522512cd7a passed in 🧪 To try this PR locally: bunx bun-pr 31451That installs a local version of the PR into your bun-31451 --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 (2)
WalkthroughBoth error-message construction sites now choose between adopting an external Zig string or copying non-external Zig buffers into a new JS string, avoiding JS strings that alias short-lived caller-owned memory. ChangesError Message String Lifetime Fixes
🚥 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}
Review feedback: switching getErrorInstance/createAggregateError to an unconditional toStringCopy dropped the external-tag (mark_global) ownership transfer, so callers that heap-allocate the message and rely on the JS string adopting and freeing it (VirtualMachine.rs's joined build-failure message for AggregateError) would leak it on every call. Guard like toIdentifier does: external-tagged messages keep toString (adopted as an ExternalStringImpl, freed via free_global_string); everything else — including the untagged stack-local buffers that motivated the change — is copied.
|
Good catch — fixed in the follow-up commit: both |
There was a problem hiding this comment.
LGTM — the follow-up commit applies the isTaggedExternalPtr ? toString : toStringCopy guard at both sites (same pattern as toIdentifier), so the external-tag ownership transfer is preserved and the original UAF on untagged stack buffers is fixed.
Extended reasoning...
Overview
Two one-line changes at the Rust↔C++ FFI boundary: getErrorInstance (helpers.h) and JSC__JSGlobalObject__createAggregateError (bindings.cpp) now build their message string via isTaggedExternalPtr(ptr) ? toString(...) : toStringCopy(...) instead of unconditionally calling toString. This copies untagged ZigString bytes (which previously produced a createWithoutCopying JSString aliasing caller-owned stack memory) while preserving the bit-62 external-tag adoption path that frees caller-transferred heap buffers via free_global_string.
Security risks
None introduced. The change removes a latent use-after-free read on error-message paths where callers pass stack-local buffers (e.g. AsyncModule's resolve/download error formatting). The external-tag branch is unchanged behavior versus the pre-PR baseline, so no new ownership hazard there.
Level of scrutiny
Moderate — this is memory-ownership code at an FFI boundary, but the final shape is a direct copy of the existing toIdentifier idiom three functions down in the same header, applied to error-path-only code. The first revision of this PR unconditionally used toStringCopy, which would have leaked the mark_global()'d AggregateError message from VirtualMachine.rs; I flagged that in the prior review and the author's follow-up commit (ae94ddd) applied the suggested guard verbatim. The current bug-hunting pass found no issues on the revised diff.
Other factors
- The sibling
getTypeErrorInstance/getSyntaxErrorInstance/getRangeErrorInstancehelpers already usetoStringCopy, so this bringsgetErrorInstancein line with established neighbors. - The added comments accurately document the tag semantics and reference the concrete callers that motivate each branch.
- Error-path only; the extra copy has no meaningful cost.
|
Looked at the
|
2 participants
Zig::getErrorInstancebuilds the Error message withZig::toString, which for an untaggedZigStringwraps the caller's bytes viaStringImpl::createWithoutCopying— no copy, no ownership, no liveness link. Several callers pass stack-local buffers (AsyncModule'sresolve_error/download_errorformat the message into a localVec<u8>that drops at function return), so the JSString backingerror.messagealiases freed memory by the time user code reads it — a long-standing latent use-after-free read on those error paths (it predates the Rust port; the Zig code freed the buffer with adeferthe same way).The
TypeError/SyntaxError/RangeErrorhelpers right next to it already usetoStringCopy. This switchesgetErrorInstanceandJSC__JSGlobalObject__createAggregateError(same lifetime requirement for the AggregateError message) to copy as well. Error paths only, so the extra copy is free in practice.Found while auditing string ownership at the FFI boundary for the Windows cross-compile work; verified the module-resolution error path still produces intact messages with a debug build.