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.

Sign up for GitHub

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

Jump to bottom

check potential overflow of compressBound() #3362

Merged
merged 2 commits into from
Dec 16, 2022
Merged

check potential overflow of compressBound() #3362

merged 2 commits into from
Dec 16, 2022

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Dec 15, 2022 •
edited
Loading

fixed #3323, reported by @nigeltao

Completed documentation around overflow risk
(Note : This scenario probably can't happen in a "valid" situation
where srcSize is the real size of a really allocated and used buffer,
but a bogus srcSize value could indeed trigger it).

@Cyan4973
check potential overflow of compressBound()
45ed0df 
fixed #3323, reported by @nigeltao

Completed documentation around this risk
(which is largely theoretical,
I can't see that happening in any "real world" scenario,
but an erroneous @srcSize value could indeed trigger it).
terrelln
terrelln requested changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to fix ZSTD_decompressBound() to not overflow in 32-bit mode.

Can you also add a test for ZSTD_compressBound() and ZSTD_COMPRESSBOUND() for too-large sources?

lib/zstd.h Outdated
Comment on lines 207 to 209
* providing @dstCapacity >= ZSTD_compressBound(srcSize) guarantees success.
* Note that it's still allowed to provide a smaller @dstCapacity value,
* in which case, the caller must inspect the return value with ZSTD_isError(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might weaken this statement a little it. It reads like you don't need to check ZSTD_isError() if you provide dstCapacity >= ZSTD_compressBound(srcSize). Which definitely isn't true.

@Cyan4973
Copy link
Contributor Author

We also need to fix ZSTD_decompressBound() to not overflow in 32-bit mode.

I'll do this one in a separate PR.

@Cyan4973
added unit tests for compressBound()
97f63ce 
and rephrased the code documentation, as suggested by @terrelln
@Cyan4973
Copy link
Contributor Author

Added test

@Cyan4973 Cyan4973 merged commit 51355e1 into dev Dec 16, 2022
@Cyan4973 Cyan4973 mentioned this pull request Dec 16, 2022
Merged
@Cyan4973 Cyan4973 deleted the compressBound branch January 13, 2023 04:27
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrelln terrelln terrelln approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ZSTD_compressBound can silently overflow
3 participants
@Cyan4973 @terrelln @facebook-github-bot