Skip to content

DecompressionStream("zstd"): decode concatenated frames instead of rejecting as trailing junk - #36021

Merged
Jarred-Sumner merged 13 commits into
mainfrom
farm/aca345a7/zstd-decompressionstream-multi-frame
Jul 27, 2026
Merged

Jarred-Sumner merged 13 commits into
mainfrom
farm/aca345a7/zstd-decompressionstream-multi-frame

Conversation

@robobun

@robobun robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator

Repro

import zlib from "node:zlib";
const f1 = zlib.zstdCompressSync(Buffer.from("first frame\n"));
const f2 = zlib.zstdCompressSync(Buffer.from("second frame\n"));
const cat = Buffer.concat([f1, f2]);            // valid 2-frame stream (cat a.zst b.zst / pzstd)

await new Response(new Blob([cat]).stream()
  .pipeThrough(new DecompressionStream("zstd"))).text();
// TypeError: Trailing junk found after the end of the compressed stream
//   code: "ERR_TRAILING_JUNK_AFTER_STREAM_END"

Bun.zstdDecompressSync(cat);  // "first frame\nsecond frame\n" (correct)

A zstd stream is one or more concatenated frames (RFC 8878 section 3.1); zstd -d, pzstd output (multi-frame by construction), Bun.zstdDecompressSync, and the fetch() Content-Encoding: zstd decoder all decode every frame. Only DecompressionStream("zstd") rejects the second frame as trailing junk, so a real pzstd file is unreadable through it while every other zstd decoder in the process accepts it. A leading skippable frame (magic 0x184D2A50..5F) is rejected the same way.

Cause

The native zstd do_work() calls ZSTD_decompressStream once per write; libzstd stops at every frame boundary (returns 0 with the next frame unconsumed), so processCallback sees availInAfter > 0 with output room and, because DecompressionStream passes rejectGarbageAfterEnd: true, raises ERR_TRAILING_JUNK_AFTER_STREAM_END. Gunzip avoids this because NativeZlib::do_work_inflate already loops on multi-member input.

Fix

In NativeZstd::do_work, after ZSTD_decompressStream returns 0 with input and output room remaining, peek the next four bytes: if they are the frame magic 0xFD2FB528 or a skippable-frame magic 0x184D2A5? (or a prefix of either when fewer than four remain, so a header split across chunks decodes), call ZSTD_decompressStream again. libzstd resets the DCtx to start-of-frame itself, so no explicit reset is needed. Bytes that are not a magic or magic prefix are left unconsumed so the existing ERR_TRAILING_JUNK_AFTER_STREAM_END check still fires for genuine garbage, and createZstdDecompress still silently ignores trailing non-frame data the way Node.js does.

src/js/node/zlib.ts is unchanged.

Verification

New tests in test/js/web/streams/compression.test.ts cover a 2-frame stream written as one chunk, a 2-frame stream split mid-header across two writes, 64 frames totalling more than one output chunk, a stream with leading and trailing skippable frames, and trailing non-frame garbage still rejected with ERR_TRAILING_JUNK_AFTER_STREAM_END. The four multi-frame tests fail on main with that error and pass with this change; the Node-compat createZstdDecompress trailing-data tests in test/js/node/zlib/zlib.test.js still pass.


[review] gate passed · iteration 2 · 3 files touched

fails on main (without fix)
ASAN without fix: 6 failed, 2 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/zlib/zlib.test.js test/js/web/streams/compression.test.ts
bun test v1.4.0 (f5a1ac96b)

test/js/web/streams/compression.test.ts:
(pass) CompressionStream and DecompressionStream > brotli > compresses data with brotli [302.30ms]
(pass) CompressionStream and DecompressionStream > brotli > decompresses brotli data [113.72ms]
(pass) CompressionStream and DecompressionStream > brotli > round-trip compression with brotli [341.86ms]
(pass) CompressionStream and DecompressionStream > zstd > compresses data with zstd [35.81ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses zstd data [58.35ms]
(pass) CompressionStream and DecompressionStream > zstd > round-trip compression with zstd [223.26ms]
TypeError: Trailing junk found after the end of the compressed stream
 code: "ERR_TRAILING_JUNK_AFTER_STREAM_END"

      at unknown:1:1
      at processCallback (node:zlib:431:37)
(fail) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream [45.46ms]
TypeError: Trailing junk found afte
... (truncated)

release without fix: 2 skipped
bun test v1.4.0-canary.1 (f62fd6a24)

test/js/web/streams/compression.test.ts:
(pass) CompressionStream and DecompressionStream > brotli > compresses data with brotli [5.42ms]
(pass) CompressionStream and DecompressionStream > brotli > decompresses brotli data [2.23ms]
(pass) CompressionStream and DecompressionStream > brotli > round-trip compression with brotli [6.12ms]
(pass) CompressionStream and DecompressionStream > zstd > compresses data with zstd [2.48ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses zstd data [1.10ms]
(pass) CompressionStream and DecompressionStream > zstd > round-trip compression with zstd [4.71ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream [0.51ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream split across writes (next = zstd frame) [1.23ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream split across writes (next = skippable frame) [2.40ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses many concatenated zstd frames larger than one output 
... (truncated)
passes on PR (with fix)
ASAN with fix: 2 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/zlib/zlib.test.js test/js/web/streams/compression.test.ts
bun test v1.4.0 (f5a1ac96b)

test/js/web/streams/compression.test.ts:
(pass) CompressionStream and DecompressionStream > brotli > compresses data with brotli [377.58ms]
(pass) CompressionStream and DecompressionStream > brotli > decompresses brotli data [115.46ms]
(pass) CompressionStream and DecompressionStream > brotli > round-trip compression with brotli [324.69ms]
(pass) CompressionStream and DecompressionStream > zstd > compresses data with zstd [44.76ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses zstd data [62.49ms]
(pass) CompressionStream and DecompressionStream > zstd > round-trip compression with zstd [186.59ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream [22.91ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream split across writes (next = zstd frame) [64.47ms]
(pass) CompressionStream and DecompressionStream > zstd > decompress
... (truncated)

release with fix: 2 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 766ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_output v
... (truncated)
diff hotspot
src/runtime/node/zlib/NativeZstd.rs     | 56 +++++++++++++++++++++----
 test/js/node/zlib/zlib.test.js          |  6 +++
 test/js/web/streams/compression.test.ts | 73 +++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 8 deletions(-)

gate history · 4 passed · 0 rejected · iteration 2

evidence per changed file
file                                     reads  edits  tests
src/runtime/node/zlib/NativeZstd.rs          6      8      0
test/js/node/zlib/zlib.test.js               2      1      0
test/js/web/streams/compression.test.ts      6      8      0

…cting as trailing junk

A zstd stream is one or more concatenated frames (RFC 8878 3.1); pzstd
output is multi-frame by construction, as is `cat a.zst b.zst`.
DecompressionStream("zstd") threw ERR_TRAILING_JUNK_AFTER_STREAM_END at
the second frame because the trailing-junk check added for deflate/brotli
treats any input remaining after a frame boundary as junk.

When the stream is zstd-decompress and opted in via rejectGarbageAfterEnd
(only DecompressionStream does), re-enter the write loop with the
remaining input so the native decoder starts the next frame. Actual
trailing garbage then surfaces as ZSTD_error_prefix_unknown from zstd
itself. The plain node:zlib zstd stream keeps stopping at the first
frame to match Node.js.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Zstandard decompression now processes concatenated and skippable frames while rejecting trailing garbage. Tests cover frame boundaries, split writes, large concatenations, and expected errors.

Zstandard decompression

Layer / File(s) Summary
Frame detection and decompression loop
src/runtime/node/zlib/NativeZstd.rs
Adds frame-prefix detection and loops ZSTD_decompressStream across valid subsequent zstd or skippable frames.
DecompressionStream frame coverage
test/js/web/streams/compression.test.ts
Adds zstd fixtures and tests for concatenated frames, split headers, large output, skippable frames, and trailing-junk errors.

Suggested reviewers: cirospaciari

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: zstd DecompressionStream now accepts concatenated frames instead of rejecting them as junk.
Description check ✅ Passed The description covers the bug, cause, fix, and verification, matching the template's intent even though it uses custom section headings.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:45 AM PT - Jul 27th, 2026

❌ @robobun, your commit f5a1ac9 has 1 failures in Build #83128 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36021

That installs a local version of the PR into your bun-36021 executable, so you can run:

bun-36021 --bun

Comment thread src/js/node/zlib.ts Outdated
Comment thread src/js/node/zlib.ts Outdated
Comment thread src/js/node/zlib.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 test/js/web/streams/compression.test.ts:277 — The bare .rejects.toThrow() will pass on any rejection, so a future regression that rejects for the wrong reason (or reverts to ERR_TRAILING_JUNK_AFTER_STREAM_END) would still go green. Since the PR description states garbage now surfaces as ZSTD_error_prefix_unknown wrapped in a TypeError, consider asserting that — e.g. .rejects.toThrow(TypeError) plus a check that e.code (or message) contains prefix_unknown.

    Extended reasoning...

    What the issue is

    The new test rejects trailing garbage after a zstd frame at test/js/web/streams/compression.test.ts:277 ends with:

    await expect(read).rejects.toThrow();

    There is no error class, code, or message argument. REVIEW.md's test rules explicitly call this out under "Every assertion must be able to fail, and assert the strongest invariant":

    specific error class/code/message (never bare toThrow())

    Why this matters here

    This test exists specifically to prove that the new multi-frame handling in processCallback didn't accidentally start accepting trailing garbage. But the assertion only proves that something rejected. Any of the following futures would still pass:

    • A regression re-introduces the old ERR_TRAILING_JUNK_AFTER_STREAM_END path (i.e., the _continueOnFrameEnd gate stops firing) — the test would happily accept that error even though it means multi-frame streams are broken again for a different-shaped input.
    • An unrelated stream-setup bug causes .text() to reject before the zstd decoder ever sees the bytes.
    • The native decoder crashes with a different error class entirely.

    The PR description itself states the intended behavior precisely: "actual garbage surfaces as ZSTD_error_prefix_unknown from zstd itself rather than ERR_TRAILING_JUNK_AFTER_STREAM_END". That's the invariant the test should pin.

    Step-by-step proof

    1. withJunk is one valid frame followed by 0xde 0xad 0xbe 0xef.
    2. With this PR, processCallback sees availInAfter > 0 && _continueOnFrameEnd, re-enters handle.write, and zstd fails on the 4-byte prefix with ZSTD_error_prefix_unknown. zlibOnError destroys the stream, and DecompressionStream surfaces it as a TypeError with the zstd code attached (same wrapping the neighboring brotli test at line ~390 asserts on).
    3. await expect(read).rejects.toThrow() matches — but it would equally match if step 2 were replaced by any thrown value.

    Suggested fix

    Match the pattern already used by the brotli-decoder-error test in this same file — assert the class and the code:

    await expect(read).rejects.toThrow(TypeError);
    await expect(read).rejects.toMatchObject({ code: expect.stringContaining("prefix_unknown") });

    (or a try/catch asserting e instanceof TypeError and e.code / e.message mentions prefix_unknown, whichever reads cleaner). Matching a substring rather than the exact code string keeps it robust to how the native layer prefixes zstd error names.

    Severity

    This is a test-quality nit per the repo guidelines — the runtime behavior is correct and the .rejects is properly awaited so the assertion can fail; it just isn't asserting the strongest invariant available.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in 0c375bf: the trailing-garbage test now asserts { code: "ZSTD_error_prefix_unknown" } so a regression back to ERR_TRAILING_JUNK_AFTER_STREAM_END (or any other rejection) would fail it. The rejection surfaces as a plain Error, not a TypeError, so the assertion keys on the code only.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nit, I also checked the re-entry loop for non-termination — ZSTD_decompressStream always consumes input, fills output, or errors when availInAfter > 0, so continueNextFrame cannot spin. Short (<4-byte) trailing garbage was also checked — zstd buffers partial magic and errors on the finish flush rather than silently accepting it.

Extended reasoning...

The change is small and correctly gated (_continueOnFrameEnd only fires when rejectGarbageAfterEnd is set, so createZstdDecompress keeps Node's stop-at-first-frame behavior). The native do_work in NativeZstd.rs calls ZSTD_decompressStream once per write, and on re-entry with leftover input it will either decode the next frame, buffer a partial header (driving availInAfter to 0), or error — no path leaves input unconsumed with output space and no error, so the loop terminates. Leaving human review for the inline nit (trailing-garbage error class changed from TypeError to plain Error) and the layering question of whether multi-frame handling belongs in the native do_work alongside gunzip's multi-member loop.

Comment thread src/js/node/zlib.ts Outdated
robobun added 2 commits July 26, 2026 22:13
Peek the 4 bytes following a completed frame: the zstd frame magic
0xFD2FB528 or a skippable-frame magic 0x184D2A5? means another frame
follows, so call ZSTD_decompressStream again (mirrors the GUNZIP
multi-member loop). A 1-3 byte tail that is a prefix of either magic is
fed through so a header split across chunks decodes; anything else is
left unconsumed so the JS layer's ERR_TRAILING_JUNK_AFTER_STREAM_END
check still fires for DecompressionStream and createZstdDecompress keeps
silently ignoring trailing data like Node.js.

Drops the JS-side _continueOnFrameEnd gate; the peek makes it
unnecessary. Adds coverage for a leading/trailing skippable frame.
Comment thread src/runtime/node/zlib/NativeZstd.rs Outdated
Comment thread src/runtime/node/zlib/NativeZstd.rs Outdated
Comment thread src/runtime/node/zlib/NativeZstd.rs Outdated
Comment thread src/runtime/node/zlib/NativeZstd.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nit: my earlier TypeError-regression concern is resolved by the native magic-peek redesign — non-magic trailing bytes are left unconsumed, so ERR_TRAILING_JUNK_AFTER_STREAM_END (a TypeError) still fires, and createZstdDecompress still silently discards them since _rejectGarbageAfterEnd is unset. Also checked the while ret == 0 loop for non-termination — each iteration either advances input.pos (libzstd buffers partial headers) or fills output, so it's bounded — and the unsafe slice in next_input_is_frame, where avail > 0 guarantees pos < size within the caller-kept-alive set_buffers region.

Extended reasoning...

The author reworked the fix after my previous review: instead of a JS-side _continueOnFrameEnd flag that fed all leftover bytes back to libzstd (which changed the trailing-garbage error from TypeError/ERR_TRAILING_JUNK_AFTER_STREAM_END to plain Error/ZSTD_error_prefix_unknown), the loop now lives in native do_work and only re-enters when the next bytes are a zstd/skippable magic or magic prefix. That mirrors NativeZlib::do_work_inflate's GUNZIP multi-member loop and preserves both the DecompressionStream trailing-junk TypeError and the Node-compat silent-discard path. The remaining nit (1-3 trailing bytes matching a magic prefix) is narrow and rooted in a pre-existing gap; deferring the approve/don't-approve call to a human given the unsafe pointer arithmetic on untrusted decompression input.

Comment thread src/runtime/node/zlib/NativeZstd.rs Outdated

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify? Do we need to use unsafe?

@robobun

robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Simplified in 021aa84: the peek now copies up to 4 bytes into a stack [u8; 4] with one ptr::copy_nonoverlapping and compares once, dropping the separate <4 / ==4 branches and the u32 reassembly.

The unsafe cannot be removed entirely: self.input is the C ZSTD_inBuffer (src: *const c_void), so reading the bytes after the frame boundary is a raw-pointer read by construction. It is the same pointer the ZSTD_decompressStream call just advanced, kept alive for the duration of the write by CompressionStream::{write,write_sync} (the JS input buffer is rooted on the callframe / cached on the handle). The unsafe is now a single 1-4 byte copy within that slice.

const ZSTD_MAGICNUMBER: [u8; 4] = 0xFD2FB528u32.to_le_bytes();
const ZSTD_MAGIC_SKIPPABLE: [u8; 4] = 0x184D2A50u32.to_le_bytes();

fn next_input_is_frame(&self) -> bool {
    let n = (self.input.size - self.input.pos).min(4);
    if n == 0 {
        return false;
    }
    let mut head = [0u8; 4];
    // SAFETY: input.src[pos..pos+n] lies within the slice installed by set_buffers.
    unsafe {
        ptr::copy_nonoverlapping(
            self.input.src.cast::<u8>().add(self.input.pos),
            head.as_mut_ptr(),
            n,
        );
    }
    head[..n] == Self::ZSTD_MAGICNUMBER[..n]
        || (head[0] & 0xF0 == Self::ZSTD_MAGIC_SKIPPABLE[0]
            && head[1..n] == Self::ZSTD_MAGIC_SKIPPABLE[1..n])
}

libzstd does ship ZSTD_isFrame(buffer, size) which would let us hand it the raw pointer directly without forming a Rust slice, but it is ZSTDLIB_STATIC_API (unstable) and returns 0 for size < 4, so it cannot handle the split-header case.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/web/streams/compression.test.ts`:
- Around line 266-278: Extend the zstd decompression tests around the existing
leading-skippable-frame case with a stream containing a zstd frame, a skippable
frame, and a second zstd frame. Split the skippable-frame magic across writes
after 1, 2, and 3 magic bytes, then assert the decompressed output includes both
payloads, covering the partial-prefix path in next_input_is_frame().
- Around line 234-246: Update the “decompresses a multi-frame zstd stream split
across writes” test to start consuming ds.readable and await both writer.write
operations and writer.close, ensuring any writable-side failure rejects the test
deterministically while preserving the existing split-boundary scenario.
🪄 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: 5172bef6-f29f-40af-bd4a-5d543346b4a9

📥 Commits

Reviewing files that changed from the base of the PR and between 44f6469 and 021aa84.

📒 Files selected for processing (2)
  • src/runtime/node/zlib/NativeZstd.rs
  • test/js/web/streams/compression.test.ts

Comment thread test/js/web/streams/compression.test.ts Outdated
Comment thread test/js/web/streams/compression.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/web/streams/compression.test.ts`:
- Around line 241-261: Flatten the offset loop in the zstd multi-frame test into
the test.each matrix so each combination of next-frame shape and offset is an
independent, named test case. Add offsets 1, 2, and 3 as matrix parameters, use
the selected offset when computing split, and remove the sequential for-loop
while preserving the existing setup and expected-output assertions.
🪄 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: 11b2b915-3ad3-4e26-9ca0-1554e5266000

📥 Commits

Reviewing files that changed from the base of the PR and between 021aa84 and f173c56.

📒 Files selected for processing (1)
  • test/js/web/streams/compression.test.ts

Comment thread test/js/web/streams/compression.test.ts
Comment thread test/js/web/streams/compression.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the multi-frame loop mirrors NativeZlib::do_work_inflate's GUNZIP member loop, and all prior feedback has been addressed.

What was reviewed:

  • next_input_is_frame bounds: the 1–4 byte raw-pointer copy stays within [input.src+pos .. input.src+size) as installed by set_buffers; the buffer is rooted by CompressionStream::write for the duration.
  • Re-drive when output fills at a frame boundary: output.pos < output.size gates the loop, so availOutAfter == 0 still hands control back to processCallback/processChunkSync for a fresh output buffer.
  • Skippable-magic match: head[0] & 0xF0 == 0x50 covers 0x184D2A50..5F; the leading/trailing skippable-frame test exercises the loop across three consecutive ret == 0 returns.
  • zlib.zstdDecompressSync now decodes all frames (diverging from Node 26.3.0's first-frame-only behavior toward RFC 8878); pinned by the new test in zlib.test.js.
Extended reasoning...

Overview

Changes NativeZstd::do_work's ZSTD_DECOMPRESS arm to loop while ZSTD_decompressStream returns 0 (frame complete), the output buffer has room, and the unconsumed input begins with a zstd frame magic (0xFD2FB528) or skippable-frame magic (0x184D2A5?) — or a 1–3 byte prefix of either, so a header split across writes still decodes. Non-matching bytes are left unconsumed so processCallback's existing ERR_TRAILING_JUNK_AFTER_STREAM_END check still fires. Adds six DecompressionStream("zstd") tests (single-chunk multi-frame, split-header at offsets 1/2/3 × zstd/skippable next-frame, 64 frames spanning multiple output chunks, leading+trailing skippable frames, and trailing garbage still rejected) plus one zlib.zstdDecompressSync multi-frame test.

Security risks

None identified. Input is untrusted compressed data, but the new code only peeks up to 4 bytes within the caller-supplied input slice whose bounds libzstd just validated (input.pos <= input.size), then hands the same pointers back to ZSTD_decompressStream. No new allocation sizing or length arithmetic on untrusted values. The loop is bounded by forward progress: each iteration either advances input.pos, fills the output buffer, or returns non-zero, so it cannot spin.

Level of scrutiny

Medium. The single unsafe block is a bounded copy_nonoverlapping from a pointer libzstd just read through, into a stack [u8; 4]. The control-flow change directly mirrors the existing GUNZIP multi-member loop at NativeZlib.rs:577-588, so the interaction with processCallback/processChunkSync's re-drive logic is already exercised for gzip. I traced the availOutAfter == 0 re-drive path in both processCallback and processChunkSync to confirm output-buffer-full at a frame boundary correctly resumes on the next call.

Other factors

All three of my prior inline comments and both CodeRabbit findings are resolved: comments were condensed, writer ops are awaited, split-header coverage was extended to skippable frames at offsets 1/2/3, and a zstdDecompressSync multi-frame test now pins the node:zlib behavior change. The remaining known edge case — 1–3 trailing garbage bytes that coincidentally prefix a magic are consumed rather than rejected — was acknowledged as the necessary cost of split-header support and is closed by the pre-existing truncated-frame-on-finish-flush work in #33413. The intentional divergence from Node 26.3.0 (which returns only the first frame) moves zlib.zstdDecompressSync toward RFC 8878 conformance and matches Bun.zstdDecompressSync, the fetch Content-Encoding: zstd decoder, and gunzip's multi-member handling; the old behavior was silent data loss.

@robobun

robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

CI on f5a1ac9: 193/196 jobs passed. The only red lane is test/cli/run/no-orphans.test.ts timing out on :darwin: 14 x64 (a bun run --no-orphans perl-subprocess reaper test, unrelated to zstd/zlib/DecompressionStream; the same test is marked flaky on two ubuntu lanes in this build). The remaining two :darwin: 14 aarch64 jobs are stuck in the scheduled queue. test/js/web/streams/compression.test.ts and test/js/node/zlib/zlib.test.js pass on every lane that ran.

@Jarred-Sumner
Jarred-Sumner merged commit 7c92467 into main Jul 27, 2026
54 of 55 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/aca345a7/zstd-decompressionstream-multi-frame branch July 27, 2026 08:41
Jarred-Sumner pushed a commit that referenced this pull request Jul 27, 2026
…jecting as trailing junk (#36021)

## Repro

```js
import zlib from "node:zlib";
const f1 = zlib.zstdCompressSync(Buffer.from("first frame\n"));
const f2 = zlib.zstdCompressSync(Buffer.from("second frame\n"));
const cat = Buffer.concat([f1, f2]);            // valid 2-frame stream (cat a.zst b.zst / pzstd)

await new Response(new Blob([cat]).stream()
  .pipeThrough(new DecompressionStream("zstd"))).text();
// TypeError: Trailing junk found after the end of the compressed stream
//   code: "ERR_TRAILING_JUNK_AFTER_STREAM_END"

Bun.zstdDecompressSync(cat);  // "first frame\nsecond frame\n" (correct)
```

A zstd stream is one or more concatenated frames (RFC 8878 section 3.1);
`zstd -d`, pzstd output (multi-frame by construction),
`Bun.zstdDecompressSync`, and the `fetch()` `Content-Encoding: zstd`
decoder all decode every frame. Only `DecompressionStream("zstd")`
rejects the second frame as trailing junk, so a real pzstd file is
unreadable through it while every other zstd decoder in the process
accepts it. A leading skippable frame (magic `0x184D2A50..5F`) is
rejected the same way.

## Cause

The native zstd `do_work()` calls `ZSTD_decompressStream` once per
write; libzstd stops at every frame boundary (returns 0 with the next
frame unconsumed), so `processCallback` sees `availInAfter > 0` with
output room and, because `DecompressionStream` passes
`rejectGarbageAfterEnd: true`, raises
`ERR_TRAILING_JUNK_AFTER_STREAM_END`. `Gunzip` avoids this because
`NativeZlib::do_work_inflate` already loops on multi-member input.

## Fix

In `NativeZstd::do_work`, after `ZSTD_decompressStream` returns 0 with
input and output room remaining, peek the next four bytes: if they are
the frame magic `0xFD2FB528` or a skippable-frame magic `0x184D2A5?` (or
a prefix of either when fewer than four remain, so a header split across
chunks decodes), call `ZSTD_decompressStream` again. libzstd resets the
DCtx to start-of-frame itself, so no explicit reset is needed. Bytes
that are not a magic or magic prefix are left unconsumed so the existing
`ERR_TRAILING_JUNK_AFTER_STREAM_END` check still fires for genuine
garbage, and `createZstdDecompress` still silently ignores trailing
non-frame data the way Node.js does.

`src/js/node/zlib.ts` is unchanged.

## Verification

New tests in `test/js/web/streams/compression.test.ts` cover a 2-frame
stream written as one chunk, a 2-frame stream split mid-header across
two writes, 64 frames totalling more than one output chunk, a stream
with leading and trailing skippable frames, and trailing non-frame
garbage still rejected with `ERR_TRAILING_JUNK_AFTER_STREAM_END`. The
four multi-frame tests fail on `main` with that error and pass with this
change; the Node-compat `createZstdDecompress` trailing-data tests in
`test/js/node/zlib/zlib.test.js` still pass.

<!-- robobun:evidence:begin -->

---

**[review]** gate passed · iteration 2 · 3 files touched

<details><summary>fails on main (without fix)</summary>

```console
ASAN without fix: 6 failed, 2 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/zlib/zlib.test.js test/js/web/streams/compression.test.ts
bun test v1.4.0 (f5a1ac9)

test/js/web/streams/compression.test.ts:
(pass) CompressionStream and DecompressionStream > brotli > compresses data with brotli [302.30ms]
(pass) CompressionStream and DecompressionStream > brotli > decompresses brotli data [113.72ms]
(pass) CompressionStream and DecompressionStream > brotli > round-trip compression with brotli [341.86ms]
(pass) CompressionStream and DecompressionStream > zstd > compresses data with zstd [35.81ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses zstd data [58.35ms]
(pass) CompressionStream and DecompressionStream > zstd > round-trip compression with zstd [223.26ms]
TypeError: Trailing junk found after the end of the compressed stream
 code: "ERR_TRAILING_JUNK_AFTER_STREAM_END"

      at unknown:1:1
      at processCallback (node:zlib:431:37)
(fail) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream [45.46ms]
TypeError: Trailing junk found afte
... (truncated)

release without fix: 2 skipped
bun test v1.4.0-canary.1 (f62fd6a)

test/js/web/streams/compression.test.ts:
(pass) CompressionStream and DecompressionStream > brotli > compresses data with brotli [5.42ms]
(pass) CompressionStream and DecompressionStream > brotli > decompresses brotli data [2.23ms]
(pass) CompressionStream and DecompressionStream > brotli > round-trip compression with brotli [6.12ms]
(pass) CompressionStream and DecompressionStream > zstd > compresses data with zstd [2.48ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses zstd data [1.10ms]
(pass) CompressionStream and DecompressionStream > zstd > round-trip compression with zstd [4.71ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream [0.51ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream split across writes (next = zstd frame) [1.23ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream split across writes (next = skippable frame) [2.40ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses many concatenated zstd frames larger than one output 
... (truncated)
```

</details>

<details><summary>passes on PR (with fix)</summary>

```console
ASAN with fix: 2 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/zlib/zlib.test.js test/js/web/streams/compression.test.ts
bun test v1.4.0 (f5a1ac9)

test/js/web/streams/compression.test.ts:
(pass) CompressionStream and DecompressionStream > brotli > compresses data with brotli [377.58ms]
(pass) CompressionStream and DecompressionStream > brotli > decompresses brotli data [115.46ms]
(pass) CompressionStream and DecompressionStream > brotli > round-trip compression with brotli [324.69ms]
(pass) CompressionStream and DecompressionStream > zstd > compresses data with zstd [44.76ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses zstd data [62.49ms]
(pass) CompressionStream and DecompressionStream > zstd > round-trip compression with zstd [186.59ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream [22.91ms]
(pass) CompressionStream and DecompressionStream > zstd > decompresses a multi-frame zstd stream split across writes (next = zstd frame) [64.47ms]
(pass) CompressionStream and DecompressionStream > zstd > decompress
... (truncated)

release with fix: 2 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 766ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_output v
... (truncated)
```

</details>

<details><summary>diff hotspot</summary>

```
src/runtime/node/zlib/NativeZstd.rs     | 56 +++++++++++++++++++++----
 test/js/node/zlib/zlib.test.js          |  6 +++
 test/js/web/streams/compression.test.ts | 73 +++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 8 deletions(-)
```

</details>

**gate history** · 4 passed · 0 rejected · iteration 2

<details><summary>evidence per changed file</summary>

```
file                                     reads  edits  tests
src/runtime/node/zlib/NativeZstd.rs          6      8      0
test/js/node/zlib/zlib.test.js               2      1      0
test/js/web/streams/compression.test.ts      6      8      0
```

</details>

<!-- robobun:evidence:end -->

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants