Tests: Make ASAN build usable - #73
Open
asonje wants to merge 6 commits into
Open
Conversation
DestroyBlock() called free(), which is right for the four Generate*Block() producers but not for the buffer ZlibUncompress() returns, and nine call sites released one of those through it. On glibc the crossing is invisible; ASAN tracks the allocator per block and halts the process on alloc-dealloc-mismatch, part way through the suite, so -DASAN=ON could not be used to check anything. Converge on new[]/delete[] rather than on malloc/free: it moves the four producers and the one destroyer, leaves every call site alone, and afterwards every buffer the suite hands out is new[] memory, so both spellings of release are correct and the mismatch is unrepresentable rather than merely absent. malloc/free would have been the other direction -- five producers plus seventeen delete[] sites plus the fuzzer, which releases a ZlibUncompress() buffer of its own. std::nothrow is load-bearing: plain new throws where malloc returns null, and each producer's "if (!buf) return nullptr" feeds callers that assert on a null pointer. GenerateZeroBlock() keeps calloc's zeroing through the trailing (), which is a silent behaviour change if dropped rather than a compile error. No new test. The suite reaching its end under -DASAN=ON is the assertion; it aborts today, and reverting any one of these five edits brings the abort back. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
With the mismatch gone, ASAN reaches the end of the suite and LeakSanitizer reports two leaks, both in the stream-copy cases that copy onto a destination which already owns ISA-L state. The leaked block is the destination's own zlib state, allocated by the test's deflateInit2/inflateInit2. zlib's *Copy overwrites the destination z_stream wholesale, dropping the old state pointer without freeing it, so an initialized destination leaks -- reproduced against stock zlib 1.3 with no shim loaded, same allocation sizes. Not something the shim can fix, and not something these tests can avoid: a destination that owns ISA-L state is the condition under test. deflateEnd on a saved copy of the z_stream does not work either; it returns Z_STREAM_ERROR, because the state carries a back-pointer to the stream it was initialized with. So keep the pointer across the copy and hand it back to the same z_stream once the copy is finished with, which is the one address zlib will accept. The added ASSERT_NE pins the premise -- if a future zlib reused the destination's state instead of allocating a new one, the restore would be a double free rather than a leak fix, and this fails first. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
ZlibUncompress() allocated its output buffer before the inflate loop and returned early from inside it without releasing the buffer, so a caller that got an error status was handed a live pointer it had no reason to release -- and one it could not distinguish from the uninitialized pointer the inflateInit2 failure leaves behind. Nothing in the suite asks for that path, which is why ASAN does not report it; the fuzzer reaches it. Make the rule "nothing is owned unless this returns Z_STREAM_END": null the out-parameter on entry, and release the buffer on the loop's error return. The fuzzer's two early returns then need only the one that follows a successful uncompress, where a buffer really is owned. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
The last leak an all-backends ASAN run reports is a mutex QATzip's OSAL allocates once and never destroys, so it is still held at exit. It is a vendor allocation on a path the shim does not own, and it is the only thing standing between a full ASAN run and a zero exit status -- which is what makes every other leak visible as a failure rather than as one more line in a report nobody reads. Name it in tests/lsan.supp, one entry per vendor allocation with the library said out loud, and have the run target point LSAN_OPTIONS at the file when ASAN is on so the knowledge lives in the build rather than in a shell history. A leak in the shim or in the suite is a bug to fix, not an entry to add. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
GenerateZeroBlock() moved from calloc to new[] with a trailing (), and the () is what carries the zeroing across. Dropping it is not a compile error and no test noticed: every case that takes a zero block only round-trips it, and uninitialized bytes round-trip as well as zeros do, so the suite would keep passing while the sweep had quietly lost its most compressible payload. Assert the contents, which is the only thing in the suite that looks at a generated block rather than at what came back through it. The case draws no randomness, so it does not disturb the payload sequence every parameterized case shares. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
The entry said QATzip allocates it. It does not: osalMutexInit is a local symbol in libqat, the QAT driver's user-space library, and appears in neither libqatzip's symbol tables nor its source. QATzip reaches it during session setup, so the leak only shows up with USE_QAT, but the allocating code belongs to the driver -- and this file's own rule is that every entry names the library it came from. Say as well that the entry depends on libqat shipping unstripped. The symbol is local, so on a stripped driver the frame has no name to match and the leak returns as a failure on a host where nothing about this tree changed. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Makes ASAN test runs usable by standardizing buffer ownership and cleaning leaks.
Changes:
- Standardizes test allocations on
new[]/delete[]. - Adds cleanup for decompression and fuzzing paths.
- Adds LSan suppression and regression coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/zlib_accel_test.cpp |
Standardizes allocation and adds regression tests. |
tests/test_utils.cpp |
Cleans decompression buffers on returns. |
tests/lsan.supp |
Suppresses the vendor QAT mutex leak. |
tests/CMakeLists.txt |
Applies LSan suppressions to ASAN runs. |
fuzzing/zlib_accel_fuzz.cpp |
Releases decompressed fuzz buffers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+88
to
+89
| delete[] *uncompressed; | ||
| *uncompressed = nullptr; |
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.
Makes -DASAN=ON usable: the test suite now allocates and releases every buffer with one allocator, so ASAN no longer halts on alloc-dealloc-mismatch part way through the run and the three leaks that abort had been hiding are fixed