Fix potential memory leak when calling .text() on shell command results #23602
Conversation
The memory leak occurred when `$\`cmd\``.text()` was called repeatedly. The `ioToJSValue` function was creating a Buffer by wrapping bytes without properly setting up a finalizer to free the memory when the Buffer was garbage collected. The fix: 1. Added `JSBuffer__fromDefaultAllocator` C++ function that creates a Buffer with proper memory management using JSC's ArrayBuffer finalizer callback 2. Added corresponding Zig binding `createBufferFromDefaultAllocator` 3. Updated `ioToJSValue` to use the new function instead of manually constructing a MarkedArrayBuffer on the stack This ensures that when the JavaScript Buffer is garbage collected, the underlying memory allocated by bun.default_allocator is properly freed via mimalloc's `mi_free`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Updated 11:29 AM PT - Oct 13th, 2025
✅ Your commit a6d7b07 has passed in 🧪 To try this PR locally: bunx bun-pr 23602That installs a local version of the PR into your bun-23602 --bun |
WalkthroughAdds a C++ extern that creates a JS Uint8Array/ArrayBuffer from a raw pointer/length using mimalloc free, exposes a Zig extern and public wrapper to call it, and updates the shell interpreter to use that wrapper for buffer creation. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.{cpp,h}📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
**/*.cpp📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/bun.js/bindings/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-30T00:11:00.890ZApplied to files:
🧬 Code graph analysis (1)src/bun.js/bindings/JSBuffer.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (3)
src/bun.js/bindings/JSBuffer.cpp(2 hunks)src/bun.js/bindings/JSValue.zig(1 hunks)src/shell/interpreter.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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
Files:
src/bun.js/bindings/JSValue.zigsrc/shell/interpreter.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/bindings/JSValue.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/bindings/JSValue.zigsrc/shell/interpreter.zig
**/*.{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/JSBuffer.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/JSBuffer.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/JSBuffer.cpp
🧠 Learnings (6)
📓 Common learnings
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 **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
📚 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 **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/JSValue.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 : 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/JSValue.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/bindings/JSValue.zigsrc/shell/interpreter.zig
📚 Learning: 2025-09-03T05:09:24.272Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.
Applied to files:
src/bun.js/bindings/JSValue.zig
📚 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 **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/bun.js/bindings/JSBuffer.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/JSBuffer.cpp (1)
src/bun.js/bindings/node/crypto/CryptoUtil.cpp (2)
encode(67-91)encode(67-67)
🔇 Additional comments (2)
src/shell/interpreter.zig (1)
1108-1114: Accept const slices in createBufferFromDefaultAllocator and verify allocator contract
- Change
createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []u8)tobytes: []const u8and perform@constCastonly at the FFI boundary.- In
ioToJSValue(src/shell/interpreter.zig:1108–1114), add a debug-only assertion to ensure the buffer was allocated withbun.default_allocator.Verify that all
ByteListinstances (e.g.,_buffered_stdout/_buffered_stderr) are allocated viabun.default_allocatorto guarantee correct finalizer behavior.src/bun.js/bindings/JSBuffer.cpp (1)
14-14: LGTM!The mimalloc.h include is correctly placed and necessary to support the
mi_freecall in the newJSBuffer__fromDefaultAllocatorfunction.
- Add exception handling to JSBuffer__fromDefaultAllocator - Improve API to accept []const u8 instead of []u8 - Remove @constcast from call site - Add SAFETY documentation - Move function to correct namespace (Bun::)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (3)
src/bun.js/bindings/JSBuffer.cpp(2 hunks)src/bun.js/bindings/JSValue.zig(1 hunks)src/shell/interpreter.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/JSBuffer.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/JSBuffer.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/JSBuffer.cpp
**/*.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
Files:
src/bun.js/bindings/JSValue.zigsrc/shell/interpreter.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/bindings/JSValue.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/bindings/JSValue.zigsrc/shell/interpreter.zig
🧠 Learnings (5)
📚 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 **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/bun.js/bindings/JSBuffer.cpp
📚 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 **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/JSValue.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 : 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/JSValue.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/bindings/JSValue.zig
📚 Learning: 2025-09-03T05:09:24.272Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.
Applied to files:
src/bun.js/bindings/JSValue.zig
🧬 Code graph analysis (1)
src/bun.js/bindings/JSBuffer.cpp (2)
src/bun.js/bindings/webcore/SerializedScriptValue.cpp (18)
lexicalGlobalObject(879-914)lexicalGlobalObject(879-895)lexicalGlobalObject(2904-2948)lexicalGlobalObject(2904-2923)lexicalGlobalObject(2970-2975)lexicalGlobalObject(2970-2970)buffer(2835-2856)buffer(2835-2835)buffer(4533-4561)arrayBuffer(1278-1289)arrayBuffer(1278-1278)arrayBuffer(3564-3569)arrayBuffer(3564-3564)arrayBuffer(3571-3588)arrayBuffer(3571-3571)create(5810-5818)create(5810-5810)WTFMove(2977-2977)src/bun.js/bindings/node/crypto/CryptoUtil.cpp (2)
encode(67-91)encode(67-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
src/bun.js/bindings/JSBuffer.cpp
Outdated
| #include "JavaScriptCore/JSGlobalObject.h" | ||
| #include "BufferEncodingType.h" | ||
| #include "JavaScriptCore/JSCJSValue.h" | ||
| #include "mimalloc.h" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify mimalloc header availability on all targets (musl builds failing)
Using "mimalloc.h" may fail to resolve on some builders. Prefer angle brackets and/or an existing project header wrapper, or add a feature guard.
Run to check existing usage patterns and availability:
🏁 Script executed:
#!/bin/bash
# Inspect existing mimalloc includes and references to mi_free
rg -n -C2 --glob '!**/node_modules/**' -e '\bmi_free\b' -e 'mimalloc\.h' -e 'mimalloc-new-delete\.h'
# Confirm the header path shipped in the repo or via toolchain
fd -a 'mimalloc*.h'Length of output: 11895
Guard mimalloc include and switch to angle brackets
[js/bindings/JSBuffer.cpp:14]
Replace
#include "mimalloc.h"with
#if USE(MIMALLOC)
# include <mimalloc.h>
#endifEnsures the system header is used and avoids build failures when mimalloc isn’t available.
🤖 Prompt for AI Agents
In src/bun.js/bindings/JSBuffer.cpp around line 14, replace the raw quoted
include of mimalloc with a guarded system-include: wrap the include in a
preprocessor check for USE(MIMALLOC) and use angle brackets for the header so
the file is only included when mimalloc is enabled and the system header search
path is used; update the surrounding preprocessor directives accordingly so
builds that don't define USE(MIMALLOC) skip the include.
| extern fn JSBuffer__fromDefaultAllocator(*JSGlobalObject, [*]u8, usize) JSValue; | ||
| /// Creates a Buffer from bytes allocated with bun.default_allocator. | ||
| /// Takes ownership of the bytes and will free them when the Buffer is garbage collected. | ||
| /// SAFETY: The bytes pointer must have been allocated with bun.default_allocator. | ||
| /// The finalizer will use mi_free, which will cause undefined behavior with other allocators. | ||
| /// The caller must not mutate or free the bytes after calling this function. | ||
| pub fn createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []const u8) JSValue { | ||
| jsc.markBinding(@src()); | ||
| return JSBuffer__fromDefaultAllocator(globalObject, @constCast(bytes.ptr), bytes.len); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Previous review feedback successfully addressed.
The implementation correctly incorporates all suggestions from the previous review:
- ✓ Accepts
[]const u8for natural ownership transfer semantics - ✓ Moves
@constCastto the FFI boundary - ✓ Comprehensive SAFETY documentation about allocator requirements
Optional improvement: Consider error handling for consistency.
While the current implementation is correct, consider wrapping the call with fromJSHostCall for consistency with createBufferFromLength (line 538-541), which handles potential JS exceptions during buffer creation:
-pub fn createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []const u8) JSValue {
+pub fn createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []const u8) bun.JSError!JSValue {
jsc.markBinding(@src());
- return JSBuffer__fromDefaultAllocator(globalObject, @constCast(bytes.ptr), bytes.len);
+ return fromJSHostCall(globalObject, @src(), JSBuffer__fromDefaultAllocator, .{ globalObject, @constCast(bytes.ptr), bytes.len });
}This would provide more defensive error handling in case the underlying JSC buffer allocation fails (e.g., OOM). However, this is optional since createBuffer (line 572) follows a similar pattern without error handling.
Based on coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extern fn JSBuffer__fromDefaultAllocator(*JSGlobalObject, [*]u8, usize) JSValue; | |
| /// Creates a Buffer from bytes allocated with bun.default_allocator. | |
| /// Takes ownership of the bytes and will free them when the Buffer is garbage collected. | |
| /// SAFETY: The bytes pointer must have been allocated with bun.default_allocator. | |
| /// The finalizer will use mi_free, which will cause undefined behavior with other allocators. | |
| /// The caller must not mutate or free the bytes after calling this function. | |
| pub fn createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []const u8) JSValue { | |
| jsc.markBinding(@src()); | |
| return JSBuffer__fromDefaultAllocator(globalObject, @constCast(bytes.ptr), bytes.len); | |
| } | |
| extern fn JSBuffer__fromDefaultAllocator(*JSGlobalObject, [*]u8, usize) JSValue; | |
| /// Creates a Buffer from bytes allocated with bun.default_allocator. | |
| /// Takes ownership of the bytes and will free them when the Buffer is garbage collected. | |
| /// SAFETY: The bytes pointer must have been allocated with bun.default_allocator. | |
| /// The finalizer will use mi_free, which will cause undefined behavior with other allocators. | |
| /// The caller must not mutate or free the bytes after calling this function. | |
| - pub fn createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []const u8) JSValue { | |
| - jsc.markBinding(@src()); | |
| - return JSBuffer__fromDefaultAllocator(globalObject, @constCast(bytes.ptr), bytes.len); | |
| pub fn createBufferFromDefaultAllocator(globalObject: *JSGlobalObject, bytes: []const u8) bun.JSError!JSValue { | |
| jsc.markBinding(@src()); | |
| return fromJSHostCall( | |
| globalObject, | |
| @src(), | |
| JSBuffer__fromDefaultAllocator, | |
| .{ globalObject, @constCast(bytes.ptr), bytes.len }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/bindings/JSValue.zig around lines 592 to 601, the
createBufferFromDefaultAllocator call is currently a direct FFI call and lacks
the defensive JS exception handling used by createBufferFromLength; wrap the
JSBuffer__fromDefaultAllocator invocation with fromJSHostCall (or the project’s
equivalent) so any JS exceptions/OOM from the underlying JSC allocation are
captured and propagated consistently; keep @constCast at the FFI boundary and
preserve the SAFETY docstring while adapting the return path to return the
fromJSHostCall result.
| // Use createBufferFromDefaultAllocator which properly sets up a finalizer | ||
| // to free the memory when the Buffer is garbage collected | ||
| return JSValue.createBufferFromDefaultAllocator(globalThis, bytelist.slice()); | ||
| } |
There was a problem hiding this comment.
Zero-length path can leak the backing store
If bytelist.len == 0 but the ByteList had allocated capacity, setting buf.* = .{} and returning an empty Buffer leaks the allocation (the C++ helper only frees when length > 0). Free either in Zig for zero-length, or update the C++ helper to free ptr when length == 0.
🤖 Prompt for AI Agents
In src/shell/interpreter.zig around lines 1111-1114, when bytelist.len == 0 we
currently return an empty JS Buffer via createBufferFromDefaultAllocator but if
the ByteList had previously allocated capacity that backing store is not freed
by the C++ helper for length==0 — free the backing store before returning an
empty Buffer: detect bytelist.len == 0 and if bytelist.capacity > 0 explicitly
release/free the underlying allocation from Zig (using the same allocator that
created it) and reset the ByteList pointers/capacity, then return the empty
Buffer (or alternatively call/update the C++ helper to free ptr when length ==
0). Ensure the free is performed only once and the ByteList state is zeroed so
no leak occurs.
- Use mimalloc unconditionally like Uint8Array.cpp - Free ptr in zero-length branch to avoid leaks - Add assertions for non-null ptr when length > 0 - Add comment explaining ownership transfer
- Use angle brackets for mimalloc system header include - Guard mi_free calls with #if USE(MIMALLOC) / #else std::free - Ensures compatibility with builds that don't have mimalloc enabled - Follows the pattern used in MimallocWTFMalloc.h
1 participant
Summary
This is a speculative fix for what appears to be a memory management issue in the shell interpreter. I haven't actually confirmed this leak exists with profiling tools, so this might be AI slop.
The Theory
When
$\cmd`.text()is called repeatedly, it appears that memory could leak because theioToJSValuefunction insrc/shell/interpreter.zigwas creating aMarkedArrayBufferon the stack, then immediately converting it to a Node Buffer. The stack-allocated struct goes out of scope, potentially losing the allocator information needed to free the memory when the Buffer is garbage collected.Changes
JSBuffer__fromDefaultAllocator- C++ function that creates a Buffer with a proper JSC finalizer callback that callsmi_freecreateBufferFromDefaultAllocator- Zig binding for the aboveioToJSValue- Now uses the new function instead of the potentially buggy stack-allocated approachWhy This Might Be Wrong
Testing
If someone can actually reproduce the reported leak with profiling tools, then this fix might be useful. Otherwise, feel free to close as AI slop.
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]