[benchmark] advance the decodeOnly result cursor by the decompressed size - #4728
Open
evilgensec wants to merge 1 commit into
Open
[benchmark] advance the decodeOnly result cursor by the decompressed size#4728evilgensec wants to merge 1 commit into
evilgensec wants to merge 1 commit into
Conversation
…size In BMK_benchMemAdvancedNoAlloc the result buffer is allocated at the total decompressed size of all inputs, and resSizes[chunkID] is set per chunk to ZSTD_findDecompressedSize() in decodeOnly mode. The write cursor, however, advanced by chunkSize, which in decodeOnly mode is the compressed size of the file rather than its decompressed size. Chunk i therefore received a destination pointer at the sum of the preceding compressed sizes inside a buffer sized by the sum of the decompressed sizes. Whenever an input compresses to more bytes than it decompresses to, the cursor runs ahead of the allocation and later chunks are handed a dst that lies outside it. A skippable frame is the simplest case, since it costs compressed bytes and contributes nothing to the decompressed size. Advance by resSizes[chunkID] instead. It is assigned three lines earlier, so it is already available. In non-decodeOnly mode resSizes[chunkID] is exactly chunkSize, which makes this a no-op there. In decodeOnly mode it makes the sum of the per-chunk sizes equal the decodedSize passed to malloc, because line 514 issues the same ZSTD_findDecompressedSize call over the same ranges as the validation loop above.
There was a problem hiding this comment.
Pull request overview
Fixes an out-of-bounds destination pointer bug in the zstd -b -d (decode-only benchmark) path by advancing the result-buffer cursor using the decompressed chunk size rather than the compressed chunk size. This aligns per-chunk destination offsets with the buffer allocation logic (which is based on total decompressed size) and prevents heap buffer overflows when benchmarking multiple input files.
Changes:
- Advance
resPtrbyresSizes[chunkID](decompressed size in decode-only mode, unchanged behavior in other modes).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One line fix in
programs/benchzstd.c. Inzstd -b -dmode the result buffer write cursor advances by each input compressed size while the buffer is allocated at the total decompressed size, so multi file decode benchmarks can handZSTD_decompressStreama destination that lies outside the allocation.Detail
BMK_benchMemAdvancedNoAllocallocates the result buffer at line 480:where
decodedSizeis the sum ofZSTD_findDecompressedSize()over the inputs.The per chunk loop then sets the size correctly but advances the cursor with the wrong quantity:
In
BMK_decodeOnlymodechunkSizeisfileSizes[fileNb], the compressed size. So chunkigets a destination at the sum of the preceding compressed sizes inside a buffer sized by the sum of the decompressed sizes.resPtrs[]andresSizes[]go straight toZSTD_decompressStreamasout.dstandout.size.Whenever an input compresses to more bytes than it decompresses to, the cursor runs ahead of the allocation. The existing guards, the
CONTENTSIZE_UNKNOWNandCONTENTSIZE_ERRORchecks at 455 to 466 and thesize_toverflow check at 474, only validate the total. They never check the per chunk cursor.A skippable frame is the simplest way to get compressed larger than decompressed, since it costs compressed bytes and contributes zero to the decompressed size.
Reproduction
Two ordinary files, the first carrying a 4096 byte skippable frame ahead of a frame that decompresses to 16 bytes:
That gives compressed 4125 and decompressed 16 for
file0.zst, and compressed 280 and decompressed 2048 forfile1.zst. The buffer ismalloc(16 + 2048)which is 2064, and chunk 1 is handed a 2048 byte destination at offset 4125.Build with a sanitizer and run the benchmark:
On
devthis reports:benchfn.c:116is thememset(dstBuffers[i], 0xE5, dstCapacities[i])that prewarms the destination. It faults first on the same out of bounds region thatZSTD_decompressStreamwould then write.The fix
Advance by
resSizes[chunkID], which is assigned three lines earlier and so is already available.In non
decodeOnlymoderesSizes[chunkID]is exactlychunkSize, which makes the change a provable no op there. IndecodeOnlymode it makes the sum of the per chunk sizes equal thedecodedSizepassed tomalloc, because line 514 issues the sameZSTD_findDecompressedSizecall over the same ranges as the validation loop above it.Verification
With the change applied, the same command completes with no sanitizer report:
Regression checked, all clean and all under ASAN:
Note
This path has no OSS-Fuzz coverage. Nothing under
ossfuzz/linksprograms/benchzstd.c, which is why the arithmetic has survived. Reachable from the CLI aszstd -b -dover more than one file, and fromBMK_benchFilesorBMK_benchFilesAdvancedwithnbFiles > 1andmode = BMK_decodeOnly.CLA is already on file from earlier contributions.