-
Notifications
You must be signed in to change notification settings - Fork 168
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
Limit zstd window size to web-safe value #490
Conversation
zstd's memory buffer is limited to 8MB by Chromium and Firefox (at least), but the encoder can exceed that at level 20 and above. Ensure it's limited to 8MB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks reasonable to me
This comment was marked as resolved.
This comment was marked as resolved.
Previous comment is now resolved, but there is another thing. The documentation for
Since it says "in bytes", it seems like one would have to pass |
The docs are unclear, but it means that "expressed as a power of 2". Also the enum docs:
And the maximum possible setting is 31. I think I can write a test for this, but I'm not sure I can do it without pulling |
Alright, given the explanation I think we can merge as-is, though having a test before releasing this would be good. |
|
Also bump the version of the zstd dev dependency.
Added a test, which fails before the fix and passes after it. Also bumped the |
When will a new version be released? Looking forward to this fix |
v0.6.0 was released with this 2w ago. |
zstd's memory buffer is limited to 8MB by Chromium and Firefox (at least), but the encoder can exceed that at level 20 and above. This ensures it's limited to 8MB.
See zstd issue 2712, RFC 8878, Chrome bug 41493659.
To verify, try this:
Cargo.toml:
main.rs:
Accessing
http://127.0.0.1:6612/zero
in recent Firefox/Chrome versions will trigger an error with tower-http 0.5.2, but works with this PR.Probably fixes #488. It should, but I haven't tested the software involved.
The window size is set for levels >=17, rather than >=20, in case
zstd
ever changes to have larger window sizes at lower presets for some reason, in the hopes that this will cover that too. (I don't think it's very likely that zstd will do that, but this shouldn't do anything at all for presets 17..=19, so it's not harmful.)