Tests: Make ASAN build usable - #73
Open
asonje wants to merge 9 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; |
ZlibUncompress() leaves its inflate loop on three conditions, and one of them -- Z_OK on the last chunk -- is not a failure. It means the input held less than a whole stream, which is exactly what CompressDecompressPartialStream asks for and then asserts on. Releasing the buffer there was wrong in kind, and it left the test comparing against a null pointer. The buffer was never the reachable half of the problem, though. The lengths were not populated on that return either, so uncompressed_length stayed 0 and the test's memcmp compared zero bytes -- true on main long before this branch, which is why nothing caught it. Corrupting one byte of the partial output now fails 864 of the 1152 cases in that suite; before, all 1152 passed regardless. The remaining 288 are the Z_DATA_ERROR parameterizations, which never reach the compare. So report the size and hand the prefix over on partial progress, and delete only for the two conditions that really are errors. Naming the three conditions is what makes the asymmetry legible; as one flat predicate it reads as though all three mean the same thing. The fuzzer's early return now releases unconditionally, since a Z_OK return owns a buffer and delete[] on the null an error leaves is a no-op. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
matt-welch
self-requested a review
September 2, 2026 17:16
matt-welch
requested changes
Sep 2, 2026
matt-welch
left a comment
matt-welch
left a comment
There was a problem hiding this comment.
PR #73 review
The individual commits are well-motivated and the fixes are correct. The non-ASAN test suite runs clean (12875/12875). Two things need to be addressed before the ASAN run actually works end-to-end.
tests/CMakeLists.txt, line 30
The run target passes LSAN_OPTIONS but not ASAN_OPTIONS, so make run in the ASAN build directory aborts before running a single test:
ERROR: AddressSanitizer: odr-violation
[1] size=16 '__tag' .../bits/shared_ptr_base.h:566 in zlib_accel_test
[2] size=16 '__tag' .../bits/shared_ptr_base.h:566 in libzlib-accel.so
ABORTING
Both the test binary and the shared library include <memory>, which defines an internal STL type-tag global. ASAN flags the two copies as an ODR violation even though they are identical. The fix is ASAN_OPTIONS=detect_odr_violation=0:
if(ASAN)
add_custom_target(run
COMMAND ${CMAKE_COMMAND} -E env
ASAN_OPTIONS=detect_odr_violation=0
LSAN_OPTIONS=suppressions=${CMAKE_CURRENT_SOURCE_DIR}/lsan.supp
./zlib_accel_test
DEPENDS zlib_accel_test
)Reproduced by running ./zlib_accel_test bare in the ASAN build directory. Running with ASAN_OPTIONS=detect_odr_violation=0 lets all 12875 tests pass.
config/config.cpp, line 14
Once the ODR issue is worked around, LSAN fires at exit and the run still exits 1:
Direct leak of 20 byte(s) in 1 object(s) allocated from:
#0 operator new
#1 std::string::operator=
#2 ConfigReader::GetValue config_reader.cpp:69
#3 config::LoadConfigFile config.cpp:104
#4 init_zlib_accel zlib_accel.cpp:247
#5 libzlib-accel.so constructor
config::log_file starts as "" (SSO, no heap). When the library constructor reads a config file that sets log_file to a path longer than 15 bytes, the assignment in GetValue triggers a heap allocation. That allocation is never freed: shared-library global destructors run after LeakSanitizer's exit check, so the std::string destructor never releases it.
The reproducer is any machine with /etc/zlib-accel.conf containing log_file = /some/path/longer/than/15/chars. Here it is log_file = /tmp/zlib-accel.log (21 chars).
lsan.supp says a leak in the shim is a bug to fix, not a suppression to add. The simplest fix is a library destructor that clears the string before LSAN scans at exit:
// config.cpp
__attribute__((destructor)) static void release_log_file() {
std::string().swap(log_file);
}swap with a default-constructed string guarantees the heap buffer is released even if the compiler elides it for a plain = "" assignment.
Everything else looks good. The allocator convergence, the ZlibUncompress ownership fix, and the stream-copy orphaned-state recovery are all correct. The Z_OK partial-decode commit (the last one) is a nice catch: CompressDecompressPartialStream was comparing zero bytes before, so all 1152 cases passed unconditionally regardless of the output.
-DASAN=ON aborts before the first test on some toolchains with an ODR violation on std::_Sp_make_shared_tag::_S_ti()::__tag and on std::piecewise_construct. Both are false positives. libstdc++ wraps namespace std in _GLIBCXX_VISIBILITY(default), so -fvisibility=hidden does not hide the std globals that the test binary and libzlib-accel.so each instantiate; where a linker binds the two copies to one address, ASAN sees one global registered twice. The definitions are identical and both objects legitimately use std::shared_ptr, so there is nothing in this tree to fix. Whether it fires is a property of the link, which is why the abort reproduces for some builds and not others. gcc 13 detects the duplicate through an ODR indicator byte, and here libzlib-accel.so exports that byte while the test binary does not, so each module flips its own and neither sees the other's. Relinking the test binary with -Wl,--export-dynamic reproduces the report exactly, and that is what this file was tested against. Name the two globals rather than switching ODR detection off, so that a third duplicate still aborts by name. Both entries are load-bearing: suppressing only __tag leaves the run aborting on piecewise_construct. ASAN matches on the bare global name, so an entry cannot be scoped narrower than that. Compile them in through the runtime's own weak hook rather than passing ASAN_OPTIONS from the run target, because a bare ./zlib_accel_test has to work too -- that is how the abort was reported in the first place. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
With a log_file set in /etc/zlib-accel.conf, the library constructor leaked the path -- a 26-byte direct leak under LeakSanitizer, reported through ConfigReader::GetValue. The global is assigned before its own initializer runs. -flto merges every translation unit's dynamic initialization into one function, and the order it picks here is _GLOBAL__sub_I_zlib_accel.cpp, which runs init_zlib_accel() and assigns config::log_file, and only then _GLOBAL__sub_I_config.cpp, whose `std::string log_file = ""` overwrites the pointer and orphans the buffer. The leak therefore happens during startup, not at exit, and clearing the global from a destructor would find it already empty. The second consequence is the worse one: config::log_file reads as "" for the rest of the process, so any future reader gets nothing. Nothing needs it to be a global. It is written once in LoadConfigFile and read three lines later in the same function; CreateLogFile copies the name into an ofstream; and it is not VISIBLE_FOR_TESTING, so -fvisibility=hidden keeps it out of the dynamic symbol table and nothing outside the library can reach it. So hand it back through an out-parameter and let the constructor own the string, which removes the ordering hazard instead of working around it. When something does want the path after startup, the place for it is beside LogFileStream() in logging.h, which is a function-local static and immune by construction. The new ConfigLoaderTest covers what the out-parameter makes testable: the path comes back from a config file that names one, and a config file that names none leaves the caller's string alone. The ordering bug itself is not unit-testable, since it needs the constructor reading /etc/zlib-accel.conf; the guard against it is that there is no longer a global to clobber. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
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