Preserve part boundaries when streaming multi-part Blobs#28765
Preserve part boundaries when streaming multi-part Blobs#28765
Conversation
|
Updated 8:40 PM PT - Apr 1st, 2026
❌ @robobun, your commit a02d6b0 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 28765That installs a local version of the PR into your bun-28765 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRecord per-part sizes when joining Blob parts, persist them in byte-backed Blob storage, make the Blob streamer emit chunks bounded by those part boundaries, and add regression tests verifying Blob.stream() preserves original part segmentation. Changes
🚥 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.
No bugs found, but this touches core blob streaming logic across multiple files with non-trivial memory ownership semantics — worth a human look before merging.
Extended reasoning...
Overview
This PR modifies 4 source files and adds a regression test. Changes span StringJoiner (new doneWithPartSizes API), Blob.Store.Bytes (new part_sizes/part_count fields with owned memory), ByteBlobLoader (new part-tracking state in setup/onStart/onPull/drain), and Blob.zig (wiring via setPartSizes). The regression test file covers 6 scenarios.
Security Risks
No security concerns. The change is purely about streaming chunk boundaries for in-memory blob data. No auth, crypto, or permission-sensitive paths are touched.
Level of Scrutiny
Moderately high. The fix is well-scoped and only activates when: no user-specified chunkSize, offset == 0, and part_count > 1. However, it introduces heap-allocated metadata (part_sizes) with ownership transferred from StringJoiner through setPartSizes into Bytes.deinit, which is a non-trivial ownership chain. The onPull logic assumes buffer.len >= max_part (guaranteed by onStart advertising max_part as chunk_size), but a caller using the reader directly with a smaller buffer would silently advance current_part without consuming the full part — a subtle invariant worth confirming is always upheld by the stream infrastructure.
Other Factors
The fix is well-documented with clear before/after semantics, and the test coverage is thorough. The PR description mentions passing the relevant stream regression tests. The u52 type used in StringJoiner.doneWithPartSizes correctly matches Blob.SizeType = u52. No existing approvals or reviewer comments to consider.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/blob/Store.zig`:
- Around line 481-484: The blob's auxiliary allocation part_sizes is not
included in memory accounting, so update both Store.memoryCost() and
Blob.calculateEstimatedByteSize() to account for it: when part_sizes != null add
part_count * `@sizeOf`(SizeType) (or equivalent byte size calculation using
SizeType) to the reported external/heap bytes; reference the fields part_sizes,
part_count and the functions Store.memoryCost() and
Blob.calculateEstimatedByteSize() and ensure the added bytes reflect that
part_sizes is an allocator-owned heap allocation.
In `@src/bun.js/webcore/ByteBlobLoader.zig`:
- Around line 58-61: The multipart guard currently uses blobe.offset == 0 which
incorrectly treats sliced(0, end) as a full blob; instead require that the blob
actually covers the entire underlying byte store by comparing the blob's range
to the store's total byte length. Update the has_parts condition (alongside
has_user_chunk_size) to check that blobe.offset == 0 AND blobe.length (or the
blob's end) equals the store's total bytes (e.g. store.data.bytes.total_size or
equivalent), so only an original/full blob with store.data == .bytes and
part_sizes != null and part_count > 1 enables multipart mode.
In `@test/regression/issue/28762.test.ts`:
- Around line 66-71: The test uses blob.arrayBuffer(), which bypasses
ByteBlobLoader; change it to consume Blob.stream() instead: call blob.stream(),
read all ReadableStreamDefaultReader chunks (via getReader()/read loop or by
piping to a collector), concatenate the Uint8Array chunks into one Uint8Array,
and assert that the concatenated result equals [1,2,3,4,5,6]; update the test
"Blob.stream() total content is correct after chunking" to use Blob.stream() and
the reader loop so the multipart chunking path (ByteBlobLoader) is exercised.
🪄 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: 5d7d7827-32ff-4489-9a90-1918432605bb
📒 Files selected for processing (5)
src/bun.js/webcore/Blob.zigsrc/bun.js/webcore/ByteBlobLoader.zigsrc/bun.js/webcore/blob/Store.zigsrc/string/StringJoiner.zigtest/regression/issue/28762.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/Blob.zig`:
- Around line 3405-3414: The function setPartSizes currently returns early when
store is null and thus drops the allocator-owned part_sizes slice, leaking
memory for multipart empty inputs; modify setPartSizes to free the allocated
part_sizes (use bun.default_allocator.free or equivalent) before any early
return when store is null or when sizes.len <= 1 so the slice backing memory is
reclaimed, and only after successful assignment to s.data.bytes.part_sizes /
s.data.bytes.part_count avoid freeing; reference the function setPartSizes, the
parameter part_sizes, the local sizes, and the fields s.data.bytes.part_sizes
and s.data.bytes.part_count to locate where to insert the free calls.
In `@src/bun.js/webcore/ByteBlobLoader.zig`:
- Around line 74-77: The code initializes and advances across parts using
current_part and current_part_remain but doesn't skip zero-length parts, causing
later non-empty parts to be merged; update initialization and all advance/read
paths that set or consume current_part_remain (including the initial set and the
advance logic used in the read functions around the ranges noted) to
loop/advance while current_part < part_count and part_sizes.?[current_part] ==
0, incrementing current_part and setting current_part_remain from the next
non-zero part_size (or 0 if none); ensure any place that falls back to
this.remain first normalizes by skipping zero-length parts so chunk boundaries
after empty parts are preserved.
🪄 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: 07c4c304-8a65-4883-ae1b-ee23e6a6879a
📒 Files selected for processing (4)
src/bun.js/webcore/Blob.zigsrc/bun.js/webcore/ByteBlobLoader.zigsrc/bun.js/webcore/blob/Store.zigtest/regression/issue/28762.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/ByteBlobLoader.zig`:
- Around line 60-63: The code currently computes has_user_chunk_size from
user_chunk_size > 0 which loses the signal that a caller explicitly passed a
non-positive chunk size; add a separate boolean parameter (e.g.
has_explicit_chunk_size or user_provided_chunk_size) threaded from the
Blob.getStream() call site and use that boolean instead of user_chunk_size > 0
when computing has_user_chunk_size; update the has_parts expression (used with
blobe.store, blobe.offset, blobe.size, store.data.bytes.part_sizes,
store.data.bytes.part_count) to check that the explicit-flag is false (i.e. only
enable part-boundary mode when no explicit chunk size was passed), and update
all call sites to pass the new boolean.
In `@src/string/StringJoiner.zig`:
- Around line 90-112: The new allocation of part_sizes in doneWithPartSizes can
fail and currently returns after freeing slice but leaves this' linked-list and
cloned buffers live; update doneWithPartSizes so that if allocating part_sizes
fails you clean up all resources: free the slice, deinit the StringJoiner (call
this.deinit() or the appropriate cleanup routine to free nodes and cloned
buffers) and ensure any node-level buffers are freed before returning the error;
also switch the allocation to use the project allocator helper (use the
allocator from src/allocators rather than bun.default_allocator) and use
errdefer/defer to centralize cleanup so failures in the part_sizes allocation
always run the same cleanup path.
🪄 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: 09d7fab9-a5b9-4c1c-92d6-480b0f40c7b1
📒 Files selected for processing (3)
src/bun.js/webcore/Blob.zigsrc/bun.js/webcore/ByteBlobLoader.zigsrc/string/StringJoiner.zig
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/string/StringJoiner.zig`:
- Around line 84-96: The current doneWithPartSizes implementation calls errdefer
this.deinit() unconditionally which causes StringJoiner to be invalidated on any
error (breaking done()'s previous contract); update the error handling so that
the node-list is only freed when the new part_sizes allocation actually succeeds
(i.e., scope the deinit to the branch that allocates/returns part_sizes) or,
alternatively, before propagating an error reset this.head/this.tail/this.len to
their empty-state so callers using errdefer joiner.deinit() won't double-free;
adjust the logic around doneWithPartSizes, done, deinit, and the part_sizes
allocation to ensure only the part_sizes OOM path consumes the joiner state.
🪄 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: 47896103-bfd2-4594-885b-f5d704f06163
📒 Files selected for processing (1)
src/string/StringJoiner.zig
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/string/StringJoiner.zig`:
- Around line 117-133: The join routine frees every node but leaves the
StringJoiner state pointing to freed nodes; after the node-consumption loop (the
block that iterates current/node, calls prev.deinit(this.allocator), and before
the final bun.assert and return), reset the joiner internal state to the same
empty-state values used by deinit() — set head and tail to null and len to 0 (or
call the same helper used by deinit() if present) so future deinit() or reuse
won't touch freed nodes; update the function that returns .{ slice, part_sizes }
(the join method in StringJoiner) to perform this reset before returning.
🪄 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: 5aafa767-5b3e-45d6-a6df-f0109bb58810
📒 Files selected for processing (1)
src/string/StringJoiner.zig
There was a problem hiding this comment.
LGTM — all previously flagged issues have been resolved.
Extended reasoning...
Overview
This PR fixes Blob.stream() to preserve original part boundaries when streaming multi-part Blobs. Changes span StringJoiner.zig (new doneWithPartSizes()), blob/Store.zig (part_sizes/part_count fields on Bytes), ByteBlobLoader.zig (per-part chunk delivery), Blob.zig (wiring), and a regression test file.
Security risks
None. This is a pure behavior fix in the streaming path with no auth, crypto, or permission code touched.
Level of scrutiny
The change is non-trivial (Zig memory management across multiple files), but the scope is well-contained: it only activates when a Blob has no explicit chunkSize, no offset, and was constructed from multiple parts. The fix is additive — existing single-part and sliced blobs are unaffected.
Other factors
All issues raised during review have been addressed: deinit() is now idempotent (head/tail/len zeroed after walk, preventing double-free), setPartSizes frees the allocation when store == null, memory accounting covers part_sizes in both memoryCost() and calculateEstimatedByteSize(), skipEmptyParts prevents merging after zero-length parts, and errdefer this.deinit() covers OOM cleanup in doneWithPartSizes. The regression test validates all edge cases including empty blobs, single parts, and string parts.
robobun
force-pushed
the
farm/d0b40a30/fix-blob-stream-chunking
branch
2 times, most recently
from
2f8e953 to
78da3b2
Compare
When a Blob is constructed from multiple parts (e.g. new Blob([a, b, c])), Blob.stream() now delivers each original part as a separate chunk instead of concatenating everything into a single chunk. The fix stores original part sizes in the Blob's byte store during construction, then uses them in ByteBlobLoader's drain() and onPull() to deliver chunks at part boundaries.
- Guard multipart mode on blob covering entire store (blob.size == store.size()), preventing sliced blobs from incorrectly using part boundaries - Include part_sizes allocation in Store.memoryCost() and Blob.calculateEstimatedByteSize() for accurate GC accounting - Handle partial reads in onPull/drain via current_part_remain tracking - Use blob.stream() instead of blob.arrayBuffer() in content test so the ByteBlobLoader path is exercised
- Free part_sizes when store is null (empty blob leak) - Add errdefer for slice in doneWithPartSizes (OOM leak) - Skip zero-length parts in ByteBlobLoader to prevent merging
robobun
force-pushed
the
farm/d0b40a30/fix-blob-stream-chunking
branch
from
376766e to
a02d6b0
Compare
| pub fn onStart(this: *ByteBlobLoader) streams.Start { | ||
| if (this.part_sizes) |sizes| { | ||
| var max_part: Blob.SizeType = 0; | ||
| for (sizes[0..this.part_count]) |s| { | ||
| max_part = @max(max_part, s); | ||
| } | ||
| return .{ .chunk_size = max_part }; | ||
| } | ||
| return .{ .chunk_size = this.chunk_size }; | ||
| } |
There was a problem hiding this comment.
🔴 The multipart streaming path removes the 16KB safety cap from drain(), allowing bun.handleOom(byte_list.clone(...)) to synchronously copy an entire large part in one call — e.g., a blob with a 100MB part triggers a 100MB blocking heap allocation on the hot stream/pipe path. Additionally, onStart() returns the uncapped max_part as the chunk_size hint, which can direct the JS stream infrastructure to allocate a receive buffer as large as the biggest part (e.g., 300MB for a 300MB-part blob). Fix drain() by applying @min(16384, this.current_part_remain); for onStart(), consider applying @min(max_part, 1024 * 1024 * 2) or a higher cap to bound worst-case allocations (noting a strict 2MB cap would break one-chunk-per-part for parts larger than 2MB).
Extended reasoning...
drain() removes the 16KB safety cap for multipart blobs
In ByteBlobLoader.drain() (lines 223-226), non-multipart blobs are capped at 16384 bytes per drain call via @min(16384, this.remain). For multipart blobs, max_drain is set to this.current_part_remain with no upper bound. Since drain() always clones synchronously via bun.handleOom(byte_list.clone(bun.default_allocator)), any blob constructed from large parts (e.g., new Blob([buf1, buf2]) where each part is 50MB) will cause drain() to allocate and copy 50MB in one blocking call on the hot stream path.
The 16KB cap exists to keep drain() a 'small best-effort grab' for stream backpressure handling. drain() is called from ResumableSink.zig during pipeTo() operations, where callers expect a small, immediately-available chunk -- not a full copy of a multi-MB blob part. Crucially, applying @min(16384, this.current_part_remain) does not break part-boundary semantics: current_part_remain is decremented by the amount actually drained (line 236), so multiple 16KB drain calls still correctly advance through part boundaries. The cap is safe to restore.
Concrete proof for drain(): Create new Blob([largeChunk1, largeChunk2]) where each chunk is 50MB, then call .stream().pipeTo(writable). During backpressure handling, ResumableSink calls drain(). Without the cap, max_drain = current_part_remain = 50MB, so byte_list.clone(...) synchronously copies 50MB into a new heap buffer, blocking the event loop and causing a large uncontrolled allocation on every such pipe operation.
onStart() returns uncapped max_part as chunk_size hint
In ByteBlobLoader.onStart() (lines 83-92), when part_sizes != null, the function computes max_part (the maximum of all part sizes) and returns it directly as chunk_size with no cap. The non-multipart path returns this.chunk_size, which is always capped at 2MB in setup() via @min(..., 1024 * 1024 * 2). The chunk_size returned from onStart() is used by the JS stream infrastructure to allocate the TypedArray buffer passed to each onPull() call -- so returning 300MB as chunk_size causes a ~300MB TypedArray allocation before the first onPull() even runs.
Addressing the refutation: One verifier argues this is intentional -- if chunk_size were capped at 2MB but a part is 5MB, buffer.len would be 2MB, and onPull() (which computes min(buffer.len, max_chunk)) would deliver only 2MB per call, requiring multiple pulls and breaking one-chunk-per-part semantics. This concern is technically valid for a strict 2MB cap. However, the original issue #28762 deals exclusively with tiny parts (1-59 bytes), and the PR's own tests only cover small parts. There is no evidence the author considered blobs with hundreds-of-MB individual parts. For the common case the feature was designed for, a 2MB cap is safe; for large parts, a higher cap (e.g., 32-64MB) would accommodate practical use cases while bounding worst-case allocations. At minimum, the unbounded allocation risk should be documented.
Concrete proof for onStart(): new Blob([new ArrayBuffer(300 * 1024 * 1024), new ArrayBuffer(300 * 1024 * 1024)]).stream() -- has_parts is true (two parts, no offset, no user chunk size), so max_part = 300MB. onStart() returns { .chunk_size = 300_000_000 }. The stream infrastructure allocates a 300MB TypedArray receive buffer before the first onPull() call, even though the data is already in memory and could be streamed in bounded chunks.
|
This isn't a bug! |
| current = stack.pop() orelse break; | ||
| } | ||
|
|
||
| const joined = try joiner.done(bun.default_allocator); | ||
| const joined, const part_sizes = try joiner.doneWithPartSizes(bun.default_allocator, true); | ||
|
|
||
| if (!could_have_non_ascii) { | ||
| return Blob.initWithAllASCII(joined, bun.default_allocator, global, true); | ||
| const blob = Blob.initWithAllASCII(joined, bun.default_allocator, global, true); | ||
| setPartSizes(blob.store, part_sizes); | ||
| return blob; | ||
| } | ||
| return Blob.init(joined, bun.default_allocator, global); | ||
| const blob = Blob.init(joined, bun.default_allocator, global); | ||
| setPartSizes(blob.store, part_sizes); | ||
| return blob; | ||
| } | ||
|
|
||
| pub const Any = union(enum) { |
There was a problem hiding this comment.
🔴 When new Blob([multiPartBlob]) is called with a single-element array, the fast path at Blob.zig:4021 returns blob.dupe() instead of going through the joiner, causing the copy to inherit the original store's part_sizes/part_count. As a result, copy.stream() emits N chunks (one per original part) instead of 1, while new Blob([multiPartBlob, ""]).stream() correctly emits 1 chunk — an observable inconsistency introduced by this PR.
Extended reasoning...
What the bug is
This PR adds part_sizes/part_count fields to Blob.Store.Bytes and new multipart-aware logic in ByteBlobLoader.setup() that activates when those fields are non-null. The guard in setup() is:
const has_parts = !has_user_chunk_size and blobe.offset == 0 and blobe.size == store.size() and
store.data == .bytes and store.data.bytes.part_sizes != null and store.data.bytes.part_count > 1;This is correct when the store was constructed directly from multiple parts. However, the PR did not account for the single-element Blob fast path in fromJSWithoutDeferGC().
The specific code path that triggers it
At Blob.zig:3960, when new Blob([existingBlob]) is called, top_iter.len == 1 sets might_only_be_one_thing = true and extracts top_value. The fast path at line 3976 activates. At lines 4012-4021, when top_value.jsTypeLoose() == .DOMWrapper and top_value.as(Blob) succeeds and move == false (the fromJSClone path), the code executes return blob.dupe().
blob.dupe() copies the Blob struct value, sharing the same *Blob.Store pointer (ref-counted). Since the original store had part_sizes != null and part_count = N, the duped blob inherits both. The setPartSizes call introduced by this PR is only invoked on the joiner path (lines 4187-4202 of the diff), not on this fast path.
Why existing code does not prevent it
The fast path was designed as a zero-copy optimization: for a single-element Blob input, share the store rather than round-tripping through StringJoiner. This was safe before this PR because stores carried no per-part metadata. After this PR, the store carries part_sizes that alter streaming behavior, making shared-store semantics observable at the stream() API level.
Impact
new Blob([multi]).stream() emits N chunks (one per original part), while new Blob([multi, ""]).stream() emits 1 chunk (the multi-element path goes through the joiner, creating a fresh 1-node store with null part_sizes). This is a behavioral regression: pre-PR, both patterns emitted 1 chunk; post-PR, they differ.
Step-by-step proof
const multi = new Blob([p1, p2, p3])— store haspart_sizes = [n1, n2, n3],part_count = 3.const copy = new Blob([multi])—top_iter.len == 1,might_only_be_one_thing = true,top_value = multi.- Fast path fires:
top_value.jsTypeLoose() == .DOMWrapper,top_value.as(Blob)succeeds,move == false→return blob.dupe()at line 4021. copy.store==multi.store(same pointer, ref-count bumped). Store still haspart_sizes != null,part_count = 3.copy.stream()→ByteBlobLoader.setup():has_partsevaluates totrue(offset==0, size==store.size(), part_sizes!=null, part_count>1).- Stream delivers 3 chunks instead of 1.
Fix
Strip part_sizes from the duped blob before returning it in the fast path, or route single-element Blob inputs through the joiner. The simplest fix is to clear the inherited part metadata on the duped copy:
var duped = blob.dupe();
if (duped.store) |s| {
if (s.data == .bytes) {
s.data.bytes.part_sizes = null;
s.data.bytes.part_count = 0;
}
}
return duped;Note this is safe because dupe() increments the ref-count but the part_sizes are owned by the store — clearing them on one ref is not correct (would affect the original). Instead, the duped blob should be routed through the joiner to get a fresh store without part_sizes, OR setPartSizes should be called with null on the blob after dupe() if the store is shared (ref_count > 1).
| pub fn done(this: *StringJoiner, allocator: Allocator) ![]u8 { | ||
| const result, _ = try this.doneWithPartSizes(allocator, false); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🟣 This is a pre-existing issue: doneWithEnd() (StringJoiner.zig:157-185) is missing both safety fixes that this PR explicitly applied to the sibling doneWithPartSizes(). (1) No errdefer this.deinit() guard -- if allocator.alloc() at line 169 fails with OOM, all accumulated linked-list nodes leak. (2) No state reset after the node walk -- this.head/this.tail/this.len are left as dangling pointers on the success path, so any future deinit() call would walk already-freed nodes causing use-after-free.
Extended reasoning...
What the bug is and how it manifests
doneWithEnd() at lines 157-185 of src/string/StringJoiner.zig has two safety issues identical to what this PR fixed in doneWithPartSizes(), but were not applied here.
Issue 1 - OOM path node leak: doneWithEnd() calls try allocator.alloc(u8, this.len + end.len) at line 169 with no errdefer this.deinit() guard. If this allocation fails with error.OutOfMemory, every linked-list node in the joiner leaks. The PR explicitly added errdefer this.deinit() to doneWithPartSizes() at line 96 but did not add the same to doneWithEnd().
Issue 2 - Missing state reset on success path: After the while loop (lines 172-179) frees every node via prev.deinit(this.allocator), this.head, this.tail, and this.len are never zeroed. The function returns with dangling non-null pointers. The PR explicitly added the state reset to doneWithPartSizes() at lines 132-134, and also made deinit() idempotent by zeroing fields at lines 148-150 -- but neither change was applied to doneWithEnd()'s own success path.
The specific code path that triggers it
For Issue 1: a caller accumulates N strings into the joiner, then calls doneWithEnd(allocator, suffix). The single try at line 169 fails. All N nodes remain unfreed.
For Issue 2: doneWithEnd() succeeds. this.head is now a dangling pointer to freed node memory. If any code path subsequently calls deinit() on the same joiner (e.g., an outer errdefer j.deinit() that fires because something later in the same scope fails), deinit() sees this.head != null, skips the early-return guard, enters the walk loop, and dereferences freed memory.
Why existing code does not prevent it
The PR audited and fixed the exact same two patterns in doneWithPartSizes() and made deinit() idempotent, but the pre-existing doneWithEnd() was not updated. The three callers (SourceMapStore.zig:189, Chunk.zig:564, sourcemap.zig:810) do not currently have outer errdefer j.deinit() guards, so the double-free scenario from Issue 2 is not reachable today. However, the OOM leak from Issue 1 is formally real whenever alloc() returns an error.
Impact
The OOM node leak (Issue 1) is a real memory leak on the error path. The missing state reset (Issue 2) is a latent use-after-free hazard: currently safe because no callers guard with errdefer j.deinit(), but fragile -- any future refactor adding such a guard to a caller would introduce heap corruption. Both issues are pre-existing relative to this PR.
Step-by-step proof for Issue 1
- A source-map builder pushes N strings into a StringJoiner via push(), accumulating N linked-list nodes.
- doneWithEnd(allocator, suffix) is called.
- allocator.alloc(u8, this.len + end.len) fails with error.OutOfMemory.
- The try propagates the error; there is no errdefer this.deinit() in scope.
- All N linked-list nodes and any heap-cloned string data they reference remain unfreed.
How to fix it
Apply the same two fixes from doneWithPartSizes(): add errdefer this.deinit() immediately after confirming this.head != null (before the try allocation), and add this.head = null; this.tail = null; this.len = 0 after the node-walk loop before return slice.
2 participants
Closes #28762
Problem
Blob.stream()concatenates all chunks into a single chunk when a Blob is constructed from multiple parts. For example:Cause
During Blob construction from multiple parts,
StringJoinerconcatenates everything into a single contiguous buffer, losing part boundary information. WhenByteBlobLoaderstreams the data, both itsdrain()andonPull()methods deliver data based on the total blob size, producing one chunk for any blob under 2MB.Fix
doneWithPartSizes()that returns both the joined buffer and an array of individual part sizes.part_sizes/part_countfields to theBytesstruct to preserve original part boundary metadata.drain()andonPull()now limit each chunk to the current part's size when part boundaries are available, delivering one chunk per original part.Part boundaries are only used when:
chunkSizewas passed to.stream()Verification