From 4d0eca9bdc07f1b618898988707ee2bba4d357b6 Mon Sep 17 00:00:00 2001 From: Liyun Xiu Date: Tue, 8 Sep 2026 09:27:01 +0000 Subject: [PATCH] Run the test suite under ASan and UBSan in CI nsparse deserializes binary index files that may be truncated or corrupt. The readers validate defensively and ~120 negative-path assertions cover that, but those assertions only prove an exception was thrown -- not that no out-of-bounds read, overflow or misaligned access happened first. For an in-process native library a memory error is much worse than an exception, and CI built Release with no sanitizers, so nothing checked. Add NSPARSE_ENABLE_SANITIZERS, which applies -fsanitize=address,undefined globally before any target exists. Global is required, not tidiness: an instrumented translation unit and an uninstrumented one disagree about libstdc++'s container annotations, which surfaces as container-overflow false positives rather than a link error. -fno-sanitize-recover=all goes with it, because UBSan otherwise prints and continues and the run still exits 0. UBSan carries as much weight as ASan here -- overflow and misalignment are what binary parsing produces -- but under GCC its null and nonnull-attribute checks make the address of a function template instantiation non-constant, which breaks abseil's constexpr hash-function dispatch in every translation unit that includes flat_hash_map. Those three are dropped on GCC; a null dereference still surfaces, as an ASan SEGV report. One per-PR job, at the generic optimization level. tests/CMakeLists.txt registers the per-ISA distance_kernel_equivalence_test binaries (and builds nsparse_avx2/nsparse_avx512 to link them) regardless of NSPARSE_OPT_LEVEL, so that single configuration already runs the vectorized kernels -- where tail-element OOB would live -- under ASan; a SIMD-configured job would only re-cover them. It runs on ubuntu-latest rather than the CI image the other Linux jobs use, because it needs the libasan/libubsan runtimes. All 639 tests pass as-is: no existing finding is being suppressed. Verified the instrumentation is live, not a silent no-op, with throwaway tests -- a heap-buffer overflow, a use-after-free and a signed overflow each aborted the run with a report. Default builds are unaffected: cmake/sanitizers.cmake returns before adding any flag when the option is OFF, which it is by default. No perf claim. A libFuzzer harness over the CSR/mmap readers is the natural follow-up. Signed-off-by: Liyun Xiu --- .github/workflows/CI.yml | 51 ++++++++++++++++++++++++++++++++++-- CMakeLists.txt | 4 +++ DEVELOPER_GUIDE.md | 34 ++++++++++++++++++++++++ cmake/sanitizers.cmake | 56 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 cmake/sanitizers.cmake diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index d77f9e7..00e8942 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -112,6 +112,53 @@ jobs: if: steps.detect-simd.outputs.opt_level != '' run: ctest --test-dir build --output-on-failure + # nsparse deserializes binary index files that may be truncated or corrupt, and + # the ~120 negative-path assertions only prove an exception was thrown -- not + # that no out-of-bounds read, overflow or misaligned access happened first. The + # jobs above build Release with no sanitizers, so nothing checks that today. + # + # UBSan carries as much weight as ASan here: overflow and misalignment are what + # binary parsing produces. RelWithDebInfo, because -O0 changes which code the + # optimizer would have folded away and the reports need line numbers. + # + # generic, and only generic: tests/CMakeLists.txt registers the per-ISA + # distance_kernel_equivalence_test binaries (and builds nsparse_avx2 / + # nsparse_avx512 to link them) at every NSPARSE_OPT_LEVEL, so this one job + # already runs the vectorized kernels -- where tail-element OOB would live -- + # under ASan. A second SIMD-configured job would only re-cover them. + # + # Not in the container the other Linux jobs use: this needs the libasan/libubsan + # runtimes, which ubuntu-latest ships with gcc and the CI image does not + # promise. + Sanitize-nsparse-Linux: + name: ASan and UBSan nsparse on Linux + needs: check-files + if: needs.check-files.outputs.RUN_BUILD_AND_TEST == 'true' + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - name: Configure + run: | + cmake -B build \ + -DNSPARSE_ENABLE_TESTS=ON \ + -DNSPARSE_OPT_LEVEL=generic \ + -DNSPARSE_ENABLE_SANITIZERS=ON \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo + + - name: Build + run: cmake --build build -j$(nproc) + + # print_stacktrace turns a UBSan one-liner into something you can act on; + # without it the report names the source line but not the caller that + # reached it. + - name: Test + env: + ASAN_OPTIONS: detect_leaks=1:detect_stack_use_after_return=1:strict_string_checks=1:check_initialization_order=1:strict_init_order=1 + UBSAN_OPTIONS: print_stacktrace=1 + run: ctest --test-dir build --output-on-failure + Build-nsparse-Python-Linux: name: Build and Test nsparse Python bindings on Linux needs: check-files @@ -260,7 +307,7 @@ jobs: run: ctest --test-dir build --build-config Release --output-on-failure check-results: - needs: [check-files, Build-nsparse-Linux, Build-nsparse-Python-Linux, Build-nsparse-MacOS, Build-nsparse-Windows] + needs: [check-files, Build-nsparse-Linux, Sanitize-nsparse-Linux, Build-nsparse-Python-Linux, Build-nsparse-MacOS, Build-nsparse-Windows] if: always() name: Check results runs-on: ubuntu-latest @@ -268,5 +315,5 @@ jobs: - name: Fail if build or test failed if: | needs.check-files.outputs.RUN_BUILD_AND_TEST == 'true' && - (needs.Build-nsparse-Linux.result == 'failure' || needs.Build-nsparse-Python-Linux.result == 'failure' || needs.Build-nsparse-MacOS.result == 'failure' || needs.Build-nsparse-Windows.result == 'failure') + (needs.Build-nsparse-Linux.result == 'failure' || needs.Sanitize-nsparse-Linux.result == 'failure' || needs.Build-nsparse-Python-Linux.result == 'failure' || needs.Build-nsparse-MacOS.result == 'failure' || needs.Build-nsparse-Windows.result == 'failure') run: exit 1 diff --git a/CMakeLists.txt b/CMakeLists.txt index 996555e..be50516 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,6 +23,10 @@ if(WIN32) add_compile_definitions(NOMINMAX WIN32_LEAN_AND_MEAN) endif() +# Sanitizers, if requested. Included before any target exists so the flags reach +# every one of them, third-party dependencies included. +include(cmake/sanitizers.cmake) + # Third party dependencies include(cmake/third_party.cmake) diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 4d6a93b..933e545 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -9,6 +9,8 @@ - [SIMD Optimization Levels](#simd-optimization-levels) - [GPU Acceleration](#gpu-acceleration) - [Run Tests](#run-tests) + - [Python integration tests](#python-integration-tests) + - [Sanitizers](#sanitizers) - [Run Benchmarks](#run-benchmarks) - [Python Bindings](#python-bindings) - [Build Python Bindings](#build-python-bindings) @@ -98,6 +100,7 @@ cmake --build build -j | `NSPARSE_ENABLE_TESTS` | `OFF` | Build unit tests | | `NSPARSE_ENABLE_BENCHMARKS` | `OFF` | Build benchmarks | | `NSPARSE_ENABLE_GPU` | `OFF` | GPU-accelerate index building via cuSPARSE (see [GPU Acceleration](#gpu-acceleration)) | +| `NSPARSE_ENABLE_SANITIZERS` | `OFF` | Build with ASan and UBSan (see [Sanitizers](#sanitizers)) | Example with multiple options: ```bash @@ -212,6 +215,37 @@ schedule the lists in. subprocesses, which is required because the OpenMP runtime reads that variable when it initialises. +### Sanitizers + +`NSPARSE_ENABLE_SANITIZERS=ON` instruments the whole build with AddressSanitizer +and UndefinedBehaviorSanitizer. This matters most for the index-file readers: the +negative-path tests assert that a truncated or corrupt file raises an exception, +but only a sanitizer can tell you no out-of-bounds read, overflow or misaligned +access happened on the way to that exception. + +```bash +cmake -S . -B build -DNSPARSE_ENABLE_TESTS=ON -DNSPARSE_ENABLE_SANITIZERS=ON \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo +cmake --build build -j +UBSAN_OPTIONS=print_stacktrace=1 ctest --test-dir build --output-on-failure +``` + +GCC and Clang only; you need the matching runtimes (`libasan`/`libubsan`, part of +`gcc` on Debian/Ubuntu, separate packages on Amazon Linux and RHEL). Findings +abort the process rather than print and continue, so any one of them fails the +test that hit it. `-DNSPARSE_SANITIZERS=` overrides the default +`address,undefined` for a different `-fsanitize` set, e.g. `thread`. + +Under GCC, the `null` check and the two nonnull-attribute checks are dropped: +they make the address of a function template instantiation non-constant, which +breaks abseil's `constexpr` hash-function dispatch in every translation unit that +includes `flat_hash_map`. A null dereference still shows up, as an ASan SEGV +report. + +This runs per-PR in CI at the `generic` optimization level. The per-ISA +`distance_kernel_equivalence_test` binaries are built and run at every +optimization level, so that one configuration also covers the SIMD kernels. + ## Run Benchmarks Build with benchmarks enabled and run: diff --git a/cmake/sanitizers.cmake b/cmake/sanitizers.cmake new file mode 100644 index 0000000..d4c9bd5 --- /dev/null +++ b/cmake/sanitizers.cmake @@ -0,0 +1,56 @@ +# Copyright OpenSearch Contributors +# SPDX-License-Identifier: Apache-2.0 +# +# The OpenSearch Contributors require contributions made to +# this file be licensed under the Apache-2.0 license or a +# compatible open source license. + +# Sanitizer instrumentation, off by default. +# +# nsparse parses binary index files that may be truncated or corrupt. The +# negative-path tests assert that an exception is thrown, but not that no +# out-of-bounds read, overflow or misaligned access happened on the way there -- +# in an in-process native library those are far worse than an exception. ASan and +# UBSan are what turn those assertions into memory-safety assertions. +option(NSPARSE_ENABLE_SANITIZERS "Instrument the build with sanitizers" OFF) +set(NSPARSE_SANITIZERS "address,undefined" CACHE STRING + "Comma-separated -fsanitize list used when NSPARSE_ENABLE_SANITIZERS=ON") + +if(NOT NSPARSE_ENABLE_SANITIZERS) + return() +endif() + +if(MSVC) + # MSVC only ships /fsanitize=address, and spells it differently; nothing + # here would apply. + message(FATAL_ERROR "NSPARSE_ENABLE_SANITIZERS is only supported with GCC and Clang") +endif() + +# Applied globally, before any target (including the FetchContent'ed abseil and +# GoogleTest) is defined: an ASan-instrumented translation unit and an +# uninstrumented one disagree about libstdc++ container annotations, which shows +# up as container-overflow false positives rather than a link error. +# +# -fno-sanitize-recover=all: by default UBSan prints and continues, so a +# findings-only run still exits 0 and ctest reports a pass. Aborting is what +# makes a UBSan finding fail CI. +set(NSPARSE_SANITIZER_FLAGS + -fsanitize=${NSPARSE_SANITIZERS} + -fno-sanitize-recover=all + -fno-omit-frame-pointer) + +# GCC's null-pointer checks make the address of a function template +# instantiation non-constant, which breaks abseil's constexpr +# `get_hash_slot_fn() == nullptr` dispatch (hash_policy_traits.h) -- every +# translation unit that includes flat_hash_map fails to compile. Clang is +# unaffected. Dropping the three checks costs little here: a null dereference +# still surfaces, as an ASan SEGV report. +if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND NSPARSE_SANITIZERS MATCHES "undefined") + list(APPEND NSPARSE_SANITIZER_FLAGS + -fno-sanitize=null,nonnull-attribute,returns-nonnull-attribute) +endif() + +add_compile_options(${NSPARSE_SANITIZER_FLAGS}) +add_link_options(${NSPARSE_SANITIZER_FLAGS}) + +message(STATUS "Sanitizers enabled: ${NSPARSE_SANITIZERS}")