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

[io] Validate ZSTD target/source sizes and compr level #15038

Closed
wants to merge 1 commit into from

Conversation

dpiparo
Copy link
Member

@dpiparo dpiparo commented Mar 24, 2024

This PR fixes #9334

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@dpiparo
Copy link
Member Author

dpiparo commented Mar 24, 2024

I found line https://github.com/root-project/root/pull/15038/files#diff-c35e46fbb246941a30fd3fa261506e5eb03b601e3bf6734460f814fd3aa156c9R46 intriguing. We are doubling the compression level given in input. The ZSTD documentation does not say anything which could possibly justify this behaviour https://facebook.github.io/zstd/zstd_manual.html . This deserves a bit of caution/discussion I believe, also given the test of the compression level which has to be coherent with that.

Copy link

Test Results

    12 files      12 suites   1d 23h 50m 36s ⏱️
 2 603 tests  2 603 ✅ 0 💤 0 ❌
29 185 runs  29 185 ✅ 0 💤 0 ❌

Results for commit 5058f06.

@hahnjo
Copy link
Member

hahnjo commented Mar 25, 2024

I found line https://github.com/root-project/root/pull/15038/files#diff-c35e46fbb246941a30fd3fa261506e5eb03b601e3bf6734460f814fd3aa156c9R46 intriguing. We are doubling the compression level given in input. The ZSTD documentation does not say anything which could possibly justify this behaviour https://facebook.github.io/zstd/zstd_manual.html . This deserves a bit of caution/discussion I believe, also given the test of the compression level which has to be coherent with that.

From the linked manual:

The library supports regular compression levels from 1 up to ZSTD_maxCLevel(),
which is currently 22. Levels >= 20, labeled --ultra, should be used with
caution, as they require more memory. The library also offers negative
compression levels, which extend the range of speed vs. ratio preferences.
The lower the level, the faster the speed (at the cost of compression).

So the doubling maps ROOT's single digit layer to the "reasonable" level 0 to 18.

@hahnjo
Copy link
Member

hahnjo commented Nov 7, 2024

As mentioned inline, the check of tgtsize is not needed since 23261a6. The check for *srcsize > 0xffffff should likely be moved to the upper layer with proper error reporting (not just silently returning).

@dpiparo dpiparo closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of size validation in ZSTD compression
4 participants