Add sanitizer instrumentation to CI

.github/workflows/ci.yml

@@ -59,6 +59,98 @@ jobs:
       - name: Run specs
         run: bundle exec rspec --format progress
 
+  sanitize:
+    name: ASan+UBSan ruby-${{ matrix.ruby }}
+    runs-on: ubuntu-24.04
+    strategy:
+      fail-fast: false
+      matrix:
+        ruby: ["4.0"]
+    services:
+      clickhouse:
+        image: clickhouse/clickhouse-server:25.3
+        ports:
+          - 9000:9000
+          - 8123:8123
+        env:
+          CLICKHOUSE_SKIP_USER_SETUP: 1
+        options: >-
+          --health-cmd "wget --spider -q http://localhost:8123/ping"
+          --health-interval 2s
+          --health-timeout 2s
+          --health-retries 20
+    env:
+      CLICKHOUSE_HOST: localhost
+      CLICKHOUSE_PORT: 9000
+      # clang + clang's libsanitizer (not gcc's).
+      CC: clang
+      CXX: clang++
+      # -O1 for readable line numbers; -fno-omit-frame-pointer for backtraces.
+      CFLAGS: "-fsanitize=address,undefined -fno-omit-frame-pointer -O1 -g"
+      CXXFLAGS: "-fsanitize=address,undefined -fno-omit-frame-pointer -O1 -g"
+      LDFLAGS: "-fsanitize=address,undefined"
+      # LSan off: Ruby's conservative GC produces false positives.
+      ASAN_OPTIONS: "detect_leaks=0:abort_on_error=1:print_stats=1"
+      UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1"
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: ${{ matrix.ruby }}
+          bundler-cache: true
+      - name: Install build deps
+        run: sudo apt-get update && sudo apt-get install -y cmake ninja-build clang
+      - name: Compile extension with sanitizers
+        run: bundle exec rake compile
+      - name: Run specs under sanitizers
+        # Preload clang's ASan+UBSan runtimes; must match what we built against.
+        run: |
+          export LD_PRELOAD="$(clang -print-file-name=libclang_rt.asan-x86_64.so):$(clang -print-file-name=libclang_rt.ubsan_standalone-x86_64.so)"
+          bundle exec rspec --format progress
+
+  gc-stress:
+    name: GC.stress ruby-${{ matrix.ruby }}
+    runs-on: ubuntu-24.04
+    strategy:
+      fail-fast: false
+      matrix:
+        ruby: ["4.0"]
+    services:
+      clickhouse:
+        image: clickhouse/clickhouse-server:25.3
+        ports:
+          - 9000:9000
+          - 8123:8123
+        env:
+          CLICKHOUSE_SKIP_USER_SETUP: 1
+        options: >-
+          --health-cmd "wget --spider -q http://localhost:8123/ping"
+          --health-interval 2s
+          --health-timeout 2s
+          --health-retries 20
+    env:
+      CLICKHOUSE_HOST: localhost
+      CLICKHOUSE_PORT: 9000
+      # spec_helper.rb opts into GC.stress when set, and prints
+      # "[spec_helper] GC.stress = true". No marker line → not active.
+      CLICKHOUSE_NATIVE_GC_STRESS: "1"
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: ${{ matrix.ruby }}
+          bundler-cache: true
+      - name: Install build deps
+        run: sudo apt-get update && sudo apt-get install -y cmake ninja-build
+      - name: Compile extension
+        run: bundle exec rake compile
+      - name: Run specs under GC.stress
+        run: bundle exec rspec --format progress
+
   macos-compile:
     name: macos compile (ruby-${{ matrix.ruby }})
     runs-on: macos-14

.github/workflows/tsan.yml

@@ -0,0 +1,59 @@
+name: TSan nightly
+
+# TSan can't compose with ASan, so it lives in a separate workflow.
+on:
+  schedule:
+    - cron: "0 4 * * *"
+  workflow_dispatch:
+
+concurrency:
+  group: tsan-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  tsan:
+    name: TSan ruby-${{ matrix.ruby }}
+    runs-on: ubuntu-24.04
+    # Soft-launched while we triage findings.
+    continue-on-error: true
+    strategy:
+      fail-fast: false
+      matrix:
+        ruby: ["4.0"]
+    services:
+      clickhouse:
+        image: clickhouse/clickhouse-server:25.3
+        ports:
+          - 9000:9000
+          - 8123:8123
+        env:
+          CLICKHOUSE_SKIP_USER_SETUP: 1
+        options: >-
+          --health-cmd "wget --spider -q http://localhost:8123/ping"
+          --health-interval 2s
+          --health-timeout 2s
+          --health-retries 20
+    env:
+      CLICKHOUSE_HOST: localhost
+      CLICKHOUSE_PORT: 9000
+      # TSan doesn't compose with UBSan; no `undefined` here.
+      CFLAGS: "-fsanitize=thread -fno-omit-frame-pointer -O1 -g"
+      CXXFLAGS: "-fsanitize=thread -fno-omit-frame-pointer -O1 -g"
+      LDFLAGS: "-fsanitize=thread"
+      TSAN_OPTIONS: "halt_on_error=1:second_deadlock_stack=1:suppressions=.tsan-suppressions"
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: ${{ matrix.ruby }}
+          bundler-cache: true
+      - name: Install build deps
+        run: sudo apt-get update && sudo apt-get install -y cmake ninja-build
+      - name: Compile extension with TSan
+        run: bundle exec rake compile
+      - name: Run specs under TSan
+        run: |
+          export LD_PRELOAD=$(gcc -print-file-name=libtsan.so)
+          bundle exec rspec --format progress

.tsan-suppressions

@@ -0,0 +1,9 @@
+# ThreadSanitizer suppressions. Each line: `<rule>:<pattern>`.
+#
+# Ruby's threading internals (GVL, ractor scheduler, waiter lists, signal
+# handling) acquire several internal mutexes in different orders depending
+# on which thread initiates the operation. The locking is correct in
+# practice but TSan's static cycle detection flags it as a potential
+# deadlock. Not a bug in our code. Suppress only patterns we've actually
+# seen, not preemptively.
+deadlock:rb_native_mutex_lock

ext/clickhouse_native/patches/0002-guard-empty-string-memcpy.patch

@@ -0,0 +1,21 @@
+diff --git a/clickhouse/columns/string.cpp b/clickhouse/columns/string.cpp
+index dff45ba..e4ba42f 100644
+--- a/clickhouse/columns/string.cpp
++++ b/clickhouse/columns/string.cpp
+@@ -136,7 +136,15 @@ struct ColumnString::Block
+     std::string_view AppendUnsafe(std::string_view str) {
+         const auto pos = &data_[size];
+
+-        memcpy(pos, str.data(), str.size());
++        // memcpy with a NULL source pointer is UB even when n == 0
++        // (its arguments are nonnull-attributed). std::string_view's
++        // data() is permitted to return nullptr for an empty view, so
++        // guard the call. UBSan flags this; libc implementations
++        // short-circuit on n == 0 in practice, hence years of working
++        // production code.
++        if (str.size() != 0) {
++            memcpy(pos, str.data(), str.size());
++        }
+         size += str.size();
+
+         return std::string_view(pos, str.size());

ext/clickhouse_native/patches/0003-no-error-on-nonnull-warning.patch

@@ -0,0 +1,13 @@
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index 24b71ba..e344795 100644
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -45,7 +45,7 @@ IF (UNIX)
+     SET (CMAKE_EXE_LINKER_FLAGS, "${CMAKE_EXE_LINKER_FLAGS} -lpthread")
+     # -Wpedantic makes int128 support somewhat harder and less performant (by not allowing builtin __int128)
+     # -Wno-deprecated-declarations to produce less cluttered output when building library itself (`deprecated` attributes are for library users)
+-    SET (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror -Wno-deprecated-declarations")
++    SET (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror -Wno-deprecated-declarations -Wno-error=nonnull")
+ ENDIF ()
+
+ IF (APPLE OR MSVC)

ext/clickhouse_native/patches/0004-lz4-zero-offset-on-null.patch

@@ -0,0 +1,41 @@
+diff --git a/contrib/lz4/lz4/lz4.c b/contrib/lz4/lz4/lz4.c
+index 82ab490..9c19c71 100644
+--- a/contrib/lz4/lz4/lz4.c
++++ b/contrib/lz4/lz4/lz4.c
+@@ -819,7 +819,12 @@ LZ4_FORCE_INLINE int LZ4_compress_generic(
+
+     int const maybe_extMem = (dictDirective == usingExtDict) || (dictDirective == usingDictCtx);
+     U32 const prefixIdxLimit = startIndex - dictSize;   /* used when dictDirective == dictSmall */
+-    const BYTE* const dictEnd = dictionary + dictSize;
++    /* Guard against `dictionary + dictSize` evaluating to `NULL + 0`, which is
++     * UB even though it produces NULL on every real implementation. UBSan's
++     * pointer-overflow check trips on it. Without a loaded dictionary,
++     * `dictionary` is NULL and `dictSize` is 0; preserve the original
++     * "dictEnd == dictionary" semantics in that case. */
++    const BYTE* const dictEnd = dictionary ? dictionary + dictSize : NULL;
+     const BYTE* anchor = (const BYTE*) source;
+     const BYTE* const iend = ip + inputSize;
+     const BYTE* const mflimitPlusOne = iend - MFLIMIT + 1;
+@@ -827,9 +832,9 @@ LZ4_FORCE_INLINE int LZ4_compress_generic(
+
+     /* the dictCtx currentOffset is indexed on the start of the dictionary,
+      * while a dictionary in the current context precedes the currentOffset */
+-    const BYTE* dictBase = (dictDirective == usingDictCtx) ?
+-                            dictionary + dictSize - dictCtx->currentOffset :
+-                            dictionary + dictSize - startIndex;
++    /* Same NULL+0 guard as `dictEnd` above — see the comment there. */
++    U32 const dictBaseOffset = (dictDirective == usingDictCtx) ? dictCtx->currentOffset : startIndex;
++    const BYTE* dictBase = dictionary ? dictionary + dictSize - dictBaseOffset : NULL;
+
+     BYTE* op = (BYTE*) dest;
+     BYTE* const olimit = op + maxOutputSize;
+@@ -2193,7 +2198,8 @@ int LZ4_setStreamDecode (LZ4_streamDecode_t* LZ4_streamDecode, const char* dicti
+ {
+     LZ4_streamDecode_t_internal* lz4sd = &LZ4_streamDecode->internal_donotuse;
+     lz4sd->prefixSize = (size_t) dictSize;
+-    lz4sd->prefixEnd = (const BYTE*) dictionary + dictSize;
++    /* Same NULL+0 guard as `dictEnd` above — see the comment there. */
++    lz4sd->prefixEnd = dictionary ? (const BYTE*) dictionary + dictSize : NULL;
+     lz4sd->externalDict = NULL;
+     lz4sd->extDictSize  = 0;
+     return 1;

spec/clickhouse_native_spec.rb

@@ -584,3 +584,27 @@
     end
   end
 end
+
+# Exercises the GC-driven cleanup path bypassed by spec_helper's
+# auto-close hook. FD count is the observable; Ruby reclaims wrappers
+# regardless of dfree's behavior, so an ObjectSpace assertion would
+# pass even on a broken dfree.
+RSpec.describe "ClickhouseNative::Client GC finalization", :clickhouse do
+  it "releases native resources when a dropped Client is GC'd" do
+    skip "Requires /proc/self/fd (Linux only)" unless File.directory?("/proc/self/fd")
+    skip "Premise breaks under GC.stress" if GC.stress
+
+    fd_count = -> { Dir.children("/proc/self/fd").size }
+    baseline = fd_count.call
+    20.times { ClickhouseNative::Client.new(**CH_KWARGS) }
+
+    # Witness that sockets actually opened, or the post-GC check is meaningless.
+    expect(fd_count.call - baseline).to be >= 20
+
+    # Forces dfree synchronously; second pass flushes deferred finalizations.
+    GC.start(full_mark: true, immediate_sweep: true)
+    GC.start(full_mark: true, immediate_sweep: true)
+
+    expect(fd_count.call).to be <= baseline
+  end
+end

spec/spec_helper.rb

@@ -1,5 +1,16 @@
 # frozen_string_literal: true
 
+# Enable GC.stress before any other allocations so the GC fires on
+# every allocation in the entire test run — including require/load. The
+# CI job sets CLICKHOUSE_NATIVE_GC_STRESS=1; local dev is unaffected.
+# RUBY_GC_STRESS env var is *not* reliably honored across CRuby builds,
+# so we wire it ourselves and emit a marker line for observability: if
+# the warn doesn't appear in CI logs, stress is not actually active.
+if ENV["CLICKHOUSE_NATIVE_GC_STRESS"] == "1"
+  GC.stress = true
+  warn "[spec_helper] GC.stress = true"
+end
+
 require "bigdecimal"
 require "logger"
 require "stringio"
@@ -21,4 +32,17 @@
   config.disable_monkey_patching!
   config.order = :random
   Kernel.srand(config.seed)
+
+  # Close any ClickhouseNative::Client that survived the example.
+  # Inline `Pool.new(...)` constructions in `it` blocks have no
+  # binding-scoped teardown; this sweep catches them. Pool-owned
+  # Clients are reachable through the Pool's ConnectionPool queue and
+  # get swept too. Client#close is a no-op on an already-closed Client.
+  config.after do
+    ObjectSpace.each_object(ClickhouseNative::Client) do |c|
+      c.close
+    rescue StandardError => error
+      warn "[teardown cleanup] #{c.class}#close raised: #{error.class}: #{error.message}"
+    end
+  end
 end