Fix undefined behavior in ZSTD_decompressStream() #3258
Conversation
lib/decompress/zstd_decompress.c
Outdated
| @@ -2059,7 +2059,7 @@ size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* output, ZSTD_inB | |||
| if (ZSTD_isError(decompressedSize)) return decompressedSize; | |||
| DEBUGLOG(4, "shortcut to single-pass ZSTD_decompress_usingDDict()") | |||
| ip = istart + cSize; | |||
| op += decompressedSize; | |||
| op = op ? op + decompressedSize : op; | |||
There was a problem hiding this comment.
Detailing the thought process :
That such a scenario can even be triggered in ubsan tests feels strange initially:
for execution to reach this code position, it needs to pass the test :
(oend-op) >= zds->fParams.frameContentSize.
So, if there is any content, then it's required that op != NULL.
However, thinking about edge cases,
it's indeed possible that frameContentSize==0.
This is an empty frame (yet still a valid frame).
It's obviously not an expected outcome in general, the point is that it can happen, it's allowed.
In which case, it becomes technically possible to "decompress" such an empty frame into NULL.
Now, there are more philosophical questions associated to this scenario :
Is it normal to ask to decompress a frame into a NULL buffer ?
Should it be an instant fail ? or should it succeed in the specific case where frameContentSize==0 ?
Note that this code block is just an "optimization" : it triggers single-pass mode when space conditions are met, which requires less workload than full-featured streaming mode.
Which means that skipping this block is not an "end" sentence : it just requires to employ the more complex streaming mode.
Therefore, one possible idea here could be to add a op != NULL condition to enter this code block,
so that op==NULL is no longer a possible scenario to support inside the code block
(it must employ the more general streaming code path).
However, such a solution does not appear "obviously" better than the ternary operator you selected.
I would say, the only piece of information missing is why the scenario (op==NULL) can technically happen here, justifying the existence of this ternary operator to any future contributor reviewing the code.
There was a problem hiding this comment.
Agree that a very brief comment would be useful here.
lib/decompress/zstd_decompress.c
Outdated
| @@ -2059,7 +2059,7 @@ size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* output, ZSTD_inB | |||
| if (ZSTD_isError(decompressedSize)) return decompressedSize; | |||
| DEBUGLOG(4, "shortcut to single-pass ZSTD_decompress_usingDDict()") | |||
| ip = istart + cSize; | |||
| op += decompressedSize; | |||
| op = op ? op + decompressedSize : op; | |||
There was a problem hiding this comment.
Agree that a very brief comment would be useful here.
daniellerozenblit
force-pushed
the
null-buffer-decompress
branch
from
28c4951 to
a06e953
Compare
TLDR
This PR fixes two instances of undefined behavior (0 offset to Null pointer) in ZSTD_decompressStream(). Also it adds some additional clang ubsan CI jobs, in addition to existing gcc ubsan jobs.
Reproduce Bug
Run
CC=clang make uasan-test-zstreamwith clang11 or higher. Make sure to add the test case that was added in this PR (in tests/zstreamtest.c) since it exposes one of these errors.Benchmarking
This bug fix does not seem to negatively impact decompression performance.
I benchmarked on an Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz machine with core isolation and turbo disabled. I measured decompression time for silesia.tar (203M), compiling with GCC11 and clang15. I ran each scenario 5 times interleaved to eliminate bias from background processes.
Clang Results
original: 0m0.472s = 430M/s
this PR: 0m0.469s = 433M/s
This PR increased compression speed by around 0.698%, probably due to random noise.
GCC Results
original: 0m0.466s = 436M/s
this PR: 0m0.466s = 436M/s
This PR did not change compression speed (0.00% decrease).