-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ext/node/ops/zlib/brotli): Allow decompressing more than 4096 bytes #20301
fix(ext/node/ops/zlib/brotli): Allow decompressing more than 4096 bytes #20301
Conversation
…ae/deno into fix-4096-limit-brotli-decompress
To project maintainers: I haven't been able to test this locally. There is some sort of issue where my EDIT: nevermind, I see that they are git submodules. |
LGTM!
Yes, thats why this module was written using the C API. We can use the Rust API for decompression but compression has a lot more flags https://nodejs.org/api/zlib.html#class-brotlioptions and no way to pass them using the Rust API. EDIT: there are compression options https://docs.rs/brotli/latest/brotli/enc/backward_references/struct.BrotliEncoderParams.html |
@littledivy okay cool, thank you for explaining those decisions! Would it be of value to the project if I created an adapter layer here, and in the result remove some |
What needs to happen for us to merge this branch? I'm working on a project right now that's at a complete standstill without a working brotli implementation in Deno 😣 |
Thanks for the ping @nberlette |
…es (denoland#20301) Fixes denoland#19816 In that issue, I suggest switching over the other brotli functionality to the Rust API provided by the `brotli` crate. Here, I only do that with the `brotli_decompress` function to fix the bug with buffers longer than 4096 bytes.
See issue #19816
In that issue, I suggest switching over the other brotli functionality to the Rust API provided by the
brotli
crate. Here, I only do that with thebrotli_decompress
function to fix the bug with buffers longer than 4096 bytes.A good follow-up might be to repeat this change throughout the rest of the module. That will involve some actual work though, because the Node.js Brotli API is presumably based on the C
google/brotli
API, so an adapter layer in this module to map a Node.js Brotli API to the Rust API would end up being a little more complicated.