From cf5451be85b0fb77e8fd4aeb2f930a1ee67f8db1 Mon Sep 17 00:00:00 2001 From: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Date: Mon, 24 Aug 2026 17:40:22 -0400 Subject: [PATCH 1/3] Honour the zstd dictionary option in node:zlib node:zlib's zstd functions accepted a `dictionary` option and dropped it. On compress that was silent, since a frame compressed without a dictionary decodes fine with one, so the only signal was a byte count that never shrank. On decompress it surfaced as "Zstd decompression failed: Data corruption detected" against bytes that were not corrupt. Wire ZSTD_CCtx_loadDictionary and ZSTD_DCtx_loadDictionary into the two zstd contexts and plumb the option through both the convenience functions and the streams, following Node's implementation in nodejs/node#59240. Both calls auto-detect the dictionary's content type, so a raw-content dictionary, which is what RFC 9842 Compression Dictionary Transport ships, works as well as a trained one. A dictionary that is neither an ArrayBufferView nor an ArrayBuffer is ignored rather than rejected, matching Node's Zstd class rather than its stricter zlib and brotli paths. Fixes #6967 --- src/node/internal/internal_zlib.ts | 29 ++- src/node/internal/internal_zlib_base.ts | 27 ++- src/node/internal/zlib.d.ts | 10 +- src/workerd/api/compression.c++ | 36 ++- src/workerd/api/compression.h | 12 +- .../api/node/tests/zlib-zstd-nodejs-test.js | 222 ++++++++++++++++++ src/workerd/api/node/zlib-util.c++ | 20 +- src/workerd/api/node/zlib-util.h | 3 +- 8 files changed, 333 insertions(+), 26 deletions(-) diff --git a/src/node/internal/internal_zlib.ts b/src/node/internal/internal_zlib.ts index 375261a45e6..38c47b989f8 100644 --- a/src/node/internal/internal_zlib.ts +++ b/src/node/internal/internal_zlib.ts @@ -16,6 +16,7 @@ import { Zlib, Brotli, Zstd, + normalizeZstdOptions, zstdInitCParamsArray, zstdInitDParamsArray, kMaxZstdCParam, @@ -345,26 +346,28 @@ export function zstdDecompressSync( data: ArrayBufferView | string, options: ZstdOptions = {} ): ZlibResult { - if (!options.info) { + const opts = normalizeZstdOptions(options); + if (!opts.info) { // Fast path, where we send the data directly to C++ - return Buffer.from(zlibUtil.zstdDecompressSync(data, options)); + return Buffer.from(zlibUtil.zstdDecompressSync(data, opts)); } // Else, use the Engine class in sync mode - return processChunk(new ZstdDecompress(options), data); + return processChunk(new ZstdDecompress(opts), data); } export function zstdCompressSync( data: ArrayBufferView | string, options: ZstdOptions = {} ): ZlibResult { - if (!options.info) { + const opts = normalizeZstdOptions(options); + if (!opts.info) { // Fast path, where we send the data directly to C++ - return Buffer.from(zlibUtil.zstdCompressSync(data, options)); + return Buffer.from(zlibUtil.zstdCompressSync(data, opts)); } // Else, use the Engine class in sync mode - return processChunk(new ZstdCompress(options), data); + return processChunk(new ZstdCompress(opts), data); } export function zstdDecompress( @@ -376,10 +379,11 @@ export function zstdDecompress( optionsOrCallback, callbackOrUndefined ); + const opts = normalizeZstdOptions(options); - if (!options.info) { + if (!opts.info) { // Fast path - zlibUtil.zstdDecompress(data, options, (res) => { + zlibUtil.zstdDecompress(data, opts, (res) => { queueMicrotask(() => { if (res instanceof Error) { callback(res); @@ -392,7 +396,7 @@ export function zstdDecompress( return; } - processChunkCaptureError(new ZstdDecompress(options), data, callback); + processChunkCaptureError(new ZstdDecompress(opts), data, callback); } export function zstdCompress( @@ -404,10 +408,11 @@ export function zstdCompress( optionsOrCallback, callbackOrUndefined ); + const opts = normalizeZstdOptions(options); - if (!options.info) { + if (!opts.info) { // Fast path - zlibUtil.zstdCompress(data, options, (res) => { + zlibUtil.zstdCompress(data, opts, (res) => { queueMicrotask(() => { if (res instanceof Error) { callback(res); @@ -420,7 +425,7 @@ export function zstdCompress( return; } - processChunkCaptureError(new ZstdCompress(options), data, callback); + processChunkCaptureError(new ZstdCompress(opts), data, callback); } export class Gzip extends Zlib { constructor(options: ZlibOptions) { diff --git a/src/node/internal/internal_zlib_base.ts b/src/node/internal/internal_zlib_base.ts index bfcd48e6745..a653a483580 100644 --- a/src/node/internal/internal_zlib_base.ts +++ b/src/node/internal/internal_zlib_base.ts @@ -826,6 +826,30 @@ export const kMaxZstdDParam = Math.max( ); export const zstdInitDParamsArray = new Int32Array(kMaxZstdDParam + 1); +// Node accepts a zstd `dictionary` as an ArrayBufferView or an ArrayBuffer, and quietly +// ignores any other type rather than throwing (lib/zlib.js, class Zstd). Normalize here so +// that both the stream path and the convenience functions' fast path behave the same way. +export function normalizeZstdDictionary( + dictionary: ZstdOptions['dictionary'] +): ArrayBufferView | undefined { + if (dictionary === undefined || isArrayBufferView(dictionary)) { + return dictionary; + } + if (isAnyArrayBuffer(dictionary)) { + return new Uint8Array(dictionary); + } + return undefined; +} + +// Returns `options` unchanged unless its dictionary needed normalizing, so the common case +// allocates nothing. +export function normalizeZstdOptions(options: ZstdOptions): ZstdOptions { + const dictionary = normalizeZstdDictionary(options.dictionary); + return dictionary === options.dictionary + ? options + : { ...options, dictionary }; +} + const zstdDefaultOptions: ZlibDefaultOptions = { flush: CONST_ZSTD_e_continue, finishFlush: CONST_ZSTD_e_end, @@ -883,7 +907,8 @@ export class Zstd extends ZlibBase { () => { queueMicrotask(processCallback.bind(handle)); }, - pledgedSrcSize + pledgedSrcSize, + normalizeZstdDictionary(options?.dictionary) ) ) { throw new ERR_ZLIB_INITIALIZATION_FAILED(); diff --git a/src/node/internal/zlib.d.ts b/src/node/internal/zlib.d.ts index 40fdddd3c5d..33b4feb1c88 100644 --- a/src/node/internal/zlib.d.ts +++ b/src/node/internal/zlib.d.ts @@ -291,6 +291,10 @@ export interface ZstdOptions { | undefined; maxOutputLength?: number | undefined; pledgedSrcSize?: number | undefined; + // Declared as a view, like ZlibOptions above, though an ArrayBuffer is also accepted at + // runtime and any other type is ignored. See normalizeZstdDictionary() in + // internal_zlib_base.ts. + dictionary?: ArrayBufferView | undefined; // Not specified in NodeJS docs but the tests expect it info?: boolean | undefined; } @@ -370,7 +374,8 @@ export class ZstdDecoder extends CompressionStream { params: Int32Array, writeResult: Uint32Array, writeCallback: () => void, - pledgedSrcSize?: number + pledgedSrcSize?: number, + dictionary?: ArrayBufferView ): boolean; params(): void; } @@ -380,7 +385,8 @@ export class ZstdEncoder extends CompressionStream { params: Int32Array, writeResult: Uint32Array, writeCallback: () => void, - pledgedSrcSize?: number + pledgedSrcSize?: number, + dictionary?: ArrayBufferView ): boolean; params(): void; } diff --git a/src/workerd/api/compression.c++ b/src/workerd/api/compression.c++ index b3ceeff3425..0735d0d10b8 100644 --- a/src/workerd/api/compression.c++ +++ b/src/workerd/api/compression.c++ @@ -603,12 +603,28 @@ ZstdEncoderContext::ZstdEncoderContext(ZlibMode _mode) : ZstdContext(_mode), cctx_(kj::disposeWith(ZSTD_createCCtx())) {} -kj::Maybe ZstdEncoderContext::initialize(uint64_t pledgedSrcSize) { +kj::Maybe ZstdEncoderContext::initialize( + uint64_t pledgedSrcSize, kj::ArrayPtr dictionary) { if (cctx_.get() == nullptr) { return CompressionError( "Could not initialize Zstd instance"_kj, "ERR_ZLIB_INITIALIZATION_FAILED"_kj, -1); } + if (dictionary.size() > 0) { + // ZSTD_CCtx_loadDictionary() copies the dictionary into the context, so `dictionary` does + // not need to outlive this call. The content type is auto-detected: a buffer starting with + // the zstd dictionary magic is read as a trained dictionary, anything else as raw content. + // Loading is deferred until the first frame begins, so the parameters set by setParams() + // afterwards still apply to the dictionary's tables. + size_t result = ZSTD_CCtx_loadDictionary(cctx_.get(), dictionary.begin(), dictionary.size()); + if (ZSTD_isError(result)) { + error_ = ZSTD_getErrorCode(result); + return CompressionError( + kj::str("Failed to load zstd dictionary: ", ZSTD_getErrorName(result)), + "ERR_ZLIB_DICTIONARY_LOAD_FAILED"_kj, -1); + } + } + if (pledgedSrcSize != ZSTD_CONTENTSIZE_UNKNOWN) { size_t result = ZSTD_CCtx_setPledgedSrcSize(cctx_.get(), pledgedSrcSize); KJ_IF_SOME(err, zstdCheckError(result, error_, "ERR_ZSTD_COMPRESSION_FAILED"_kj)) { @@ -674,7 +690,8 @@ ZstdDecoderContext::ZstdDecoderContext(ZlibMode _mode) : ZstdContext(_mode), dctx_(kj::disposeWith(ZSTD_createDCtx())) {} -kj::Maybe ZstdDecoderContext::initialize() { +kj::Maybe ZstdDecoderContext::initialize( + kj::ArrayPtr dictionary) { // dctx_ is created in the constructor. It can only be nullptr if ZSTD_createDCtx() // failed due to memory allocation failure. if (dctx_.get() == nullptr) { @@ -682,6 +699,21 @@ kj::Maybe ZstdDecoderContext::initialize() { "Could not initialize Zstd instance"_kj, "ERR_ZLIB_INITIALIZATION_FAILED"_kj, -1); } + if (dictionary.size() > 0) { + // As with the encoder, the bytes are copied into the context and the content type is + // auto-detected. Note that a raw-content dictionary carries no dictionary ID, so reading a + // frame written against a different one is not rejected as ZSTD_error_dictionary_wrong: it + // fails as corrupt, or decodes to different bytes if the frame carries no checksum. That is + // zstd's behaviour and matches what Node does with the same calls. + size_t result = ZSTD_DCtx_loadDictionary(dctx_.get(), dictionary.begin(), dictionary.size()); + if (ZSTD_isError(result)) { + error_ = ZSTD_getErrorCode(result); + return CompressionError( + kj::str("Failed to load zstd dictionary: ", ZSTD_getErrorName(result)), + "ERR_ZLIB_DICTIONARY_LOAD_FAILED"_kj, -1); + } + } + return kj::none; } diff --git a/src/workerd/api/compression.h b/src/workerd/api/compression.h index 97d6f928dfc..c4b87dd9459 100644 --- a/src/workerd/api/compression.h +++ b/src/workerd/api/compression.h @@ -455,7 +455,8 @@ class ZstdContext { jsg::Optional> params; jsg::Optional maxOutputLength; jsg::Optional pledgedSrcSize; - JSG_STRUCT(flush, finishFlush, chunkSize, params, maxOutputLength, pledgedSrcSize); + jsg::Optional> dictionary; + JSG_STRUCT(flush, finishFlush, chunkSize, params, maxOutputLength, pledgedSrcSize, dictionary); }; protected: @@ -474,7 +475,11 @@ class ZstdEncoderContext final: public ZstdContext { KJ_DISALLOW_COPY_AND_MOVE(ZstdEncoderContext); void work(); - kj::Maybe initialize(uint64_t pledgedSrcSize); + // An empty `dictionary` means no dictionary, matching Node.js, which passes an empty + // std::string_view for the same case. The bytes are copied into the context, so the + // caller does not need to keep them alive. + kj::Maybe initialize( + uint64_t pledgedSrcSize, kj::ArrayPtr dictionary = nullptr); kj::Maybe resetStream(); kj::Maybe setParams(int key, int value); kj::Maybe getError() const; @@ -495,7 +500,8 @@ class ZstdDecoderContext final: public ZstdContext { KJ_DISALLOW_COPY_AND_MOVE(ZstdDecoderContext); void work(); - kj::Maybe initialize(); + // See the note on ZstdEncoderContext::initialize() regarding `dictionary`. + kj::Maybe initialize(kj::ArrayPtr dictionary = nullptr); kj::Maybe resetStream(); kj::Maybe setParams(int key, int value); kj::Maybe getError() const; diff --git a/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js b/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js index 3fcff0eb5d2..a59800d9e7a 100644 --- a/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js +++ b/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js @@ -446,3 +446,225 @@ export const zstdStreamLargeDecompressTest = { ); }, }; + +// The dictionary tests below share this pair. `input` repeats phrases that appear in +// `dictionary`, so a dictionary-aware encoder can reference them instead of emitting them, +// which is what makes the size assertions meaningful. +const DICTIONARY = Buffer.from( + 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. ' + + 'Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. ' + + 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris.' +); + +const DICT_INPUT = + 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. ' + + 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. ' + + 'Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. ' + + 'Duis aute irure dolor in reprehenderit in voluptate velit esse cillum.'; + +// A dictionary must shrink the output and must round-trip. +export const zstdDictionarySyncTest = { + test() { + const input = Buffer.from(DICT_INPUT); + const plain = zlib.zstdCompressSync(input); + const withDict = zlib.zstdCompressSync(input, { dictionary: DICTIONARY }); + + assert( + withDict.length < plain.length, + `Dictionary should shrink the output, got ${withDict.length} with and ` + + `${plain.length} without` + ); + + const decompressed = zlib.zstdDecompressSync(withDict, { + dictionary: DICTIONARY, + }); + assert.strictEqual( + decompressed.toString(), + input.toString(), + 'Dictionary round-trip should match' + ); + }, +}; + +// Compressing a buffer against itself is the sharpest form of the size check: every byte of +// the input is already in the dictionary, so the frame collapses to a few bytes. This is the +// feature-detect that a caller has to resort to when the option is silently dropped. +export const zstdDictionaryCollapseTest = { + test() { + const input = Buffer.from( + 'the quick brown fox jumps over the lazy dog '.repeat(200) + ); + const params = { [zlib.constants.ZSTD_c_compressionLevel]: 19 }; + + const none = zlib.zstdCompressSync(input, { params }); + const good = zlib.zstdCompressSync(input, { dictionary: input, params }); + + assert( + good.length < none.length / 2, + `Self-dictionary should collapse the frame, got ${good.length} against ${none.length}` + ); + + // Parameters are applied after the dictionary is loaded, so this also proves the + // compression level still reaches the encoder. + const level1 = zlib.zstdCompressSync(input, { + dictionary: input, + params: { [zlib.constants.ZSTD_c_compressionLevel]: 1 }, + }); + assert.strictEqual( + zlib.zstdDecompressSync(good, { dictionary: input }).toString(), + input.toString(), + 'Self-dictionary round-trip should match' + ); + assert.strictEqual( + zlib.zstdDecompressSync(level1, { dictionary: input }).toString(), + input.toString(), + 'Self-dictionary round-trip at level 1 should match' + ); + }, +}; + +// Buffer, TypedArray, DataView and ArrayBuffer all name the same bytes and must behave +// identically. Mirrors Node's test/parallel/test-zlib-zstd-dictionary.js. +export const zstdDictionaryTypesTest = { + test() { + const input = Buffer.from(DICT_INPUT); + const baseline = zlib.zstdCompressSync(input, { + dictionary: DICTIONARY, + }).length; + + const arrayBuffer = DICTIONARY.buffer.slice( + DICTIONARY.byteOffset, + DICTIONARY.byteOffset + DICTIONARY.byteLength + ); + const uint8 = new Uint8Array(arrayBuffer); + const dataView = new DataView(arrayBuffer); + + for (const dictionary of [arrayBuffer, uint8, dataView]) { + const compressed = zlib.zstdCompressSync(input, { dictionary }); + assert.strictEqual( + compressed.length, + baseline, + 'Every dictionary representation should compress identically' + ); + assert.strictEqual( + zlib.zstdDecompressSync(compressed, { dictionary }).toString(), + input.toString(), + 'Every dictionary representation should decompress identically' + ); + } + }, +}; + +// Node ignores a dictionary that is neither an ArrayBufferView nor an ArrayBuffer rather +// than throwing, on both the fast path and the stream path. +export const zstdDictionaryIgnoredTypeTest = { + test() { + const input = Buffer.from(DICT_INPUT); + const plain = zlib.zstdCompressSync(input).length; + + assert.strictEqual( + zlib.zstdCompressSync(input, { dictionary: 'not a buffer' }).length, + plain, + 'A non-buffer dictionary should be ignored' + ); + assert.strictEqual( + zlib.zstdCompressSync(input, { dictionary: 'not a buffer', info: true }) + .buffer.length, + plain, + 'A non-buffer dictionary should be ignored on the stream path too' + ); + }, +}; + +// A frame written with a dictionary cannot be read without it. The checksum is enabled so +// that the mismatch is always detected rather than left to chance. +export const zstdDictionaryMismatchTest = { + test() { + const input = Buffer.from(DICT_INPUT); + const other = Buffer.from('completely unrelated filler bytes '.repeat(20)); + const compressed = zlib.zstdCompressSync(input, { + dictionary: DICTIONARY, + params: { [zlib.constants.ZSTD_c_checksumFlag]: 1 }, + }); + + assert.throws( + () => zlib.zstdDecompressSync(compressed), + (err) => err instanceof Error, + 'Decompressing without the dictionary should fail' + ); + assert.throws( + () => zlib.zstdDecompressSync(compressed, { dictionary: other }), + (err) => err instanceof Error, + 'Decompressing with the wrong dictionary should fail' + ); + }, +}; + +// The async convenience functions take the same option. +export const zstdDictionaryAsyncTest = { + async test() { + const input = Buffer.from(DICT_INPUT); + + const compressed = await new Promise((resolve, reject) => { + zlib.zstdCompress(input, { dictionary: DICTIONARY }, (err, res) => { + if (err) reject(err); + else resolve(res); + }); + }); + + assert( + compressed.length < zlib.zstdCompressSync(input).length, + 'Async compression should honour the dictionary' + ); + + const decompressed = await new Promise((resolve, reject) => { + zlib.zstdDecompress( + compressed, + { dictionary: DICTIONARY }, + (err, res) => { + if (err) reject(err); + else resolve(res); + } + ); + }); + + assert.strictEqual( + decompressed.toString(), + input.toString(), + 'Async dictionary round-trip should match' + ); + }, +}; + +// And so do the streams, where the dictionary reaches the context through initialize(). +export const zstdDictionaryStreamTest = { + async test() { + const input = Buffer.from(DICT_INPUT); + + const compress = zlib.createZstdCompress({ dictionary: DICTIONARY }); + compress.end(input); + const compressedChunks = []; + for await (const chunk of compress) { + compressedChunks.push(chunk); + } + const compressed = Buffer.concat(compressedChunks); + + assert( + compressed.length < zlib.zstdCompressSync(input).length, + 'Stream compression should honour the dictionary' + ); + + const decompress = zlib.createZstdDecompress({ dictionary: DICTIONARY }); + decompress.end(compressed); + const decompressedChunks = []; + for await (const chunk of decompress) { + decompressedChunks.push(chunk); + } + + assert.strictEqual( + Buffer.concat(decompressedChunks).toString(), + input.toString(), + 'Stream dictionary round-trip should match' + ); + }, +}; diff --git a/src/workerd/api/node/zlib-util.c++ b/src/workerd/api/node/zlib-util.c++ index a5d7dc60d42..a64970b13a9 100644 --- a/src/workerd/api/node/zlib-util.c++ +++ b/src/workerd/api/node/zlib-util.c++ @@ -669,16 +669,21 @@ bool ZlibUtil::ZstdCompressionStream::initialize(jsg::Lock& jsg::JsArrayBufferView params, jsg::JsArrayBufferView writeResult, jsg::Function writeCallback, - jsg::Optional pledgedSrcSize) { + jsg::Optional pledgedSrcSize, + jsg::Optional> dictionary) { this->initializeStream(js, writeResult, kj::mv(writeCallback)); uint64_t srcSize = pledgedSrcSize.orDefault(ZSTD_CONTENTSIZE_UNKNOWN); + kj::ArrayPtr dict = nullptr; + KJ_IF_SOME(d, dictionary) { + dict = d.asPtr(); + } kj::Maybe maybeError; if constexpr (CompressionContext::Mode == ZlibMode::ZSTD_ENCODE) { - maybeError = this->context()->initialize(srcSize); + maybeError = this->context()->initialize(srcSize, dict); } else { - maybeError = this->context()->initialize(); + maybeError = this->context()->initialize(dict); } KJ_IF_SOME(err, maybeError) { @@ -900,13 +905,18 @@ kj::Array ZlibUtil::zstdSync(jsg::Lock& js, InputSource data, ZstdCont GrowableBuffer result(ZLIB_PERFORMANT_CHUNK_SIZE, maxOutputLength); // Initialize the context + kj::ArrayPtr dictionary = nullptr; + KJ_IF_SOME(d, opts.dictionary) { + dictionary = d.asPtr(); + } + if constexpr (Context::Mode == ZlibMode::ZSTD_ENCODE) { uint64_t pledgedSrcSize = opts.pledgedSrcSize.orDefault(ZSTD_CONTENTSIZE_UNKNOWN); - KJ_IF_SOME(err, ctx.initialize(pledgedSrcSize)) { + KJ_IF_SOME(err, ctx.initialize(pledgedSrcSize, dictionary)) { JSG_FAIL_REQUIRE(Error, err.message); } } else { - KJ_IF_SOME(err, ctx.initialize()) { + KJ_IF_SOME(err, ctx.initialize(dictionary)) { JSG_FAIL_REQUIRE(Error, err.message); } } diff --git a/src/workerd/api/node/zlib-util.h b/src/workerd/api/node/zlib-util.h index 4e4a7bcd9d5..dea02f4142f 100644 --- a/src/workerd/api/node/zlib-util.h +++ b/src/workerd/api/node/zlib-util.h @@ -333,7 +333,8 @@ class ZlibUtil final: public jsg::Object { jsg::JsArrayBufferView params, jsg::JsArrayBufferView writeResult, jsg::Function writeCallback, - jsg::Optional pledgedSrcSize); + jsg::Optional pledgedSrcSize, + jsg::Optional> dictionary); void params() { // Currently a no-op, and not accessed from JS land. From 91512d591855716f5eace110220bf1186a5d3ac8 Mon Sep 17 00:00:00 2001 From: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:49:51 -0400 Subject: [PATCH 2/3] Test the zstd dictionary error paths and trained dictionaries The dictionary tests only used raw-content dictionaries and never reached a load failure. Add coverage for: - a trained dictionary: it shrinks the frame, the frame header carries its dictionary ID, and a frame read without it fails as "Dictionary mismatch" - a corrupt trained dictionary on the decoder, which fails at load on the fast path, the stream constructor and the engine path - the same dictionary on the encoder, which zstd only parses once the first frame begins, so it is accepted at construction and fails on first write - pledgedSrcSize alongside a dictionary, correct and incorrect Every new assertion also passes under Node 26.9.0. Use Node's exact "Failed to load zstd dictionary" message. zstd reports a corrupt dictionary as ZSTD_error_memory_allocation, because the DDict it built came back null, so appending zstd's error name told the caller they were out of memory. --- src/workerd/api/compression.c++ | 15 +- .../api/node/tests/zlib-zstd-nodejs-test.js | 163 ++++++++++++++++++ 2 files changed, 173 insertions(+), 5 deletions(-) diff --git a/src/workerd/api/compression.c++ b/src/workerd/api/compression.c++ index 045a6c88df3..a8ba757d804 100644 --- a/src/workerd/api/compression.c++ +++ b/src/workerd/api/compression.c++ @@ -609,13 +609,14 @@ kj::Maybe ZstdEncoderContext::initialize( // not need to outlive this call. The content type is auto-detected: a buffer starting with // the zstd dictionary magic is read as a trained dictionary, anything else as raw content. // Loading is deferred until the first frame begins, so the parameters set by setParams() - // afterwards still apply to the dictionary's tables. + // afterwards still apply to the dictionary's tables. It also means a malformed trained + // dictionary is not detected here: it fails in work() when the first frame begins, which + // is where Node reports it too. On a fresh context this call can only fail to allocate. size_t result = ZSTD_CCtx_loadDictionary(cctx_.get(), dictionary.begin(), dictionary.size()); if (ZSTD_isError(result)) { error_ = ZSTD_getErrorCode(result); return CompressionError( - kj::str("Failed to load zstd dictionary: ", ZSTD_getErrorName(result)), - "ERR_ZLIB_DICTIONARY_LOAD_FAILED"_kj, -1); + "Failed to load zstd dictionary"_kj, "ERR_ZLIB_DICTIONARY_LOAD_FAILED"_kj, -1); } } @@ -699,12 +700,16 @@ kj::Maybe ZstdDecoderContext::initialize( // frame written against a different one is not rejected as ZSTD_error_dictionary_wrong: it // fails as corrupt, or decodes to different bytes if the frame carries no checksum. That is // zstd's behaviour and matches what Node does with the same calls. + // + // Unlike the encoder, the decoder parses a trained dictionary's entropy tables here, so a + // malformed one fails now. zstd reports that as ZSTD_error_memory_allocation, because the + // DDict it tried to build came back null, so the message leaves zstd's error name out and + // uses Node's exact text instead. size_t result = ZSTD_DCtx_loadDictionary(dctx_.get(), dictionary.begin(), dictionary.size()); if (ZSTD_isError(result)) { error_ = ZSTD_getErrorCode(result); return CompressionError( - kj::str("Failed to load zstd dictionary: ", ZSTD_getErrorName(result)), - "ERR_ZLIB_DICTIONARY_LOAD_FAILED"_kj, -1); + "Failed to load zstd dictionary"_kj, "ERR_ZLIB_DICTIONARY_LOAD_FAILED"_kj, -1); } } diff --git a/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js b/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js index a59800d9e7a..6095e9891ed 100644 --- a/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js +++ b/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js @@ -668,3 +668,166 @@ export const zstdDictionaryStreamTest = { ); }, }; + +// Every dictionary above is raw content. A trained dictionary is a different path through +// zstd: it starts with the dictionary magic, carries entropy tables, and gives the frame a +// dictionary ID. This one was trained with `zstd --train --maxdict=256` on 400 lines shaped +// like TRAINED_INPUT. +const TRAINED_DICTIONARY = Buffer.from( + 'N6Qw7PgzMzEZEOAKlQ7/////66r6nNxy7y2TZK30a621FgMDAAAAQ4n6AQAABAAA' + + 'gC1bQAgAAAAAAAAGAAAAiEgFCQEAGAAAAAAAAAAAAID1BQAAAAAAhK/FKg0AAAAA' + + 'AAAAAAAAAAAAAAEAAAAEAAAACAAAADEvaXRlbXMvMjYiLCJzdGF0dXMiOjIwMCwi' + + 'bXMiOjI0fQp7ImlkIjoyMDcsImxldmVsMS9pdGVtcy8xMyIsInN0YXR1cyI6MjAw' + + 'LCJtcyI6MTJ9CnsiaWQiOjEzNSwibGV2ZWwxL2l0ZW1zLzE4Iiwic3RhdHVzIjoy' + + 'MDAsIm1zIjo0Mn0KeyJpZA==', + 'base64' +); + +const TRAINED_INPUT = + '{"id":5,"level":"info","service":"api","route":"/v1/items/5","status":200,"ms":35}\n'; + +// The dictionary magic, then bytes that cannot parse as entropy tables. +const CORRUPT_TRAINED_DICTIONARY = Buffer.concat([ + Buffer.from([0x37, 0xa4, 0x30, 0xec]), + Buffer.alloc(60, 0xff), +]); + +export const zstdTrainedDictionaryTest = { + async test() { + const input = Buffer.from(TRAINED_INPUT); + const dictionary = TRAINED_DICTIONARY; + assert.strictEqual(dictionary.readUInt32LE(0), 0xec30a437); + const dictID = dictionary.readUInt32LE(4); + + const plain = zlib.zstdCompressSync(input); + const withDict = zlib.zstdCompressSync(input, { dictionary }); + assert( + withDict.length < plain.length, + `Trained dictionary should shrink the output, got ${withDict.length} with and ` + + `${plain.length} without` + ); + + // Bits 0-1 of the frame header descriptor give the size of the dictionary ID field, and + // 3 means four bytes. A raw-content dictionary would leave it at 0. + assert.strictEqual( + withDict[4] & 0b11, + 3, + 'Frame should carry a 4-byte dictionary ID' + ); + assert.strictEqual( + withDict.readUInt32LE(5), + dictID, + 'Frame should name the trained dictionary' + ); + + assert.strictEqual( + zlib.zstdDecompressSync(withDict, { dictionary }).toString(), + TRAINED_INPUT + ); + + const decompress = zlib.createZstdDecompress({ dictionary }); + decompress.end(withDict); + const chunks = []; + for await (const chunk of decompress) { + chunks.push(chunk); + } + assert.strictEqual(Buffer.concat(chunks).toString(), TRAINED_INPUT); + + // Because the frame names its dictionary, reading it without that dictionary is refused + // outright, with no checksum needed. Compare zstdDictionaryMismatchTest. + for (const options of [{}, { dictionary: DICTIONARY }]) { + assert.throws( + () => zlib.zstdDecompressSync(withDict, options), + /Dictionary mismatch/, + 'A frame naming a dictionary should be refused without it' + ); + } + }, +}; + +// The decoder parses a trained dictionary when it is loaded, so a corrupt one fails before +// any data is read. Node throws ERR_ZLIB_INITIALIZATION_FAILED from the constructor. +export const zstdCorruptDictionaryDecoderTest = { + test() { + const frame = zlib.zstdCompressSync(Buffer.from(TRAINED_INPUT)); + const dictionary = CORRUPT_TRAINED_DICTIONARY; + + assert.throws( + () => zlib.zstdDecompressSync(frame, { dictionary }), + /Failed to load zstd dictionary/, + 'The fast path should report the dictionary' + ); + assert.throws( + () => zlib.createZstdDecompress({ dictionary }), + { code: 'ERR_ZLIB_INITIALIZATION_FAILED' }, + 'The stream constructor should throw' + ); + assert.throws( + () => zlib.zstdDecompressSync(frame, { dictionary, info: true }), + { code: 'ERR_ZLIB_INITIALIZATION_FAILED' }, + 'The engine path should throw the same way' + ); + }, +}; + +// The encoder defers loading until the first frame begins, so the same corrupt dictionary +// is accepted at construction and fails on the first write. zstd reports it as an allocation +// failure, because the CDict it tried to build came back null. Node sees the same message. +export const zstdCorruptDictionaryEncoderTest = { + async test() { + const input = Buffer.from(TRAINED_INPUT); + const dictionary = CORRUPT_TRAINED_DICTIONARY; + + assert.throws( + () => zlib.zstdCompressSync(input, { dictionary }), + /Allocation error/, + 'The fast path should fail when the frame begins' + ); + + const compress = zlib.createZstdCompress({ dictionary }); + const { promise, resolve, reject } = Promise.withResolvers(); + compress.on('error', resolve); + compress.on('end', () => reject(new Error('Stream should not finish'))); + compress.resume(); + compress.end(input); + const err = await promise; + assert.match(err.message, /Allocation error/); + }, +}; + +// pledgedSrcSize is set after the dictionary is loaded. A correct size must still round-trip, +// and a wrong one must still be enforced, which proves the size reached the encoder. The +// stream is used for the wrong size because it feeds zstd in more than one call: a single +// ZSTD_e_end call makes zstd replace the pledge with the real input size. +export const zstdDictionaryPledgedSrcSizeTest = { + async test() { + const input = Buffer.from(DICT_INPUT); + + const compressed = zlib.zstdCompressSync(input, { + dictionary: DICTIONARY, + pledgedSrcSize: input.length, + }); + assert.strictEqual( + zlib + .zstdDecompressSync(compressed, { dictionary: DICTIONARY }) + .toString(), + DICT_INPUT + ); + + const compress = zlib.createZstdCompress({ + dictionary: DICTIONARY, + pledgedSrcSize: input.length + 1, + }); + const { promise, resolve, reject } = Promise.withResolvers(); + compress.on('error', resolve); + compress.on('end', () => reject(new Error('Stream should not finish'))); + compress.resume(); + compress.end(input); + const err = await promise; + assert.match( + err.message, + /Src size is incorrect/, + 'A wrong pledgedSrcSize should be enforced alongside a dictionary' + ); + }, +}; From 321633533694662a91b9221203bbc1a0223d99b3 Mon Sep 17 00:00:00 2001 From: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:52:26 -0400 Subject: [PATCH 3/3] Throw ERR_INVALID_ARG_TYPE for a non-buffer zstd dictionary Node stopped ignoring a zstd dictionary that is neither an ArrayBufferView nor an ArrayBuffer in nodejs/node#65867, first released in v26.10.0. It now throws ERR_INVALID_ARG_TYPE from every entry point, null included, and the async functions throw instead of calling back. Follow it. The replacement test mirrors the one that PR added to test/parallel/test-zlib-zstd-dictionary.js. It passes under Node v26.10.0 and fails under v26.9.0, which still ignored the option. --- src/node/internal/internal_zlib_base.ts | 13 ++++-- src/node/internal/zlib.d.ts | 2 +- .../api/node/tests/zlib-zstd-nodejs-test.js | 42 ++++++++++++------- 3 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/node/internal/internal_zlib_base.ts b/src/node/internal/internal_zlib_base.ts index a653a483580..85c598501aa 100644 --- a/src/node/internal/internal_zlib_base.ts +++ b/src/node/internal/internal_zlib_base.ts @@ -826,9 +826,10 @@ export const kMaxZstdDParam = Math.max( ); export const zstdInitDParamsArray = new Int32Array(kMaxZstdDParam + 1); -// Node accepts a zstd `dictionary` as an ArrayBufferView or an ArrayBuffer, and quietly -// ignores any other type rather than throwing (lib/zlib.js, class Zstd). Normalize here so -// that both the stream path and the convenience functions' fast path behave the same way. +// Node accepts a zstd `dictionary` as an ArrayBufferView or an ArrayBuffer, and throws +// ERR_INVALID_ARG_TYPE for any other type, null included (lib/zlib.js, class Zstd, since +// nodejs/node#65867; earlier releases ignored it). Normalize here so that both the stream +// path and the convenience functions' fast path behave the same way. export function normalizeZstdDictionary( dictionary: ZstdOptions['dictionary'] ): ArrayBufferView | undefined { @@ -838,7 +839,11 @@ export function normalizeZstdDictionary( if (isAnyArrayBuffer(dictionary)) { return new Uint8Array(dictionary); } - return undefined; + throw new ERR_INVALID_ARG_TYPE( + 'options.dictionary', + ['Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'], + dictionary + ); } // Returns `options` unchanged unless its dictionary needed normalizing, so the common case diff --git a/src/node/internal/zlib.d.ts b/src/node/internal/zlib.d.ts index 33b4feb1c88..5f2d54b438b 100644 --- a/src/node/internal/zlib.d.ts +++ b/src/node/internal/zlib.d.ts @@ -292,7 +292,7 @@ export interface ZstdOptions { maxOutputLength?: number | undefined; pledgedSrcSize?: number | undefined; // Declared as a view, like ZlibOptions above, though an ArrayBuffer is also accepted at - // runtime and any other type is ignored. See normalizeZstdDictionary() in + // runtime and any other type throws. See normalizeZstdDictionary() in // internal_zlib_base.ts. dictionary?: ArrayBufferView | undefined; // Not specified in NodeJS docs but the tests expect it diff --git a/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js b/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js index 6095e9891ed..e802470f96e 100644 --- a/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js +++ b/src/workerd/api/node/tests/zlib-zstd-nodejs-test.js @@ -555,24 +555,34 @@ export const zstdDictionaryTypesTest = { }, }; -// Node ignores a dictionary that is neither an ArrayBufferView nor an ArrayBuffer rather -// than throwing, on both the fast path and the stream path. -export const zstdDictionaryIgnoredTypeTest = { +// A dictionary that is neither an ArrayBufferView nor an ArrayBuffer is a TypeError on every +// entry point, and the async functions throw rather than calling back. Mirrors Node's +// test/parallel/test-zlib-zstd-dictionary.js. +export const zstdDictionaryInvalidTypeTest = { test() { const input = Buffer.from(DICT_INPUT); - const plain = zlib.zstdCompressSync(input).length; - - assert.strictEqual( - zlib.zstdCompressSync(input, { dictionary: 'not a buffer' }).length, - plain, - 'A non-buffer dictionary should be ignored' - ); - assert.strictEqual( - zlib.zstdCompressSync(input, { dictionary: 'not a buffer', info: true }) - .buffer.length, - plain, - 'A non-buffer dictionary should be ignored on the stream path too' - ); + const expected = { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError' }; + const mustNotCall = () => assert.fail('The callback should not be called'); + + for (const dictionary of [null, 'string', 123, true, {}, [1, 2, 3]]) { + const options = { dictionary }; + assert.throws(() => zlib.createZstdCompress(options), expected); + assert.throws(() => zlib.createZstdDecompress(options), expected); + assert.throws(() => zlib.zstdCompressSync(input, options), expected); + assert.throws(() => zlib.zstdDecompressSync(input, options), expected); + assert.throws( + () => zlib.zstdCompressSync(input, { ...options, info: true }), + expected + ); + assert.throws( + () => zlib.zstdCompress(input, options, mustNotCall), + expected + ); + assert.throws( + () => zlib.zstdDecompress(input, options, mustNotCall), + expected + ); + } }, };