Tests: Make ASAN build usable

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;

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);

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;
   }
 

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

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";
+}

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

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;
     }

tests/zlib_accel_test.cpp

@@ -8,6 +8,7 @@
 #include <stdio.h>
 #include <unistd.h>
 
+#include <algorithm>
 #include <cstdint>
 #include <cstdio>
 #include <cstdlib>
@@ -17,6 +18,7 @@
 #include <iostream>
 #include <limits>
 #include <map>
+#include <new>
 #include <sstream>
 #include <thread>
 #include <tuple>
@@ -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<size_t>(std::count(buf, buf + length, '\0')), length);
+  DestroyBlock(buf);
+}
+
 class ZlibTest
     : public testing::TestWithParam<
           std::tuple<ExecutionPath, bool, ExecutionPath, bool, int, int, int,
@@ -5077,9 +5098,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
@@ -5092,6 +5121,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);
@@ -5177,9 +5208,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.
@@ -5200,6 +5236,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);
@@ -6773,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

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