Repository navigation
Fix segmentation fault during building stack traces string - #22902
Conversation
|
Updated 2:42 AM PT - Oct 3rd, 2025
❌ @Jarred-Sumner, your commit 8f3d7ea has 3 failures in
🧪 To try this PR locally: bunx bun-pr 22902That installs a local version of the PR into your bun-22902 --bun |
|
Note that in each of these three call sites in So I think somewhere we are incorrectly double decrementing it, and this PR Is likely a workaround but not the correct fix |
|
@Jarred-Sumner I know that, but don't we have to increment the ref count when we copy the frame as well as when we first set the name? |
|
This leaks the non-builtin function names. if (frame.function_name.tag == BunStringTag::WTFStringImpl) {
printf("[%zu] function name: %s\n", frame.function_name.impl.wtf->refCount(), frame.function_name.toWTFString().utf8().data());
}This branch: Main: [20001] function name: moduleEvaluation
[20002] function name: moduleEvaluation
[9] function name: loadAndEvaluateModule
[6] function name: Hello
[20003] function name: moduleEvaluation
[20004] function name: moduleEvaluation
[9] function name: loadAndEvaluateModule
[6] function name: Hello
[20005] function name: moduleEvaluation
[20006] function name: moduleEvaluation
[9] function name: loadAndEvaluateModuleScript: function Hello() {
throw new Error("Hello");
}
try {
Hello();
} catch (e) {
Bun.gc(true);
for (let i = 0; i < 10000; i++) {
reportError(e);
}
}Run: bun-debug error.js 2> /dev/null > out.txt |
effb642 to
e1d81c1
Compare
e1d81c1 to
679524b
Compare
WalkthroughRemoved a Log method and a local alias in Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/VirtualMachine.zig(1 hunks)src/logger.zig(0 hunks)
💤 Files with no reviewable changes (1)
- src/logger.zig
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/VirtualMachine.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/VirtualMachine.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zig
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-08-30T00:07:54.573Z
Learning: Applies to src/**/*.zig : Implement debug logs in Zig using `const log = bun.Output.scoped(.${SCOPE}, false);` and invoking `log("...", .{})`
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: For platform-specific changes, run bun run zig:check-all to compile on all platforms
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/bun.js/VirtualMachine.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/VirtualMachine.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/VirtualMachine.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: For platform-specific changes, run bun run zig:check-all to compile on all platforms
Applied to files:
src/bun.js/VirtualMachine.zig
📚 Learning: 2025-09-02T18:29:58.304Z
Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/memory.zig:37-43
Timestamp: 2025-09-02T18:29:58.304Z
Learning: In bun's `src/memory.zig`, the `initDefault` function intentionally calls `T.init()` without parameter validation to cause compile-time errors when `init` requires arguments or returns an error union. This design forces developers to add explicit `initDefault` methods for complex types rather than silently falling back to `{}`. The `{}` fallback is only intended for types that have no `init` function at all.
Applied to files:
src/bun.js/VirtualMachine.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/VirtualMachine.zig
| @memset(frames[j..], jsc.ZigStackFrame.Zero); | ||
| exception.stack.frames_len = @as(u8, @truncate(j)); | ||
| frames.len = j; |
There was a problem hiding this comment.
Avoid zeroing ZigStackFrame without releasing its string refs.
ZigStackFrame owns bun.String fields (function_name, source_url, etc.) whose refcounts are maintained manually. Blasting the trailing slice with @memset(..., .Zero) drops those pointers without calling .deref(), so the underlying WTF::StringImpl instances never release their refs and the counts climb each time we build a stack trace. Please walk the discarded frames and deref their strings (or call the appropriate deinit helper) before zeroing them out.
🤖 Prompt for AI Agents
In src/bun.js/VirtualMachine.zig around lines 2693-2695, before zeroing the
trailing slice (frames[j..]) and shrinking the lengths, iterate over the frames
that are being discarded and release their owned bun.String fields (e.g.,
function_name, source_url, module_name, etc.) by calling the appropriate
deinit/deref helper for each field or a ZigStackFrame.deinit(&frame) helper,
then perform the @memset and update exception.stack.frames_len and frames.len;
this ensures the underlying WTF::StringImpl refs are decremented before the
pointers are overwritten.
There was a problem hiding this comment.
The frames are copied WITHOUT incrementing refcounts during compaction:
frames[j] = frame; // Simple assignment, no ref() call
After compaction, frames[0..j] and frames[j..] may contain duplicate
pointers to the same StringImpl. Calling deref() on frames[j..] would
incorrectly decrement refcounts for StringImpls still referenced by
frames[0..j], causing use-after-free.
The memset(.Zero) is correct - it clears duplicate pointers without
touching refcounts, preventing double-deref when the Holder is reused.
What do you think?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| j += 1; | ||
| } | ||
|
|
||
| @memset(frames[j..], jsc.ZigStackFrame.Zero); |
There was a problem hiding this comment.
why is this the correct fix? remember that zig doesn't have automatic constructor/destructors, so unless we are calling deref on frames outside the bounds of .len, this change should not do anything. and if that is what's happening, then it should be fixed at those call sites and not at this call site.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
I'm still confused why this would fix it
6840ea5 to
4a0ae70
Compare
4a0ae70 to
b858187
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/bindings/bindings.cpp (1)
4798-4804: Guardjsc_stack_frame_indexbefore use in release buildsAt the moment we only
ASSERTthatjsc_stack_frame_indexis within bounds before indexingframes[...]. ThoseASSERTs disappear in release builds, so if this value ever remains at the sentinel-1(or otherwise drifts out of range) we’ll index with an invalid size_t and immediately crash—exactly the class of failure this work is trying to eradicate. Let’s keep the assertion for debug, but add a runtime guard (or upgrade it to aRELEASE_ASSERT) so production builds fail safe.} else if (flags == PopulateStackTraceFlags::OnlySourceLines) { for (uint8_t i = 0; i < trace.frames_len; i++) { ZigStackFrame& frame = trace.frames_ptr[i]; - // A call with flags set to OnlySourceLines always follows a call with flags set to OnlyPosition, - // so jsc_stack_frame_index is always a valid value here. - ASSERT(frame.jsc_stack_frame_index >= 0); - ASSERT(static_cast<size_t>(frame.jsc_stack_frame_index) < frames.size()); - populateStackFrame(vm, trace, frames[frame.jsc_stack_frame_index], frame, i == 0, &trace.referenced_source_provider, globalObject, flags); + // A call with flags set to OnlySourceLines always follows a call with flags set to OnlyPosition, + // but guard in release builds as well. + ASSERT(frame.jsc_stack_frame_index >= 0); + ASSERT(static_cast<size_t>(frame.jsc_stack_frame_index) < frames.size()); + int32_t stackFrameIndex = frame.jsc_stack_frame_index; + if (stackFrameIndex < 0 || static_cast<size_t>(stackFrameIndex) >= frames.size()) + continue; + populateStackFrame(vm, trace, frames[stackFrameIndex], frame, i == 0, &trace.referenced_source_provider, globalObject, flags); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpp(1 hunks)src/bun.js/bindings/headers-handwritten.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C/C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/bindings.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Define class properties using HashTableValue arrays in JSC bindings
Add iso subspaces for classes with C++ fields in JSC bindings
Cache structures in ZigGlobalObject for JSC-bound classes
Files:
src/bun.js/bindings/bindings.cpp
🧠 Learnings (3)
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/headers-handwritten.h
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/headers-handwritten.h
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes
Applied to files:
src/bun.js/bindings/headers-handwritten.h
What does this PR do?
Bun sometimes crashes with a segmentation fault while generating stack traces.
the following might be happening in
remapZigException:frames_len(e.g., frames_len = 5)bun/src/bun.js/bindings/bindings.cpp
Line 4793 in 613aea1
frames_len(e.g., frames_len = 3)bun/src/bun.js/VirtualMachine.zig
Lines 2686 to 2704 in 613aea1
frames_len(e.g., frames_len = 5)bun/src/bun.js/bindings/bindings.cpp
Line 4793 in 613aea1
When deinit is executed on these frames, the ref count is excessively decremented (for frame4 and frame5), resulting in a UAF.
How did you verify your code works?
WIP. I'm working on creating minimal reproduction code.
However, I've confirmed that
twenty-servertests passes with this PR.