From de24e6829da85d9f3e2e1671a062b2687c768b19 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 27 Aug 2026 14:59:50 -0700 Subject: [PATCH 1/9] tests: release generated blocks with one allocator 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 --- tests/zlib_accel_test.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index c6a1dcb..eae0027 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -55,7 +56,7 @@ std::string GenerateRandomString(size_t length) { } char* GenerateCompressibleBlock(size_t length, int ratio = 4) { - char* buf = (char*)malloc(length); + char* buf = new (std::nothrow) char[length]; if (!buf) { return nullptr; } @@ -83,7 +84,7 @@ char* GenerateCompressibleBlock(size_t length, int ratio = 4) { } char* GenerateIncompressibleBlock(size_t length) { - char* buf = (char*)malloc(length); + char* buf = new (std::nothrow) char[length]; if (!buf) { return nullptr; } @@ -95,7 +96,9 @@ char* GenerateIncompressibleBlock(size_t length) { } char* GenerateZeroBlock(size_t length) { - char* buf = (char*)calloc(length, sizeof(char)); + // The () is what calloc's zeroing becomes; without it the block is + // uninitialized and nothing here would say so. + char* buf = new (std::nothrow) char[length](); if (!buf) { return nullptr; } @@ -129,7 +132,7 @@ void GenerateSeededBytes(char* out, size_t length, uint32_t* state) { char* GenerateSeededCompressibleBlock(size_t length, uint32_t seed, int ratio = 4) { - char* buf = (char*)malloc(length); + char* buf = new (std::nothrow) char[length]; if (!buf) { return nullptr; } @@ -158,7 +161,10 @@ char* GenerateSeededCompressibleBlock(size_t length, uint32_t seed, return buf; } -void DestroyBlock(char* buf) { free(buf); } +// Releases anything the suite hands out, so every producer here and in +// test_utils.cpp has to allocate the way this releases. It used to free() while +// ZlibUncompress() returned new[] memory, which ASAN halts on. +void DestroyBlock(char* buf) { delete[] buf; } int ZlibCompressUtility(const char* input, size_t input_length, std::string* output, size_t* output_upper_bound) { From 0cc9d8aa7a1f3cad0682993c30a454b16cf8d4b8 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 27 Aug 2026 15:00:22 -0700 Subject: [PATCH 2/9] tests: reclaim the zlib state deflateCopy/inflateCopy orphan 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 --- tests/zlib_accel_test.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index eae0027..b2172c0 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -5083,9 +5083,17 @@ TEST_F(StreamCopyRegressionTest, ASSERT_EQ(GetDeflateExecutionPath(&source), ZLIB); const size_t source_produced = source_output.size() - source.avail_out; + // zlib's deflateCopy overwrites the destination z_stream wholesale, so the + // destination's own zlib state is orphaned rather than freed -- stock zlib + // does this with no shim loaded. Keep the pointer so the test can hand it + // back afterwards; deflateEnd refuses any other z_stream address, because the + // state points back at the stream it was initialized with. + struct internal_state* orphaned_state = dest.state; + ASSERT_EQ(deflateCopy(&dest, &source), Z_OK); EXPECT_EQ(GetDeflateExecutionPath(&dest), ZLIB); EXPECT_FALSE(DeflateOwnsIgzipState(&dest)); + ASSERT_NE(dest.state, orphaned_state); // Finishing on the copy proves the release did not disturb the state the copy // is meant to continue from: the prefix the source emitted plus the tail the @@ -5098,6 +5106,8 @@ TEST_F(StreamCopyRegressionTest, ASSERT_EQ(deflate(&dest, Z_FINISH), Z_STREAM_END); const size_t dest_produced = dest_output.size() - dest.avail_out; ASSERT_EQ(deflateEnd(&dest), Z_OK); + dest.state = orphaned_state; + ASSERT_EQ(deflateEnd(&dest), Z_OK); // The source is abandoned with its stream unfinished, which is exactly the // case zlib reports Z_DATA_ERROR for; the copy carried the tail. ASSERT_EQ(deflateEnd(&source), Z_DATA_ERROR); @@ -5183,9 +5193,14 @@ TEST_F(StreamCopyRegressionTest, ASSERT_EQ(GetInflateExecutionPath(&source), ZLIB); const size_t source_produced = source.total_out; + // As on the deflate side, the copy orphans the destination's own zlib state, + // which only this z_stream address can release. + struct internal_state* orphaned_state = dest.state; + ASSERT_EQ(inflateCopy(&dest, &source), Z_OK); EXPECT_EQ(GetInflateExecutionPath(&dest), ZLIB); EXPECT_FALSE(InflateOwnsIgzipState(&dest)); + ASSERT_NE(dest.state, orphaned_state); // As on the deflate side, the copy has to be able to finish the stream the // source was partway through. @@ -5206,6 +5221,8 @@ TEST_F(StreamCopyRegressionTest, EXPECT_EQ(memcmp(dest_output.data(), input + source_produced, dest_produced), 0); ASSERT_EQ(inflateEnd(&dest), Z_OK); + dest.state = orphaned_state; + ASSERT_EQ(inflateEnd(&dest), Z_OK); ASSERT_EQ(inflateEnd(&source), Z_OK); DestroyBlock(input); From 1ba8f05e01b5ce1f683fa32692d51f57a2d44db2 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 27 Aug 2026 15:00:34 -0700 Subject: [PATCH 3/9] tests: give ZlibUncompress a single ownership rule 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 --- fuzzing/zlib_accel_fuzz.cpp | 5 ++++- tests/test_utils.cpp | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/fuzzing/zlib_accel_fuzz.cpp b/fuzzing/zlib_accel_fuzz.cpp index 956e030..11ac352 100644 --- a/fuzzing/zlib_accel_fuzz.cpp +++ b/fuzzing/zlib_accel_fuzz.cpp @@ -54,7 +54,7 @@ void CompressDecompress(const uint8_t* input_data, size_t input_data_length, return; } - char* uncompressed; + char* uncompressed = nullptr; size_t uncompressed_length; size_t input_consumed; execution_path = UNDEFINED; @@ -63,6 +63,8 @@ void CompressDecompress(const uint8_t* input_data, size_t input_data_length, window_bits_uncompress, flush_uncompress, 1, &execution_path); + // ZlibUncompress only hands back a buffer on Z_STREAM_END, so the failure + // return above owns nothing; a mismatch after it does. if (ret != Z_STREAM_END) { *fuzz_ret = 1; return; @@ -70,6 +72,7 @@ void CompressDecompress(const uint8_t* input_data, size_t input_data_length, if (memcmp(uncompressed, input, uncompressed_length) != 0) { *fuzz_ret = 1; + delete[] uncompressed; return; } diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index afb9dc8..24ef491 100644 --- a/tests/test_utils.cpp +++ b/tests/test_utils.cpp @@ -52,6 +52,11 @@ int ZlibUncompress(const char* input, size_t input_length, size_t output_length, z_stream stream; memset(&stream, 0, sizeof(z_stream)); + // Nothing is owned by the caller unless this returns Z_STREAM_END, so every + // error return below leaves *uncompressed null rather than handing back a + // buffer no caller checks the status before releasing. + *uncompressed = nullptr; + int st = inflateInit2(&stream, window_bits); if (st != Z_OK) { inflateEnd(&stream); @@ -80,6 +85,8 @@ int ZlibUncompress(const char* input, size_t input_length, size_t output_length, (st == Z_OK && input_chunk == (input_chunks - 1)) || (st != Z_OK && st != Z_STREAM_END)) { inflateEnd(&stream); + delete[] *uncompressed; + *uncompressed = nullptr; return st; } } From f36d06377260c4011381dcde006c02d4e70240f8 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 27 Aug 2026 15:00:42 -0700 Subject: [PATCH 4/9] tests: suppress the QATzip mutex leak so LSAN can gate the rest 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 --- tests/CMakeLists.txt | 20 ++++++++++++++++---- tests/lsan.supp | 9 +++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 tests/lsan.supp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b0c4564..ca268a7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,10 +23,22 @@ add_executable(zlib_accel_test ../utils.cpp ) -add_custom_target(run - COMMAND ./zlib_accel_test - DEPENDS zlib_accel_test -) +if(ASAN) + # LeakSanitizer runs at exit under ASAN, and a vendor library holds + # allocations it never releases; lsan.supp names them so the run can still + # fail on a leak that is ours. + add_custom_target(run + COMMAND ${CMAKE_COMMAND} -E env + LSAN_OPTIONS=suppressions=${CMAKE_CURRENT_SOURCE_DIR}/lsan.supp + ./zlib_accel_test + DEPENDS zlib_accel_test + ) +else() + add_custom_target(run + COMMAND ./zlib_accel_test + DEPENDS zlib_accel_test + ) +endif() if(COVERAGE) add_custom_target(coverage diff --git a/tests/lsan.supp b/tests/lsan.supp new file mode 100644 index 0000000..a3e9f94 --- /dev/null +++ b/tests/lsan.supp @@ -0,0 +1,9 @@ +# LeakSanitizer suppressions for the ASAN build (-DASAN=ON). +# +# One entry per allocation that is never released and is not ours to release. +# A leak in the shim or in the test suite is a bug to fix, not an entry to add +# here -- everything listed below is inside a vendor library, named per entry. + +# QATzip's OSAL allocates a process-lifetime mutex and never calls the matching +# destroy, so it is still held at exit. Only reachable with USE_QAT. +leak:osalMutexInit From 2cbbec716f1d17a92489aada37e97efa4cb7064a Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 27 Aug 2026 15:28:06 -0700 Subject: [PATCH 5/9] tests: assert that a zero block is actually zeroed 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 --- tests/zlib_accel_test.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index b2172c0..a365bec 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -662,6 +663,20 @@ void RunDummyQATJob() { DestroyBlock(input); } +// A zero block that is not zeros still round-trips, so every case that takes +// one would pass on uninitialized memory and the parameterized sweep would +// quietly lose its most compressible payload. Nothing else in the suite looks +// at the contents of a generated block, so state the one generator whose +// contents are part of its contract. Draws no randomness, so it leaves the +// payload sequence the parameterized cases share alone. +TEST(GeneratedBlockTest, ZeroBlockIsZeroed) { + const size_t length = 4096; + char* buf = GenerateBlock(length, zero_block); + ASSERT_NE(buf, nullptr); + EXPECT_EQ(static_cast(std::count(buf, buf + length, '\0')), length); + DestroyBlock(buf); +} + class ZlibTest : public testing::TestWithParam< std::tuple Date: Thu, 27 Aug 2026 16:39:21 -0700 Subject: [PATCH 6/9] tests: attribute the suppressed mutex to the right library 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 --- tests/lsan.supp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/lsan.supp b/tests/lsan.supp index a3e9f94..602dd0c 100644 --- a/tests/lsan.supp +++ b/tests/lsan.supp @@ -4,6 +4,11 @@ # A leak in the shim or in the test suite is a bug to fix, not an entry to add # here -- everything listed below is inside a vendor library, named per entry. -# QATzip's OSAL allocates a process-lifetime mutex and never calls the matching -# destroy, so it is still held at exit. Only reachable with USE_QAT. +# The QAT driver's OSAL (libqat, reached through QATzip's session setup) +# allocates a process-lifetime mutex and never calls the matching destroy, so it +# is still held at exit. Only reachable with USE_QAT. +# +# osalMutexInit is a local symbol, so this matches only while libqat ships +# unstripped. Against a stripped driver the frame has no name, the entry stops +# matching, and the leak comes back as a failure. leak:osalMutexInit From 48e0ac53a18c54d8e2a15539d7f7b6cf4baa1a96 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Fri, 28 Aug 2026 10:24:01 -0700 Subject: [PATCH 7/9] tests: treat a Z_OK partial decode as a result, not an error 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 --- fuzzing/zlib_accel_fuzz.cpp | 6 ++++-- tests/test_utils.cpp | 28 ++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/fuzzing/zlib_accel_fuzz.cpp b/fuzzing/zlib_accel_fuzz.cpp index 11ac352..5905f1c 100644 --- a/fuzzing/zlib_accel_fuzz.cpp +++ b/fuzzing/zlib_accel_fuzz.cpp @@ -63,10 +63,12 @@ void CompressDecompress(const uint8_t* input_data, size_t input_data_length, window_bits_uncompress, flush_uncompress, 1, &execution_path); - // ZlibUncompress only hands back a buffer on Z_STREAM_END, so the failure - // return above owns nothing; a mismatch after it does. + // A Z_OK return owns the partial prefix it produced and an error owns + // nothing, so release unconditionally here -- delete[] on the null an error + // leaves is a no-op, and anything else is the buffer this call is abandoning. if (ret != Z_STREAM_END) { *fuzz_ret = 1; + delete[] uncompressed; return; } diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index 24ef491..45d6e58 100644 --- a/tests/test_utils.cpp +++ b/tests/test_utils.cpp @@ -52,9 +52,9 @@ int ZlibUncompress(const char* input, size_t input_length, size_t output_length, z_stream stream; memset(&stream, 0, sizeof(z_stream)); - // Nothing is owned by the caller unless this returns Z_STREAM_END, so every - // error return below leaves *uncompressed null rather than handing back a - // buffer no caller checks the status before releasing. + // The caller owns a buffer on Z_STREAM_END and on the Z_OK partial return, + // and nothing on any error, which leaves *uncompressed null rather than + // handing back a buffer no caller checks the status before releasing. *uncompressed = nullptr; int st = inflateInit2(&stream, window_bits); @@ -81,12 +81,24 @@ int ZlibUncompress(const char* input, size_t input_length, size_t output_length, st = inflate(&stream, flush); *execution_path = GetInflateExecutionPath(&stream); - if ((st == Z_STREAM_END && input_chunk < (input_chunks - 1)) || - (st == Z_OK && input_chunk == (input_chunks - 1)) || - (st != Z_OK && st != Z_STREAM_END)) { + + // Z_OK on the last chunk means the input held less than a whole stream, so + // the prefix that came back is the result the caller asked for rather than + // a failure. Report its size and hand it over; the other two stop + // conditions are errors and own nothing. + bool partial_progress = (st == Z_OK && input_chunk == (input_chunks - 1)); + bool premature_end = + (st == Z_STREAM_END && input_chunk < (input_chunks - 1)); + bool failed = (st != Z_OK && st != Z_STREAM_END); + if (partial_progress || premature_end || failed) { + if (partial_progress) { + *uncompressed_length = stream.total_out; + *input_consumed = stream.total_in; + } else { + delete[] *uncompressed; + *uncompressed = nullptr; + } inflateEnd(&stream); - delete[] *uncompressed; - *uncompressed = nullptr; return st; } } From 0ec9e369c880a60086b78f3bbc386d1243c58f23 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 3 Sep 2026 10:34:41 -0700 Subject: [PATCH 8/9] tests: keep the ASAN run alive past startup -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 --- tests/CMakeLists.txt | 4 ++++ tests/asan_suppressions.cpp | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/asan_suppressions.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ca268a7..647c000 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,6 +23,10 @@ add_executable(zlib_accel_test ../utils.cpp ) +if(ASAN) + target_sources(zlib_accel_test PRIVATE asan_suppressions.cpp) +endif() + if(ASAN) # LeakSanitizer runs at exit under ASAN, and a vendor library holds # allocations it never releases; lsan.supp names them so the run can still diff --git a/tests/asan_suppressions.cpp b/tests/asan_suppressions.cpp new file mode 100644 index 0000000..9d93974 --- /dev/null +++ b/tests/asan_suppressions.cpp @@ -0,0 +1,26 @@ +// Copyright (C) 2025 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +// AddressSanitizer suppressions for the test binary (-DASAN=ON), compiled in +// rather than passed through ASAN_OPTIONS so that a bare ./zlib_accel_test is +// covered as well as the run target. +// +// libstdc++ marks all of namespace std with default visibility, 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 and reports an ODR violation +// before the first test runs. The definitions are identical: this is structural +// to linking two objects that both use std::shared_ptr and +// std::piecewise_construct, not a defect to chase. +// +// One entry per duplicated global, so ODR detection stays on for everything +// else. ASAN matches these against the bare global name, which is as narrow as +// the runtime allows -- there is no way to scope an entry to a namespace or a +// module. A toolchain that duplicates a further std global aborts on it by +// name, which is the intended failure: the entry is then a decision, not a +// blanket flag. +extern "C" __attribute__((visibility("default"))) const char* +__asan_default_suppressions() { + return "odr_violation:__tag\n" + "odr_violation:piecewise_construct\n"; +} From e95092d0340066ba88eefbc7d882307ffed1dd58 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Thu, 3 Sep 2026 10:34:52 -0700 Subject: [PATCH 9/9] config: hand the log path back instead of parking it in a global 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 --- config/config.cpp | 9 +++++---- config/config.h | 12 +++++++++--- tests/zlib_accel_test.cpp | 23 +++++++++++++++++++++++ zlib_accel.cpp | 8 +++++--- 4 files changed, 42 insertions(+), 10 deletions(-) diff --git a/config/config.cpp b/config/config.cpp index 6893da8..95edf69 100644 --- a/config/config.cpp +++ b/config/config.cpp @@ -11,8 +11,6 @@ namespace config { -std::string log_file = ""; - // default config values at initialization uint32_t configs[CONFIG_MAX] = { 1, /*use_qat_compress*/ @@ -36,7 +34,8 @@ uint32_t configs[CONFIG_MAX] = { 64 /*map_shards*/ }; -bool LoadConfigFile(std::string& file_content, const char* file_path) { +bool LoadConfigFile(std::string& file_content, const char* file_path, + std::string* log_file) { // Initialize config_names within the function to avoid initialization order // problems. LoadConfigFile is called from the zlib-accel shared library // constructor. If config_names is a global array of strings, it may not be @@ -101,7 +100,9 @@ bool LoadConfigFile(std::string& file_content, const char* file_path) { trySetConfig(LOG_STATS_SAMPLES, UINT32_MAX, 0); trySetConfig(MAP_SHARDS, 65536, 2, [](uint32_t v) { return (v & (v - 1)) == 0; }); - config_reader.GetValue("log_file", log_file); + if (log_file != nullptr) { + config_reader.GetValue("log_file", *log_file); + } file_content.append(config_reader.DumpValues()); return true; diff --git a/config/config.h b/config/config.h index 31baa0f..c81e71e 100644 --- a/config/config.h +++ b/config/config.h @@ -32,12 +32,18 @@ enum ConfigOption { CONFIG_MAX }; -extern std::string log_file; - extern uint32_t configs[CONFIG_MAX]; +inline constexpr const char* kDefaultConfigPath = "/etc/zlib-accel.conf"; + +// log_file, when given, receives the log path the config file names, and is +// left untouched when the file names none. It is an out-parameter rather than a +// global: the shim opens the log file a few lines after loading the config and +// has no reader for the path afterwards, and a global written from the library +// constructor is written before its own initializer runs. VISIBLE_FOR_TESTING bool LoadConfigFile( - std::string& file_content, const char* file_path = "/etc/zlib-accel.conf"); + std::string& file_content, const char* file_path = kDefaultConfigPath, + std::string* log_file = nullptr); VISIBLE_FOR_TESTING void SetConfig(ConfigOption option, uint32_t value); VISIBLE_FOR_TESTING uint32_t GetConfig(ConfigOption option); diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index a365bec..6191987 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -6811,6 +6811,29 @@ TEST_F(ConfigLoaderTest, MapShardsInvalidNonPowerOfTwo) { SetConfig(MAP_SHARDS, saved_shards); } +// The log path is handed back through an out-parameter rather than parked in a +// global, so a caller that asks for it gets what the config file names, and a +// config file that names none leaves the caller's string alone. +TEST_F(ConfigLoaderTest, LogFilePathHandedBack) { + std::string file_content; + std::string log_file; + EXPECT_TRUE( + LoadConfigFile(file_content, "../../config/default_config", &log_file)); + EXPECT_EQ(log_file, "/tmp/zlib-accel.log"); + + const char* config_path = "/tmp/no_log_file_config"; + std::ofstream config_file(config_path); + config_file << "log_level=1\n"; + config_file.close(); + std::string untouched = "unchanged"; + EXPECT_TRUE(LoadConfigFile(file_content, config_path, &untouched)); + EXPECT_EQ(untouched, "unchanged"); + std::remove(config_path); + + // Restore config from the official config file. + LoadConfigFile(file_content); +} + // The shim keeps per-stream state in maps keyed by z_streamp, and every entry // point that consumes that state has to cope with the entry being absent: a // stream that was never initialized at all, one whose *Init failed, or a diff --git a/zlib_accel.cpp b/zlib_accel.cpp index e4aea5e..0b34bfd 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -244,7 +244,9 @@ static int init_zlib_accel(void) { // Load configuration file; on failure (file absent or is a symlink) continue // with compiled-in defaults — a missing config is not fatal. std::string config_file_content; - const bool config_loaded = config::LoadConfigFile(config_file_content); + std::string log_file; + const bool config_loaded = config::LoadConfigFile( + config_file_content, config::kDefaultConfigPath, &log_file); if (!config_loaded) { Log(LogLevel::LOG_ERROR, "Failed to load configuration file, continuing with defaults\n"); @@ -253,8 +255,8 @@ static int init_zlib_accel(void) { InitStreamRegistries(); #if defined(DEBUG_LOG) || defined(ENABLE_STATISTICS) - if (config_loaded && !config::log_file.empty()) { - CreateLogFile(config::log_file.c_str()); + if (config_loaded && !log_file.empty()) { + CreateLogFile(log_file.c_str()); } #endif