Honour the zstd dictionary option in node:zlib - #7106
Open
oddharsh wants to merge 1 commit into
Open
Conversation
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 cloudflare#6967
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6967.
node:zlib's zstd functions accept adictionaryoption and drop it. On compress that issilent, because a frame compressed without a dictionary decodes fine with one, so the only
signal is a byte count that never shrank. On decompress it surfaces as
Zstd decompression failed: Data corruption detectedagainst bytes that are not corrupt.This wires
ZSTD_CCtx_loadDictionary/ZSTD_DCtx_loadDictionaryinto the bindings from#6007, following Node's own implementation
(nodejs/node@201304537e, nodejs/node#59240).
What changed
C++.
ZstdContext::Optionsgainsdictionary, and bothinitialize()overloads take akj::ArrayPtr<const kj::byte>whose empty case means no dictionary, which is how Node passesthe same thing. A load failure returns
ERR_ZLIB_DICTIONARY_LOAD_FAILED, matching Node's errorcode. The option reaches the convenience functions through
zstdSync()and the streams throughZstdCompressionStream::initialize().TypeScript. One helper normalizes the option for both the stream class and the four
convenience functions, so the fast path and the
info: truepath agree.Three details worth a reviewer's eye
Parameter order.
ZSTD_CCtx_loadDictionarydefers building the dictionary's tables untilthe first frame begins, so the
setParamsloop that runs afterinitialize()still applies tothem. Measured against zstd 1.5.7 with this exact call order (create, load dictionary, set
level,
ZSTD_compressStream2) on the issue's 8800-byte input: 63 bytes with no dictionary, 19with the right one, and 63 with a wrong one, which are the numbers Node produces.
zstdDictionaryCollapseTestpins it by round-tripping one dictionary at level 19 and again atlevel 1.
Dictionary content type. Both calls auto-detect: a buffer starting with the zstd dictionary
magic is read as a trained dictionary and anything else as raw content. That is what lets a
Worker participate in RFC 9842 Compression
Dictionary Transport, whose dictionaries are raw bytes, and it is the behaviour Node has.
Wrong types are ignored rather than rejected. Node accepts an ArrayBufferView or an
ArrayBuffer for zstd and quietly ignores any other type, unlike its zlib and brotli paths which
throw
ERR_INVALID_ARG_TYPE(lib/zlib.js, classZstd). I matched Node rather thantightening it, since the convenience functions hand the options object straight to JSG, which
would otherwise throw a
TypeErroron a value Node accepts silently. Say the word if you wouldrather have
ERR_INVALID_ARG_TYPEon both paths and I will change it.Tests
Seven cases added to
zlib-zstd-nodejs-test.js:test/parallel/test-zlib-zstd-dictionary.js)maintoo, since theoption is dropped there; it guards the behaviour rather than the fix)
createZstdCompress/createZstdDecompresshonour itThe mismatch test sets
ZSTD_c_checksumFlagdeliberately. A raw-content dictionary carries nodictionary ID, so without a checksum, decoding that frame against a different dictionary
returns 8800 bytes of wrong data rather than an error (measured). With the checksum it is
Restored data doesn't match checksum, and decoding with no dictionary at all isData corruption detectedeither way.What I ran, and what I could not
Ran and green:
//src/workerd/api:compression(the C++ carrying the zstd calls),//src/node:node@tsproject,//src/workerd/api/node/tests:zlib-zstd-nodejs-test@eslint, plusthe standalone zstd measurements above.
Could not run: the test target itself, and therefore the
zlib-util.c++plumbing. My machineonly has the beta Xcode, and its
ldproduces Rust proc-macro dylibs that the same machine'sdyld refuses to load (
mis-aligned LINKEDIT string pool), so every[for tool]Rust cratefails and the capnp codegen those tools drive cannot run. That is an Apple toolchain problem
rather than anything in this branch, and neither
-ld_classicnor re-signing the dylib got pastit. Flagging it rather than implying a full local run: if CI turns something up I will fix it
promptly.
Not in this PR
BrotliContext::Optionshas nodictionaryfield either, andBrotliCompressionStream::initialize()takes no dictionary argument, so brotli drops the optionthe same way. Node supports it there through
BrotliEncoderAttachPreparedDictionary. Happy tofollow up with that separately if you want it.