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

When decompressing truncated input, calling ZSTD_decompressStream() with input = {NULL, 0, 0} computes NULL + 0, which is UB #3351

Closed
QrczakMK opened this issue Dec 14, 2022 · 1 comment · Fixed by #3356
Assignees

Comments

@QrczakMK
Copy link

Describe the bug
At least in certain circumstances, when decompressing truncated compressed input, calling ZSTD_decompressStream() with input = {NULL, 0, 0} computes NULL + 0 here:

ip += loadedSize;

which is UB in C, detected by ubsan (nullptr-with-offset), even though it is benign in practice.

To Reproduce
I could try extracting a test case, but I would prefer to avoid unnecessary work. The fix should be uncontroversial.

Expected behavior
Decompression successfully stops wanting more input, even under ubsan.

Possible fix:

--- a/google3/third_party/zstdlib/decompress/zstd_decompress.c
+++ b/google3/third_party/zstdlib/decompress/zstd_decompress.c
@@ -2166,8 +2166,10 @@ size_t ZSTD_decompressStream(ZSTD_DStrea
                                     "should never happen");
                     loadedSize = ZSTD_limitCopy(zds->inBuff + zds->inPos, toLoad, ip, (size_t)(iend-ip));
                 }
-                ip += loadedSize;
-                zds->inPos += loadedSize;
+                if (loadedSize != 0) {
+                    ip += loadedSize;
+                    zds->inPos += loadedSize;
+                }
                 if (loadedSize < toLoad) { someMoreWork = 0; break; }   /* not enough input, wait for more */
 
                 /* decode loaded input */

Our randomized tests succeed with this fix many times, which suggests that this is the only place to fix for this issue.

@terrelln terrelln self-assigned this Dec 15, 2022
terrelln added a commit to terrelln/zstd that referenced this issue Dec 15, 2022
Fix an instance of `NULL + 0` in `ZSTD_decompressStream()`. Also, improve our
`stream_decompress` fuzzer to pass `NULL` in/out buffers to
`ZSTD_decompressStream()`, and fix 2 issues that were immediately surfaced.

Fixes facebook#3351
@terrelln
Copy link
Contributor

Thanks for the report @QrczakMK! Should be fixed after PR #3356 is merged.

terrelln added a commit that referenced this issue Dec 15, 2022
Fix an instance of `NULL + 0` in `ZSTD_decompressStream()`. Also, improve our
`stream_decompress` fuzzer to pass `NULL` in/out buffers to
`ZSTD_decompressStream()`, and fix 2 issues that were immediately surfaced.

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

Successfully merging a pull request may close this issue.

2 participants