Skip to content
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

Merged
merged 2 commits into from
May 27, 2024

Conversation

AlyoshaVasilieva
Copy link
Contributor

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:

[package]
name = "bug-repro"
version = "0.1.0"
edition = "2021"

[dependencies]
anyhow = "1.0.86"
tokio = { version = "1.37", features = ["rt", "rt-multi-thread", "macros"] }
axum = "0.7.5"
#tower-http = { version = "0.5.2", features = ["compression-zstd"] }

[dependencies.tower-http]
git = "https://github.com/AlyoshaVasilieva/tower-http.git"
features = ["compression-zstd"]
rev = "a9575c0bf7098204098bc5ea7949342da9134c3f"

[profile.dev]
opt-level = 1

main.rs:

use std::net::{Ipv4Addr, SocketAddr};

use anyhow::Result;
use axum::routing::get;
use axum::Router;
use tower_http::compression::CompressionLayer;
use tower_http::CompressionLevel;

#[tokio::main]
async fn main() -> Result<()> {
    let router = Router::new()
        .route("/zero", get(zeroes))
        .layer(CompressionLayer::new().quality(CompressionLevel::Precise(20)));
    let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 6612);
    let listener = tokio::net::TcpListener::bind(addr).await?;
    axum::serve(listener, router.into_make_service()).await?;
    Ok(())
}

async fn zeroes() -> String {
    let zeroes = vec![b'0'; 33_554_432];
    String::from_utf8(zeroes).unwrap()
}

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.)

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.
Copy link
Collaborator

@Nehliin Nehliin left a 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

@jplatte

This comment was marked as resolved.

@jplatte
Copy link
Collaborator

jplatte commented May 25, 2024

Previous comment is now resolved, but there is another thing. The documentation for CParameter::window_log says

Window size in bytes (as a power of two)

Since it says "in bytes", it seems like one would have to pass 8388608 (2 ^ 23), not 23? Seems like it would be good to have a test, but maybe hard to do? I guess we should have one that shows that it works at all (i.e. you can make a request against a service using this middleware and decompress the body successfully).

@AlyoshaVasilieva
Copy link
Contributor Author

Since it says "in bytes", it seems like one would have to pass 8388608 (2 ^ 23), not 23?

The docs are unclear, but it means that 23 is interpreted as 2 ^ 23. The C docs are slightly clearer:

https://github.com/facebook/zstd/blob/0e2ceb2d5061f3a8357d124029ebaae16d915a3d/doc/zstd_manual.html#L253

"expressed as a power of 2".

Also the enum docs:

The actual distance is 2 power “this value”.

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 axum in as a dev dependency (been a while since I used any other HTTP server-type crate). The zstd crate has a window_log_max function on its decoder, so setting it to 23 and trying to decompress a response processed through a CompressionLayer set to maximum compression should work.

@jplatte
Copy link
Collaborator

jplatte commented May 25, 2024

Alright, given the explanation I think we can merge as-is, though having a test before releasing this would be good.

@jplatte
Copy link
Collaborator

jplatte commented May 25, 2024

cargo hack is properly broken though, need to fix that (in a separate PR) before merging as I also don't have the permission to bypass the checks.

@jplatte jplatte closed this May 26, 2024
@jplatte jplatte reopened this May 26, 2024
Also bump the version of the zstd dev dependency.
@AlyoshaVasilieva
Copy link
Contributor Author

Added a test, which fails before the fix and passes after it.

Also bumped the zstd dev dep to 0.13 just to keep up-to-date. It fixed 2 buffer overruns, though that doesn't really matter for test code.

@Nehliin Nehliin merged commit a1e3f8a into tower-rs:main May 27, 2024
11 checks passed
@i18nsite
Copy link

When will a new version be released? Looking forward to this fix

@jplatte
Copy link
Collaborator

jplatte commented Oct 2, 2024

v0.6.0 was released with this 2w ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug : chrome ERR_ZSTD_WINDOW_SIZE_TOO_BIG
4 participants