Aware of compression ratio for unpack size limit #11337
Conversation
|
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
weihanglo
force-pushed
the
compression-ratio
branch
from
3cc4509
to
9d896cb
Compare
| fn max_unpack_size(size: u64) -> u64 { | ||
| const SIZE_VAR: &str = "__CARGO_TEST_MAX_UNPACK_SIZE"; | ||
| const RATIO_VAR: &str = "__CARGO_TEST_MAX_UNPACK_RATIO"; | ||
| let max_unpack_size = if cfg!(debug_assertions) && std::env::var(SIZE_VAR).is_ok() { |
There was a problem hiding this comment.
Does the CFG mean that the test suite will only pass in debug mode?
There was a problem hiding this comment.
Yep, unfortunately.
If SIZE_VAR were exposed, this PR would become unnecessary 😆.
cargo/tests/testsuite/registry.rs
Lines 2734 to 2737 in 9286a1b
weihanglo
force-pushed
the
compression-ratio
branch
from
9d896cb
to
de7cd31
Compare
|
Now that we have a ratio limit, why do we still have the MB limit? Especially given that we take the max of the two? |
It's possible that some small crate contains a small amount of highly compressible data -- and that should be allowed. For example, if a 30KB crate compresses to 1K, we don't want to block that. |
|
Friendly ping @joshtriplett, as you're the original author of that CVE fix. Do you think this enhancement is safe enough in terms of security? |
|
Discussed this in today's meeting. Looks good, thanks! @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306 2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000 - Refactor generate_targets into separate module (rust-lang/cargo#11445) - Improve file found in multiple build targets warning (rust-lang/cargo#11299) - Error when precise without -p flag (rust-lang/cargo#11349) - Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430) - Aware of compression ratio for unpack size limit (rust-lang/cargo#11337) - Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403) - Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400) - Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387) - Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
Update cargo 9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306 2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000 - Refactor generate_targets into separate module (rust-lang/cargo#11445) - Improve file found in multiple build targets warning (rust-lang/cargo#11299) - Error when precise without -p flag (rust-lang/cargo#11349) - Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430) - Aware of compression ratio for unpack size limit (rust-lang/cargo#11337) - Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403) - Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400) - Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387) - Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
What does this PR try to resolve?
Cargo now is aware of the compression ratio and pick the larger one between
.cratefile times a fixed compression ratio (20:1)to determine the unpack size limit of a compressed
.cratefile.How should we test and review this PR?
Get a debug build and tweak values of
__CARGO_TEST_MAX_UNPACK_SIZEand__CARGO_TEST_MAX_UNPACK_RATIOif you want to manually test it.Additional information
I've heard of #11151 and other use case hitting the hard limit 512MiB after we set it. Weeks ago I posted a topic on Zulip to discuss adding
registries.<regname>.max-unpack-sizeto configure the limit. After some investigations, I felt like we can first simply add a hard ratio as @arlosi suggested, without stacking up the code complexity.A ratio of 20:1 should fit most cases in general. The ratio should cover other algorithms with higher compression ratio, such as lzma/xz and bzip2. I've listed a couple of references in the doc comment of
fn max_unpack_size(…).Here is data from Cargo's dependencies (size in bytes):
Expand to see raw data