Uh oh!
There was an error while loading. Please reload this page.
Add a blocking AddressSanitizer job - #180
Merged
Merged
Conversation
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=<list> passes <list> 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
HugoFara
commented
Aug 12, 2026
CollaboratorAuthor
Sanitizer only, merging. |
Uh oh!
There was an error while loading. Please reload this page.
antonio-leblanc pushed a commit
that referenced
this pull request
Aug 13, 2026
The CHANGELOG landed on master carries only #154 and #155 under [Unreleased], because that is all master has. This adds the ten pull requests merged into dev since: the unit suite, the moisture invariants, the sanitizer job, the HTTP characterisation tests, the threading reproduction, and the shared-state and double-free fixes. Two things went stale on dev while that work landed, both of them ours. The README's CMake option table stopped at six options; #156 and #180 added FOREFIRE_BUILD_TESTS, FOREFIRE_ENABLE_WARNINGS, FOREFIRE_WARNINGS_AS_ERRORS and FOREFIRE_SANITIZE. And CONTRIBUTING told contributors to run `cd tests && bash run.bash`, which does not reach the C++ unit tests at all; ctest does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#162. Adds an AddressSanitizer job that blocks on errors from the day it lands, plus the build option that makes the same build reproducible locally.
Why it can be blocking immediately
ForeFire reports zero ASan errors on both test paths — no use-after-free, no overflow, no double free. There is no backlog to clear before switching this on, which is unusual and worth capturing before it stops being true. The state refactor in #175 will start moving allocations around; better to have the check in place first than to add it afterwards and discover what changed.
Verified in
ubuntu:24.04, which is whatubuntu-latestruns-fsanitize=addressdetect_leaks=0runff,detect_leaks=0detect_leaks=1Negative control. A green job proves nothing unless it can go red, so I reintroduced the double free fixed in #157 into
ForeFireV1HeatFluxModeland rebuilt:So the job catches the class of bug this project has actually had, with a full allocation and free stack trace rather than glibc's bare abort.
Leaks are deliberately not blocking
ASAN_OPTIONS=detect_leaks=0on the two gating steps. Nothing owns aPropagationModel(#159), so ASan reports every one as leaked — the last row of the table is what the job would look like with leak detection on. A permanently red job is a job everyone learns to ignore, which is the same failure mode as theERROR:-prefixed messages in #165.The leak count still runs, as an informational step with
continue-on-error, so the figure stays visible in the log and can be made blocking once #159 lands. At that point ~4.7 MB becomes a regression check rather than a known quantity.FOREFIRE_SANITIZEas a build option-DFOREFIRE_SANITIZE=addressrather than rawCMAKE_CXX_FLAGSin the workflow, so anyone can reproduce a CI failure with one flag. It reaches the compile line, the executables and the shared library — the core is where the allocations happen, so linking it without the library would miss most of them.It also switches the optimisation flags to
-g -O1 -fno-omit-frame-pointer. The default non-MPI set is-O3 -flto -fomit-frame-pointer -finline-functions -funroll-loops, which strips exactly what a sanitizer needs for a readable stack trace. That is why this is a separate workflow rather than a job inmain.yml:install-forefire.shbuilds with the release flags.Other values pass straight through —
undefined, oraddress,undefined— but onlyaddressis verified clean here, so nothing claims UBSan is quiet.This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.