From f2701250775ceaa6b76e0fb9829cb44039cb09a5 Mon Sep 17 00:00:00 2001 From: HugoFara Date: Wed, 12 Aug 2026 21:27:59 +0200 Subject: [PATCH] ci: add a blocking AddressSanitizer job ForeFire reports zero ASan errors today on both test paths, so the job can be blocking from the day it lands rather than after a cleanup. The point of turning it on now is to keep that property while the ownership work in #175 starts moving allocations around. -DFOREFIRE_SANITIZE= passes to -fsanitize= on the compile line, the executables and the shared library, and switches the optimisation flags to -g -O1 -fno-omit-frame-pointer: the default -O3 -flto -fomit-frame-pointer set makes the reports unreadable. Having it as a build option rather than raw CMAKE_CXX_FLAGS in the workflow means the same build is reproducible locally. The job runs the unit suite and runff under ASAN_OPTIONS=detect_leaks=0. Errors block; leaks do not, and that is deliberate. Nothing owns a PropagationModel (#159), so ASan reports every one as leaked, and a job that is permanently red is a job everyone ignores. The leak count still runs as an informational step so the figure stays visible, and it can be made blocking once #159 lands. Verified in ubuntu:24.04, which is what ubuntu-latest runs: build with -fsanitize=address clean unit suite, detect_leaks=0 4/4, 0 errors runff, detect_leaks=0 passes, KML and NetCDF match leak report, detect_leaks=1 exits 8, ~4.7 MB reported Negative control: reintroducing the double free fixed in #157 into ForeFireV1HeatFluxModel turns the job red with ERROR: AddressSanitizer: attempting double-free SUMMARY: AddressSanitizer: double-free in operator delete[](void*) so the job catches the class of bug this project has actually had. Closes #162 --- .github/workflows/sanitizers.yml | 81 ++++++++++++++++++++++++++++++++ CMakeLists.txt | 16 +++++++ TESTING.md | 29 ++++++++++++ 3 files changed, 126 insertions(+) create mode 100644 .github/workflows/sanitizers.yml diff --git a/.github/workflows/sanitizers.yml b/.github/workflows/sanitizers.yml new file mode 100644 index 0000000..aef0c39 --- /dev/null +++ b/.github/workflows/sanitizers.yml @@ -0,0 +1,81 @@ +name: ๐Ÿงผ Sanitizers (Linux) + +# AddressSanitizer over both test paths: the unit suite, which constructs and +# destroys every propagation and flux model one call at a time, and runff, +# which runs a full simulation with a reload and NetCDF and KML output. +# +# This is a blocking check from the day it lands, which is only possible +# because ForeFire reports zero ASan errors today โ€” no use-after-free, no +# overflow, no double free, on either path. There is no backlog to clear +# first, and the point of turning it on now is to keep it that way. See #162. +# +# Leaks are a separate matter and are deliberately NOT blocking: nothing owns +# a PropagationModel (#159), so ASan reports every one as leaked. Making that +# fail the build would produce a permanently red job, which teaches everyone +# to ignore it. The leak check runs anyway, as an informational step, so the +# number stays visible and can be made blocking once #159 lands. +# +# A separate workflow rather than a job in main.yml because it needs its own +# build: install-forefire.sh builds with -O3 -flto -fomit-frame-pointer, which +# is the opposite of what a sanitizer wants. + +on: + push: + branches: + - "master" + - "dev" + pull_request: + branches: [ "master", "dev" ] + workflow_dispatch: + +jobs: + address-sanitizer: + name: AddressSanitizer + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - name: Checkout repository + uses: actions/checkout@v6 + with: + lfs: true # runff reads data.nc, which is stored in LFS + + - name: Install Dependencies + run: | + sudo apt-get update -y + sudo apt-get install -y --no-install-recommends \ + build-essential cmake python3 python3-pip + sudo apt-get install -y --no-install-recommends \ + libnetcdf-dev libnetcdf-c++4-dev + pip3 install --no-cache-dir --break-system-packages lxml xarray netCDF4 + + - name: Build with AddressSanitizer + # FOREFIRE_SANITIZE also switches the compile flags to -g -O1 + # -fno-omit-frame-pointer, so the stack traces are readable. + run: | + cmake -S . -B build-asan -DFOREFIRE_SANITIZE=address + cmake --build build-asan -j"$(nproc)" + + # Errors are blocking. detect_leaks=0 is the whole reason this can be: + # see the note at the top about #159. + - name: Unit tests under ASan + env: + ASAN_OPTIONS: detect_leaks=0 + run: ctest --test-dir build-asan --output-on-failure + + - name: runff under ASan + env: + ASAN_OPTIONS: detect_leaks=0 + run: | + cd tests/runff + bash ff-run.bash + + # Informational only. Prints what is leaking so the figure stays visible + # without gating the build on a known ownership gap. + - name: Leak report (informational) + continue-on-error: true + env: + ASAN_OPTIONS: detect_leaks=1 + run: | + echo "::group::Unit suite leaks" + ctest --test-dir build-asan --output-on-failure || true + echo "::endgroup::" diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c402cf..a11c7b1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -52,6 +52,8 @@ option(FOREFIRE_CHECK_LFS "Run the Git LFS data integrity check at configure tim option(FOREFIRE_BUILD_TESTS "Build the C++ unit tests, registered with CTest" ${_ff_default_tests}) option(FOREFIRE_ENABLE_WARNINGS "Compile ForeFire's own sources with -Wall -Wextra" ON) option(FOREFIRE_WARNINGS_AS_ERRORS "Fail the build on a compiler warning" OFF) +set(FOREFIRE_SANITIZE "" CACHE STRING + "Sanitizers to build with, passed to -fsanitize= (e.g. address, undefined, address,undefined)") # ---------------------------------- # Set C++ Standard @@ -220,6 +222,11 @@ endif() # ---------------------------------- if(MPI_FOUND) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -O0 -DMPI_COUPLING") +elseif(FOREFIRE_SANITIZE) + # A sanitizer build wants frame pointers and debug info; the release set + # below strips both, and -flto makes the reports harder to read besides. + # -O1 keeps the suites quick enough to run in CI. + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -O1 -fno-omit-frame-pointer") else() # uncomment if want to debug # set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -O0") @@ -229,6 +236,15 @@ else() endif() endif() +# Sanitizers have to reach the link line as well as the compile line, and the +# shared library too: the core is where the allocations happen. +if(FOREFIRE_SANITIZE) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=${FOREFIRE_SANITIZE}") + set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=${FOREFIRE_SANITIZE}") + set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=${FOREFIRE_SANITIZE}") + message(STATUS "Sanitizers enabled: -fsanitize=${FOREFIRE_SANITIZE}") +endif() + # Warnings are a target property rather than a global flag on purpose: they # should apply to code this project writes, not to pybind11's headers or to # whatever a consumer compiles against the installed library. diff --git a/TESTING.md b/TESTING.md index c42ea85..13d0f7f 100644 --- a/TESTING.md +++ b/TESTING.md @@ -115,6 +115,35 @@ for the right reason. The `tests/` directory contains other subdirectories (`mnh_*`, `runANN`) for testing specific features like coupled simulations. A main `tests/run.bash` script exists but is not currently fully validated in CI. Refer to specific subdirectories for details if needed. +## Sanitizers + +`-DFOREFIRE_SANITIZE=` builds with `-fsanitize=`, applied to the +compile line, the executables and the shared library. It also switches the +optimisation flags to `-g -O1 -fno-omit-frame-pointer`, since the default +release set (`-O3 -flto -fomit-frame-pointer`) makes sanitizer reports hard to +read. + +```bash +cmake -S . -B build-asan -DFOREFIRE_SANITIZE=address +cmake --build build-asan -j +ASAN_OPTIONS=detect_leaks=0 ctest --test-dir build-asan --output-on-failure +``` + +`address` is what CI runs, on both the unit suite and `runff`, as a blocking +check. Other values are passed straight through โ€” `undefined`, or +`address,undefined` for both โ€” but only `address` is currently verified clean. + +**`detect_leaks=0` is deliberate, not a workaround.** ForeFire reports zero +ASan *errors* โ€” no use-after-free, no overflow, no double free โ€” on either test +path, which is what makes a blocking job possible. It does leak: nothing owns a +`PropagationModel` (#159), so every one is reported. Leaving leak detection on +would produce a permanently failing job that everyone learns to ignore. The CI +workflow runs the leak check anyway as an informational step, so the number +stays visible, and it can be made blocking once #159 lands. + +Note that the sanitizer build writes `bin/forefire` and `lib/libforefireL.so` +like any other build, so it replaces a release build in the source tree. + ## Compiler Warnings ForeFire's own sources compile with `-Wall -Wextra` by default. The warnings