Repository navigation
Conversation
reset() of a zstd stream freed the ZSTD_CCtx or ZSTD_DCtx and made a new one, so the stream lost its dictionary and its parameters. It now resets the session on the same context and sets the pledged size again, as Node does since v26.10.0 (nodejs/node#65867). A zstd patch clears mtctx->jobReady where zstd erases its job table. With ZSTD_c_nbWorkers, zstd kept that flag after it erased a job that waited for a worker, and the next frame on the context ran the erased job (SIGSEGV). zstd reaches that state by itself after a job fails. A session reset in an open frame reaches it too, so reset() needs the patch.
|
Status Reproduced on bun 1.4.3 canary (367d939) and on a release build of main (542f52b) with the script in the description. After Pull request: #44261 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. Walkthrough
ChangesZstandard stream reset
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The supplied change only adds tests for the zstd reset behavior, and no merge-blocking issue was found in them. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:35 PM PT - Sep 29th, 2026
✅ @robobun, your commit 6bf1d1af55332bc1ad21a8e9e30741ab02a20190 passed in 🧪 To try this PR locally: bunx bun-pr 44261That installs a local version of the PR into your bun-44261 --bun |
Node v24.3.0 has no dictionary option for zstd streams. The test of the options failed when `node` on the machine was that version. An older Node now skips all 12 stream tests.
Each comment that reset() adds is one line. The comment in CompressionStream::reset keeps the text of main and changes one line.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/node/zlib/NativeZstd.rs— A process that writes to a multithreaded zstd compressor with a dictionary and then close()s it can crash with a heap use-after-free in the worker thread, with or without a reset(). close() at NativeZstd.rs:698 calls ZSTD_CCtx_reset(session_and_parameters), whose parameter half runs ZSTD_clearAllDicts and frees the cdict while a ZSTD_c_nbWorkers job that reads it may still be running; the worker is only joined later inside ZSTD_freeCCtx at NativeZstd.rs:712. The PR touches the reset/close lifecycle and its own fixture comment names this order (#44201) but leaves the close() site as is. …Why this was flagged
…Fix: join outstanding jobs before dropping the dictionary on every teardown path, e.g. call ZSTD_freeCCtx directly (it joins the pool before freeing) or a session-only reset followed by a flush that waits for the workers, and apply the same to the reset()+close() sequence.
Trigger: createZstdCompress({ dictionary, params: { [ZSTD_c_nbWorkers]: 1 } }), one write() large enough to post a job (>= 512 KiB or ZSTD_c_jobSize), then close() or stream destruction while the worker is still compressing; reached through CompressionStream::close -> Context::close at NativeZstd.rs:688. At NativeZstd.rs:698-701 close() calls ZSTD_CCtx_reset(state, ZSTD_reset_session_and_parameters): the session half sets streamStage to init, so the parameters half passes its stage check and runs ZSTD_clearAllDicts, freeing the cdict the running ZSTDMT_compressionJob dereferences. Only afterwards does deinit_state at NativeZstd.rs:712 call ZSTD_freeCCtx, which joins the pool. The base branch has the same close() body, so this is a pre-existing crash, but the diff makes reset() no longer free the context (the old…
Verification: pre-existing. Trigger: a zstd compressor created with
dictionaryandZSTD_c_nbWorkers >= 1, one write large enough to post a job (>= jobSize; default target job size at level 3 is 8 MiB, 512 KiB whenZSTD_c_jobSizeis set, or any filled input on ZSTD_e_flush), thenclose()/destroy()before the frame ends and while the worker is still on the first job. Mechanism:Context::closeat…
…d Node The case "reset() while a job waits for the worker" still tries up to 10 times to reach that state. It no longer fails when the last try does not reach it. The file runs under Node.js only when the `node` of the machine is v26.10.0 or later. An older Node drops the options in reset(), and Node 20 cannot load a .ts file. The test is skipped there.
|
The The old |
| // With nbWorkers >= 1, zstd keeps its jobReady flag when it erases the | ||
| // job table, so the next frame on that context posts an erased job to a | ||
| // worker (SIGSEGV). zstd gets there by itself after a failed job, and | ||
| // node:zlib reset() gets there too. Not reported upstream yet. | ||
| "patches/zstd/mt-clear-job-ready.patch", |
There was a problem hiding this comment.
🟡 nit (optional): Maintainers get a vendored zstd bug-fix patch with no upstream issue to track and no self-obsoleting check, so it can outlive the upstream fix silently. scripts/build/deps/zstd.ts:53 registers mt-clear-job-ready.patch with only a comment saying "Not reported upstream yet", and nothing is added to scripts/build/workarounds.ts. Fix: file the upstream facebook/zstd issue (the PR already has a standalone C repro) and link it from the patch header and this comment, and add a workarounds.ts entry whose expectedToBeFixed trips when ZSTD_COMMIT moves past the pinned commit, with a cleanup string naming the patch to drop.
Why this was flagged
The diff adds patches/zstd/mt-clear-job-ready.patch and lists it in the patches array at scripts/build/deps/zstd.ts:49-53; the patch header (patches/zstd/mt-clear-job-ready.patch:22-23) says no upstream issue exists and the zstd.ts comment says "Not reported upstream yet". scripts/build/CLAUDE.md "Adding a workaround" says every temporary fix waiting on an upstream release registers an entry in scripts/build/workarounds.ts with an expectedToBeFixed predicate, and workarounds.ts:9-11 explicitly lists "vendored dep bump" as such a case; the registry currently has only two entries (workarounds.ts:67-119), none for this patch. The consequence is operational for maintainers: when ZSTD_COMMIT is bumped later, the one-line hunk will still apply cleanly on top of an upstream fix (or a refactor that moved the bug), so nothing tells the developer to re-evaluate or drop the patch, and there is no upstream reference to check against. On the base branch this patch and this class of tracking gap do not exist.
Verification: nit. Triggering condition: a future zstd bump where upstream fixes jobReady differently (so the Bun hunk still applies) — nothing then tells anyone the patch is obsolete. Verified facts: (1) scripts/build/deps/zstd.ts:49-53 (diff) adds "patches/zstd/mt-clear-job-ready.patch" with the comment "... Not reported upstream yet."; (2) the patch header…
There was a problem hiding this comment.
No code change for this one.
The upstream issue. There is none yet, and I did not open one on facebook/zstd. The Notes in the description have what a report needs: a C program with no bun in it, its result on v1.5.7 and on dev at 01b7154f (SIGSEGV in 20 of 20 runs), and its result with the one line of the patch (0 of 20). When an issue exists, its link belongs in the header of the patch and in the comment here.
The workarounds.ts entry. None of the other 30 patches in patches/ has one. The two entries there are for the toolchain and for the libc crate. A check on ZSTD_COMMIT stops every zstd update at configure, also when upstream has no fix. The test in this PR covers the other direction: without the patch, the handle case of zlib-zstd-reset.test.ts crashes in ZSTDMT_compressionJob while the bug is there. If a maintainer wants the entry, I will add it.
|
A note for the merge with #44421, the fix for #44201. Both PRs change how a zstd handle is torn down. Measured on a debug + ASAN build with both diffs applied (head 6bf1d1a of this PR, 2026-10-01):
// in the block "zstd: a compressor with workers and a dictionary"
[
"reset(), a write and destroy()",
`const stream = open();
await write(stream, input.subarray(0, 1000));
stream.reset();
await write(stream, input);
stream.destroy();
console.log("destroyed");`,
"destroyed",
],Neither PR needs the other to be correct by itself. The row above was measured with an earlier state of the block (a 123-byte dictionary). |
Problem
reset()of anode:zlibzstd stream drops its dictionary and parameters. AftercreateZstdDecompress({ dictionary }).reset(), a valid frame fails withZSTD_error_corruption_detected.Context::reset(src/runtime/node/zlib/NativeZstd.rs:461) calledinit(), which makes a default zstd context. Node did the same until v26.9.0 and keeps the options since v26.10.0 (zlib: fix zstd reset nodejs/node#65867). No user reported this.Fix
Context::resetresets the session on the same context and sets the pledged size again, as Node v26.10.0 does.jobReadywhere zstd erases its job table. Without it, the frame after areset()in an open frame can crash inZSTDMT_compressionJob. Node v26.10.0 does.test/js/node/zlib/zlib-zstd-reset.test.tsand Node'stest-zlib-zstd-reset.js. main fails 14 of 17 tests.Background
ZSTD_c_nbWorkers, zstd cuts the input into jobs for worker threads.jobReadymarks a job that waits for a free worker.zlib.tskeeps the init arguments and callsinit()again. That misses_handle.reset()and adds state to every stream.Downsides
reset()to drop the options of a zstd stream now keeps them.reset(). node:zlib: a zstd compressor with ZSTD_c_nbWorkers and a dictionary crashes when it closes while a job runs #44201 and node:zlib: a zstd compressor with ZSTD_c_nbWorkers gives no output for a chunk larger than one job #44257 (on main withoutreset()) now apply after it too.reset()toclose(): 3,663,393 B, not 5,288 B, for a default compressor. A decompressor keeps 134,706,984 B, not 95,976 B, after awindowLog27 frame.Notes
Which Node versions keep the options
ZstdCompressContext::ResetStream/ZstdDecompressContext::ResetStreamreturn Init(...): a new context, no dictionary, no parametersZSTD_CCtx_reset(session_only)+ZSTD_CCtx_setPledgedSrcSize,ZSTD_DCtx_reset(session_only)zlib.reset()now says: "For Zstd streams, cancel the current frame and start a new session while preserving the configured parameters and dictionary."process.versionv26.3.0, and zlib: brotli + zstd dictionaries, Node-compatible reset() — +4 node v26.3.0 tests, zlib suite 57/62 → 61/62 (98.4%) #34427 matched v26.3.0 on purpose. The tree already follows a newer Node in other places, for example the free-socket guard of v26.4.0 (node-http-agent-free-socket.test.ts).reset()kept it too.Reproduction
before reset 2000,after reset throws ZSTD_error_corruption_detected Data corruption detectedbefore reset 2000,after reset 2000before reset 2000,after reset 2000The three cases of Node's test (compressor with dictionary, level 19, checksum and pledged size, decompressor with dictionary, decompressor with
ZSTD_d_windowLogMax): main gives 76 bytes in place of 26,ZSTD_error_corruption_detected, and 4096 decoded bytes where the limit must reject the frame. This PR and Node v26.10.0 give 26 bytes, the decoded input, andZSTD_error_frameParameter_windowTooLarge.The zstd patch
ZSTDMT_createCompressionJob()setsmtctx->jobReadywhen it prepared a job and no worker is free.ZSTDMT_releaseAllJobResources()erases every job description and keeps the flag. The next frame skips the preparation and posts the erased job. The worker callsZSTDMT_getCCtx(NULL)(zstdmt_compress.c:697)._handleand call noreset()exit with SIGSEGV on main in 30 of 30 and 29 of 30 runs, and in 0 of 30 with the patch.SEGV ... in ZSTDMT_compressionJob zstdmt_compress.c:697in 5 of 5 runs. With the patch the case passes.reset()did not reach it, because it made a new context.devat 01b7154f (2026-09-18), and in 0 of 20 with the line of the patch. Ondev, aZSTD_e_continuecall after the frame started to end (stage_wrong) and aZSTD_CCtx_reset(cctx, ZSTD_reset_session_only)in an open frame give the same two results. An open pull request there (number 4805) adds a NULL check toZSTDMT_getCCtx()and does not clearjobReady.Tests
test/js/node/zlib/zlib-zstd-reset.test.tsusesnode:test. 12 stream tests run in Bun and in Node. The last test runs the file under thenodeof the machine when that Node is v26.10.0 or later, and 12 of 12 pass there. The test is skipped for an older Node: itsreset()drops the options, Node v24.3.0 has nodictionaryoption for zstd, and Node v20.19.0 cannot load a.tsfile. CI has Node v26.3.0, so CI skips this one test.reset()of a handle with no context (2),reset()while a job waits for the worker, a failed job while a job waits (noreset()), andreset()while the worker reads the dictionary. The worker case checks that the job did wait (the two writes gave no output) and tries again if it did not. After 10 tries it runs in either state, so the scheduler cannot fail it. At load average 710, 1 of 500 first tries missed the state on a release build, and 0 of 300 on a debug build.test/js/node/test/parallel/test-zlib-zstd-reset.jsis the file of Node v26.10.0, byte for byte (blob839669bc63).heap-use-after-freeinZSTDMT_compressionJobwith the free inContext::init.test/js/node/zlib/(all files),test/js/web/streams/compression.test.ts,test/js/bun/util/zstd.test.ts,test/js/web/fetch/fetch-compress.test.ts, and the 71 vendoredtest-zlib*,test-webstreams-*compression*andtest-stream-iter-transform*files.Measurements (release builds, main 542f52b against this PR, linux x64)
reset(), gdb breakpoints, 1000 resets of a compressor and 1000 of a decompressor: context frees 2000 to 0, allocations 2000 (101,264,000 B requested) to 0.reset()and the next frame:reset()makes 3 allocator calls on main (5,288 B for a compressor, 95,976 B for a decompressor) and 0 now. The next frame allocates 856,217 B (compressor) or 131,072 B (decompressor) again on main and nothing now.valgrind,perfandstraceare not on this machine, so there is no instruction count. The instruction sequences ofNativeZstdPrototype__write(729),__writeSync(568),NativeZstdClass__construct(227),ZSTD_compressStream2(544) andZSTD_decompressStream(787) are the same in the two binaries after addresses are removed.Contexthas no new field.ZSTDMT_releaseAllJobResources99 to 106 instructions (1 store, 6 of padding), 394 to 404 bytes. 20,000 one-shotzstdCompressSynccalls with default options call it 0 times in the two builds.NativeZstdPrototype__reset223 to 261 bytes,NativeZstdPrototype__init2112 to 2884 (it now containsContext::init, 867 bytes, which has one caller left),ZSTDMT_releaseAllJobResources.bloatyis not installed. The numbers are fromsizeandnm -S.reset()toclose()(ZSTD_sizeof_CCtxandZSTD_sizeof_DCtx, a harness linked to the zstd objects of the release build): default compressor 3,663,393 B, level 19 93,848,223 B,nbWorkers2 51,680,744 B and 2 idle threads, decompressor after awindowLog27 frame 134,706,984 B. main: 5,288 B and 95,976 B, because it made a new context. A stream that is used again allocates these buffers again on main, so the peak is the same. bun reports 5,272 B and 95,968 B to the GC in the two builds.reset()blocks the JS thread for 829 to 1147 ms on main (median 960) and 0 ms now. The first write after it takes 0.13 to 0.45 ms on main and 574 to 983 ms now (median 616). zstd waits for the running jobs in the two cases. For a stream write that wait is on the thread pool. 100 cycles of jobs andreset()make 1 worker pool and free 1 in the two builds.Behaviour that changes outside the report
reset()on a handle with no context made a default context on main. It makes none now, so a later write does nothing, as it does withoutreset(). Two handles have no context: one made from_handle.constructorand never initialized (Node v26.10.0 crashes on it), and a decompressor whose dictionary failed to load (Node throws from the constructor, see node:zlib: one failure path for zstd and brotli init(): close, call onerror, throw #38684). For the second one, main decoded frames afterreset()without the dictionary.reset()of a compressor with a corrupt dictionary: main then compresses with no dictionary. Now the write fails withZSTD_error_memory_allocation, as it does withoutreset()and as in Node v26.10.0.reset(): main gave a frame, made with no dictionary, no workers and the default level. Now the stream ends with no output (node:zlib: a zstd compressor with ZSTD_c_nbWorkers gives no output for a chunk larger than one job #44257), and with a dictionary the process can crash at close (node:zlib: a zstd compressor with ZSTD_c_nbWorkers and a dictionary crashes when it closes while a job runs #44201). main and Node v26.10.0 do that withoutreset(). Node v26.10.0 does it afterreset()too.Open pull request on the same struct
#33413 adds decoder state to
Context(decode,frame_prefix_size,possible_frame_types) and clears it ininit(). The two branches merge with no text conflict.reset()here names every field ofContextin a pattern with no.., so the merged tree does not compile untilreset()clears the new fields, as Node'sZstdDecompressContext::ResetStreamdoes.Not in this PR
close(), the finalizer and a secondinit(). Tracked in node:zlib: a zstd compressor with ZSTD_c_nbWorkers and a dictionary crashes when it closes while a job runs #44201. This PR removes the fourth place, the oldreset(). The cause is the order ofZSTD_freeCCtxContent, and it is on main withoutreset().ArrayBufferaccepted, other types rejected). Tracked in node:zlib: zstd ignores an ArrayBuffer dictionary and does not reject a dictionary of a wrong type (Node v26.10.0 does) #44258. They come from zlib: accept ArrayBuffer dictionary in Zstd nodejs/node#64599 and from the second commit of zlib: fix zstd reset nodejs/node#65867, and they change the constructor.jobReady. I did not open one. The program above and its results ondevare ready for it. The link then belongs in the header of the patch and inscripts/build/deps/zstd.ts.reset()also drops the dictionary and the parameters. Every released Node does the same. Node main changed it (zlib: preserve brotli params and dictionary on reset nodejs/node#66157).reset()after a partial frame gave output, for zstd (zlib: reject reset while a zstd frame is incomplete nodejs/node#66088) and for gzip and deflate (zlib: reject reset after gzip emitted incomplete output nodejs/node#66179). Node v26.10.0 accepts it, and so does this PR.