zstd: free the output buffer when Bun.zstdDecompress decodes a content-size-less frame to 0 bytes - #39083
zstd: free the output buffer when Bun.zstdDecompress decodes a content-size-less frame to 0 bytes#39083robobun wants to merge 1 commit into
Conversation
…des to 0 bytes A frame without a content size in its header is decoded through the streaming path of bun_zstd::decompress_alloc, which reserves 4 KiB of output before the first ZSTD_decompressStream call. When the frame decodes to nothing, the Vec comes back with len 0 and capacity 4096. Both Bun.zstdDecompressSync and Bun.zstdDecompress then leaked it into JSValue::create_buffer, which registers no deallocator for an empty slice (the slice cannot say whether its pointer is dangling or a live allocation), so the block was never freed: about 4 KiB per call. Hand the result over as a Box<[u8]> via create_buffer_from_box instead. into_boxed_slice frees an empty Vec's allocation outright and trims the spare capacity of a non-empty streamed result, so the Buffer owns exactly its bytes and an empty Buffer owns nothing.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 12:13 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
|
Reproduced on bun 1.4.0 and on the debug (ASAN) build at 88a6398: decompressing a content-size-less frame that decodes to 0 bytes ( The new tests in |
There was a problem hiding this comment.
LGTM — targeted leak fix using the documented create_buffer_from_box hand-off.
What was reviewed:
- Confirmed
create_buffer_from_box(src/jsc/JSValue.rs:606) is the documented preferred path overleak()+create_buffer, and its empty-box arm registers no deallocator (no invalid-free risk). - Checked the shared async
ZstdJob::thenalso serves compress results — those are alreadyshrink_to_fit'd and never empty, sointo_boxed_slice()is a no-op there. - Leak tests mirror the existing streaming-error leak test's warmup/threshold shape; the 10 MiB bound is ~5× below the unfixed per-round growth, and the loop only fails if all 5 rounds exceed it, so isolated ASAN spikes don't flake.
Extended reasoning...
Overview
Two-line fix at each of two call sites in src/runtime/api/BunObject.rs (JSZstd::decompress_sync and ZstdJob::then): replace output.leak() → JSValue::create_buffer with output.into_boxed_slice() → JSValue::create_buffer_from_box. This frees the 4 KiB growth buffer that bun_zstd::decompress_alloc's streaming path reserves when a content-size-less frame decodes to 0 bytes, which create_buffer can't free because it registers no deallocator for empty slices (src/jsc/JSValue.rs:594). New tests in test/js/bun/util/zstd.test.ts cover correctness (empty and non-empty content-size-less frames round-trip on both sync and async entry points) and leak regression (RSS converges after warmup).
Security risks
None. No new input parsing; the hand-built test frames exercise an existing decoder path. into_boxed_slice() is a standard Rust operation that shrinks a Vec to its length and can only reduce the allocation. The empty-box case in create_buffer_from_box correctly passes None for the deallocator (verified at src/jsc/JSValue.rs:621), so there is no invalid-free on the dangling pointer of an empty box — the existing "collecting empty decompression results does not free a dangling pointer" test continues to guard that.
Level of scrutiny
Medium. This is a memory-ownership change at an FFI boundary, which the repo review guidelines flag as the most-blocked category. However: the change moves to a more type-safe API (Box<[u8]> carries the allocation-vs-dangling distinction that &mut [u8] cannot), create_buffer_from_box is explicitly documented as the preferred hand-off, and it's already used the same way in Archive.rs and NodeHTTPResponse.rs. The async then is shared with the compress path, but compress output is already shrink_to_fit'd in run() and is never empty, so into_boxed_slice() there is effectively a no-op. The one behavioral side effect (non-empty streamed results now shed doubling slack via one extra realloc) is called out in the PR description and matches what the compress path already does.
Other factors
The new leak tests follow the exact shape of the neighboring "does not leak on streaming decompression error" test (#29857's fix for the error path of the same reader): 10k iterations per batch, Bun.gc(true), up to 5 warmup rounds, pass on the first round with <10 MiB growth. The PR's measurements show the unfixed build grows 52–56 MiB per round under ASAN vs <5 MiB fixed, so the threshold has ~5× headroom. The async test's isolated 10–11 MiB single-round spikes don't flake because the loop only reports failure if all 5 rounds exceed the bound. The correctness test also covers the non-empty content-size-less frame, guarding against into_boxed_slice() breaking the common case. robobun independently confirmed the repro and fix. No prior reviews or outstanding comments.
|
#39417 ended up needing the same fix (its new test decompresses a content-size-less frame to 0 bytes, and LeakSanitizer flagged this leak), so it now also hands |
|
Closing: this change landed on main as part of #39417 (8bc4d2a). Both Bun.zstdDecompressSync and the Bun.zstdDecompress job now hand the result to JS through JSValue::create_buffer_from_box on a boxed slice trimmed to the bytes produced (decompress_to_box in src/runtime/api/BunObject.rs), so an empty result owns no memory, and main's zstd.test.ts has a test for the empty content-size-less frame. Verified by running this PR's tests against a debug build of main: the sync and async leak tests pass there, and the same tests still fail against a build from before #39417 (RSS grows by 39 MiB per 10000 calls), so they do detect the leak this PR fixed. |
Problem
Bun.zstdDecompressSync/Bun.zstdDecompresscall on a frame that has no content size in its header and decodes to 0 bytes leaks about 4 KiB. 50k calls grow RSS from 37 MiB to 236 MiB on bun 1.4.0; the process never gets the memory back.bun_zstd::decompress_alloc(src/zstd/lib.rs:311), whose reader reserves 4096 bytes of output before the firstZSTD_decompressStreamcall (src/zstd/lib.rs:428). When the frame produces nothing, theVeccomes back withlen 0, capacity 4096.JSZstd::decompress_syncandZstdJob::then(src/runtime/api/BunObject.rs), then passedoutput.leak()toJSValue::create_buffer, which registers no deallocator for an empty slice (src/jsc/JSValue.rs:594), so the 4 KiB block is never freed.Bun.zstdCompressSync(new Uint8Array(0))produces takes the preallocating path with a capacity-0Vecand does not leak; only the streaming path is affected.Fix
JSValue::create_buffer_from_box(global, output.into_boxed_slice()).create_buffercannot fix this itself, since a&mut [u8]of length 0 does not say whether its pointer is a live allocation or the dangling pointer of a capacity-0Vec(the existing "collecting empty decompression results does not free a dangling pointer" test covers the second case, where registering a deallocator would be an invalid free).Box<[u8]>carries that distinction in the type:into_boxed_slicefrees the allocation of an emptyVecand trims the spare capacity of a non-empty one, socreate_buffer_from_boxalways has exactly the bytes it is handed and nothing else. This is the hand-offcreate_buffer_from_boxdocuments as the preferred one and whatArchive.rsandNodeHTTPResponse.rsalready use; the zlib twins in this file (leak_list_into_uint8array) get the same effect fromshrink_to_fit.decompress_allockeeps it independent of how theVecgets filled: the other callers ofdecompress_alloconly read theVec, and the in-flight rewrites of its internals (zstd: delete ZstdReaderArrayList in favour of StreamingDecoder #37555 moves it toStreamingDecoder, zstd: throw instead of aborting when the output buffer cannot be allocated #39038 makes its growth fallible) both still return aVecwith spare capacity.Vec's doubling slack alive for the lifetime of the returnedBuffer. That costs onereallocper streamed decompression; the compress entry points already pay the sameshrink_to_fit.zstdCompressSync/zstdCompressare deliberately unchanged: a compressed frame is never empty and both alreadyshrink_to_fit, so there is nothing to leak there (and zstd: throw instead of aborting when the output buffer cannot be allocated #39038 rewrites those lines).frames without a content size in the headerblock intest/js/bun/util/zstd.test.ts: a hand-built frame (no content size, one empty raw block) decodes to an emptyBufferon both entry points, and 10k decompressions per batch must stop growing RSS after warmup (same shape as the neighbouring streaming-error leak test). Before the fix each batch grows RSS by 38 to 39 MiB on bun 1.4.0 and by 52 to 56 MiB on the unfixed debug (ASAN) build; with the fix the growth is under 5 MiB in the first round and around 0 afterwards. A non-empty content-size-less frame still round-trips on both entry points.test/js/bun/util/zstd.test.ts(86 tests) andtest/regression/issue/23314/(which streams a 20 MiB declared-size frame through the now-shrunk path) pass on the debug build.Related but distinct: #35856 adds a size check on these same lines; #29857 fixed the error path of the same streaming reader, this is its success path.
Background
decompress_allocpreallocates exactly the declared size when it is present and at most 16 MiB, and otherwise decodes throughZSTD_decompressStream, appending to aVecthat it grows in 4 KiB steps (so theVecnormally ends with spare capacity).JSValue::create_bufferwraps a mimalloc allocation in a NodeBufferwithout copying and registersMarkedArrayBuffer_deallocatorto free it when theBufferis collected. AVecwithlen 0may point at a live allocation (capacity > 0) or at a dangling sentinel (capacity 0), and a slice does not distinguish them, socreate_bufferregisters nothing for empty slices.Vec::into_boxed_sliceshrinks the allocation tolen, which forlen 0means freeing it, so an emptyBox<[u8]>never owns memory.Repro and measurements
zstdDecompressSynczstdDecompresszstdDecompressSynczstdDecompressPer-round RSS growth of the new test's batches (10k calls each) on the fixed debug (ASAN) build, three runs each; the test passes as soon as a round grows by less than 10 MiB:
Final RSS after 90k and after 180k fixed calls is the same to within a few MiB (sync 368-372 vs 374 MiB, async 390-392 vs 395 MiB), so the isolated async spikes are one-time heap growth, not a slower leak. On the unfixed debug build every round grows by 52 to 56 MiB.