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/fuzzing/zlib_accel_fuzz.cpp b/fuzzing/zlib_accel_fuzz.cpp index 956e030..5905f1c 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,13 +63,18 @@ void CompressDecompress(const uint8_t* input_data, size_t input_data_length, window_bits_uncompress, flush_uncompress, 1, &execution_path); + // 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; } if (memcmp(uncompressed, input, uncompressed_length) != 0) { *fuzz_ret = 1; + delete[] uncompressed; return; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b0c4564..647c000 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,10 +23,26 @@ add_executable(zlib_accel_test ../utils.cpp ) -add_custom_target(run - COMMAND ./zlib_accel_test - DEPENDS zlib_accel_test -) +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 + # 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/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"; +} diff --git a/tests/lsan.supp b/tests/lsan.supp new file mode 100644 index 0000000..602dd0c --- /dev/null +++ b/tests/lsan.supp @@ -0,0 +1,14 @@ +# 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. + +# 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 diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index afb9dc8..45d6e58 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)); + // 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); if (st != Z_OK) { inflateEnd(&stream); @@ -76,9 +81,23 @@ 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); return st; } diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index c6a1dcb..6191987 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -17,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -55,7 +57,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 +85,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 +97,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 +133,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 +162,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) { @@ -656,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