Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions patches/zstd/mt-clear-job-ready.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
Clear jobReady when the job table is erased.

With nbWorkers >= 1, ZSTDMT_createCompressionJob() sets mtctx->jobReady when
it has prepared a job and no worker is free to take it.
ZSTDMT_releaseAllJobResources() erases every job description, the prepared one
too, and leaves the flag set. ZSTDMT_initCStream_internal() does not clear it
either.

The next frame on that context then skips the preparation and posts the
erased description. The worker calls ZSTDMT_getCCtx(NULL) and the process gets
SIGSEGV (zstdmt_compress.c:697).

The caller does not have to reset anything to get there. zstd erases the table
and starts a new session by itself when a job fails, and when
ZSTDMT_compressStream_generic() returns stage_wrong. A frame on the same
context after one of those, while a job waited for a worker, crashes.

ZSTD_CCtx_reset(cctx, ZSTD_reset_session_only) in an open frame is one more
way in. reset() of a node:zlib zstd stream makes that call, and Node v26.10.0
exits with SIGSEGV there.

facebook/zstd has the same code on its dev branch, and no issue there reports
it (checked 2026-09-29).

--- a/lib/compress/zstdmt_compress.c
+++ b/lib/compress/zstdmt_compress.c
@@ -1023,6 +1023,8 @@
}
mtctx->inBuff.buffer = g_nullBuffer;
mtctx->inBuff.filled = 0;
+ /* Bun: the loop above erased the job that jobReady refers to. */
+ mtctx->jobReady = 0;
mtctx->allJobsCompleted = 1;
}

15 changes: 11 additions & 4 deletions scripts/build/deps/zstd.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,17 @@ export const zstd: Dependency = {
commit: ZSTD_COMMIT,
}),

// x64 targets nehalem, so zstd picks its BMI2 kernels at run time and
// probes CPUID in every CCtx/DCtx init. CPUID is a VM exit under a
// hypervisor (about 2 us each, two per init). Probe once instead.
patches: ["patches/zstd/bmi2-probe-once.patch"],
patches: [
// x64 targets nehalem, so zstd picks its BMI2 kernels at run time and
// probes CPUID in every CCtx/DCtx init. CPUID is a VM exit under a
// hypervisor (about 2 us each, two per init). Probe once instead.
"patches/zstd/bmi2-probe-once.patch",
// 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",
Comment on lines +49 to +53

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.

🟡 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…

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

],

build: cfg => {
const sources = [...SOURCES];
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/node/node_zlib_binding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ impl<T: CompressionStreamImpl> CompressionStream<T> {
global_this: &JSGlobalObject,
callframe: &CallFrame,
) -> JsResult<JSValue> {
// reset() destroys and re-creates the brotli/zstd encoder state (or
// reset() re-creates the brotli encoder state (or resets the zstd session, or
// mutates the z_stream). Doing so while an async write is running on
// the threadpool would be a use-after-free / data race, so node throws
// a plain Error here rather than touching live state.
Expand Down
45 changes: 41 additions & 4 deletions src/runtime/node/zlib/NativeZstd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,11 +458,48 @@ mod _impl {
}
}

/// Keeps the dictionary and parameters, as node does since v26.10.0 (nodejs/node#65867).
pub(crate) fn reset(&mut self) -> Error {
// Matches node's `ZstdContext::ResetStream()`, which calls `Init()`
// with its default (empty) dictionary — a reset drops the dictionary.
// `init` frees the previous context itself.
self.init(self.pledged_src_size, None)
// No `..`: a field added to `Context` must be kept or cleared here to compile.
let Self {
mode,
state,
pledged_src_size,
flush: _,
input: _,
output: _,
remaining: _,
} = *self;
// JS can reach this with no context: init() was never called, or it failed.
let Some(state) = state else {
return Error::OK;
};
let result = match mode {
NodeMode::ZSTD_COMPRESS => {
// SAFETY: state is a valid CCtx set by init().
let result =
unsafe { c::ZSTD_CCtx_reset(state.cast(), c::ZSTD_reset_session_only) };
if c::ZSTD_isError(result) > 0 {
result
} else {
// A session reset clears the pledged size: zstd keeps it for one frame.
// SAFETY: state is a valid CCtx set by init().
unsafe {
c::ZSTD_CCtx_setPledgedSrcSize(state.cast(), pledged_src_size as _)
}
}
}
// SAFETY: state is a valid DCtx set by init().
NodeMode::ZSTD_DECOMPRESS => unsafe {
c::ZSTD_DCtx_reset(state.cast(), c::ZSTD_reset_session_only)
Comment thread
robobun marked this conversation as resolved.
},
_ => unreachable!(),
};
if c::ZSTD_isError(result) == 0 {
return Error::OK;
}
self.remaining = result as u64;
self.get_error_info()
}

/// Frees the Zstd encoder/decoder state without changing mode.
Expand Down
2 changes: 2 additions & 0 deletions src/zstd/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ pub mod c {
// ZSTD_cParameter
pub const ZSTD_c_compressionLevel: ZSTD_cParameter = 100;

// ZSTD_ResetDirective
pub const ZSTD_reset_session_only: ZSTD_ResetDirective = 1;
pub const ZSTD_reset_session_and_parameters: ZSTD_ResetDirective = 3;

// ZSTD_ErrorCode (zstd_errors.h) — only the public stable subset.
Expand Down
68 changes: 68 additions & 0 deletions test/js/node/test/parallel/test-zlib-zstd-reset.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
'use strict';

require('../common');
const assert = require('assert');
const { finished } = require('stream/promises');
const test = require('node:test');
const zlib = require('zlib');

const dictionary = Buffer.from(
'Lorem ipsum dolor sit amet, consectetur adipiscing elit. ' +
'Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.',
);
const input = Buffer.from(
'Lorem ipsum dolor sit amet, consectetur adipiscing elit. '.repeat(100),
);

async function collect(stream, ...data) {
const chunks = [];
stream.on('data', (chunk) => chunks.push(chunk));
for (let i = 0; i < data.length - 1; i++) {
stream.write(data[i]);
}
stream.end(data[data.length - 1]);
await finished(stream);
return Buffer.concat(chunks);
}

test('ZstdCompress reset preserves its initial options', async () => {
const options = {
dictionary,
pledgedSrcSize: input.length,
params: {
[zlib.constants.ZSTD_c_compressionLevel]: 19,
[zlib.constants.ZSTD_c_checksumFlag]: 1,
},
};
const expected = await collect(zlib.createZstdCompress(options), input);
const reset = zlib.createZstdCompress(options);
reset.reset();

assert.deepStrictEqual(await collect(reset, input), expected);
});

test('ZstdDecompress reset preserves its dictionary', async () => {
const compressed = zlib.zstdCompressSync(input, { dictionary });
const decompress = zlib.createZstdDecompress({ dictionary });
decompress.reset();

assert.deepStrictEqual(await collect(decompress, compressed), input);
});

test('ZstdDecompress reset preserves its parameters', async () => {
const compressed = await collect(zlib.createZstdCompress({
params: {
[zlib.constants.ZSTD_c_windowLog]: 11,
},
}), Buffer.alloc(2048), Buffer.alloc(2048));
const decompress = zlib.createZstdDecompress({
params: {
[zlib.constants.ZSTD_d_windowLogMax]: 10,
},
});
decompress.reset();

await assert.rejects(collect(decompress, compressed), {
code: 'ZSTD_error_frameParameter_windowTooLarge',
});
});
Loading
Loading