feat: add sanitizer profiles and validation tests - #918
niteshpurohit wants to merge 4 commits into
Conversation
- Introduced support for AddressSanitizer (ASan), UndefinedBehaviorSanitizer (UBSan), and ThreadSanitizer (TSan) profiles. - Added new CMake functions for configuring sanitizer profiles and validating suppressions. - Created test fixtures for heap misuse and data race scenarios to validate sanitizer functionality. - Implemented validation scripts to ensure proper usage of sanitizer suppressions. - Updated toolchain and build configurations to integrate sanitizer options seamlessly. closes: #74
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect CI installation, sanitizer linking, fixture validation, suppression handling, and build correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable ASan/UBSan and TSan profiles, CMake integration, sanitizer fixtures, suppression validation, and CI coverage.
Changes:
- Adds sanitizer profiles, presets, toolchain flags, and build identity metadata.
- Adds heap, undefined-behavior, and data-race fixtures.
- Adds suppression validation and sanitizer CI jobs.
File summaries
| File | Reviewed change / final review note |
|---|---|
tests/sanitizers/undefined_behavior.cpp |
UBSan overflow fixture. |
tests/sanitizers/suppressions.tsv |
Suppression manifest. |
tests/sanitizers/negative/third-party.tsv |
Invalid third-party suppression fixture. |
tests/sanitizers/negative/missing-reason.tsv |
Missing-reason suppression fixture. |
tests/sanitizers/negative/invalid-sanitizer.tsv |
Invalid-sanitizer suppression fixture. |
tests/sanitizers/negative/global.tsv |
Invalid global-suppression fixture. |
tests/sanitizers/heap_misuse.cpp |
ASan heap fixture. Critical (1 vote): warnings-as-errors may reject the raw-pointer subscript before ASan runs. |
tests/sanitizers/data_race.cpp |
TSan data-race fixture. |
CMakePresets.json |
Sanitizer presets. |
CMakeLists.txt |
Sanitizer setup and target integration. |
cmake/ValidateSanitizerSuppression.cmake |
Suppression validation entry point. |
cmake/LaghuToolchain.cmake |
Sanitizer flags and validation tests. Critical (3 votes): link options are applied to static/object targets that cannot accept them. |
cmake/LaghuSanitizers.cmake |
Profile and fixture implementation. Moderate (3 votes): cached flags can remain stale across reconfiguration. Moderate (1 vote): whitespace-only reasons are accepted. Moderate (1 vote): normal UBSan runs may not fail on findings. |
cmake/LaghuBuildIdentity.cmake |
Sanitizer profile identity metadata. |
cmake/ExpectSanitizerSuppression.cmake |
Negative validation harness. Moderate (1 vote): unrelated child failures can satisfy the test. |
cmake/ExpectSanitizerReleaseExclusion.cmake |
Release artifact check. |
cmake/ExpectSanitizerFailure.cmake |
Fixture failure assertions. Moderate (1 vote each): require specific heap-buffer-overflow and TSan data-race diagnostics. |
.github/workflows/toolchain.yml |
Sanitizer CI jobs. Critical (1 vote): the clang-19 installation lacks the required Ubuntu toolchain PPA. |
Review details
Suppressed comments (6)
.github/workflows/toolchain.yml:182
- This job has the same package-source gap as the ASan/UBSan job: it installs
clang-19onubuntu-24.04without adding theubuntu-toolchain-r/testPPA used by the existing Clang 19 setup at.github/workflows/toolchain.yml:33-35. The job can therefore fail atapt-get installbefore any TSan tests run. Add that PPA before updating apt.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
cmake/ExpectSanitizerFailure.cmake:27
- The race fixture is accepted for any nonzero TSan process whose output merely mentions
ThreadSanitizer; for example, a TSan startup failure such asunexpected memory mappingalso has that prefix. This can make CI pass without observing a race. Require the specificThreadSanitizer: data racediagnostic.
elseif(EXPECTED STREQUAL "data_race" AND NOT combined MATCHES "ThreadSanitizer")
message(FATAL_ERROR "Laghu sanitizer fixture expectation failed: fixture=data_race; report=missing_thread_sanitizer")
cmake/ExpectSanitizerFailure.cmake:22
- The heap fixture is accepted whenever the process exits nonzero and the output contains the generic
AddressSanitizerlabel. An ASan startup/runtime failure can contain that label without reporting the intended out-of-bounds access, allowing the gate to pass without validating this fixture. Match the specificheap-buffer-overflowdiagnostic instead.
if(EXPECTED STREQUAL "heap_misuse" AND NOT combined MATCHES "AddressSanitizer")
message(FATAL_ERROR "Laghu sanitizer fixture expectation failed: fixture=heap_misuse; report=missing_address_sanitizer")
cmake/ExpectSanitizerSuppression.cmake:14
- Checking only for a nonzero child result lets a syntax error, missing include, or missing manifest make every negative fixture pass. Assert that the child failed with the validator's
Laghu sanitizer profile failed:diagnostic (and, ideally, the fixture-specific reason) so these tests cannot pass on an unrelated script failure.
if(result EQUAL 0)
message(FATAL_ERROR "Laghu sanitizer suppression expectation failed: manifest=${MANIFEST}; rejection=missing")
endif()
cmake/LaghuSanitizers.cmake:45
- The required-reason check only compares the raw field to an empty string, so a suppression whose fourth TSV field is spaces is accepted as having a technical reason. Trim the field before checking it so the validator enforces a non-blank justification.
if(reason STREQUAL "")
laghu_sanitizer_fail("target_or_source=${target_or_source}; technical reason is required")
cmake/LaghuSanitizers.cmake:66
-fsanitize=undefineduses recoverable checks by default. The onlyUBSAN_OPTIONS=halt_on_error=1here is passed to the seeded fixture wrapper; the normal-suite workflow invokeslaghu_verify_toolchainwithout it, so an UBSan report in an ordinary test can be printed while the test still exits successfully. Add-fno-sanitize-recover=undefined(or propagateUBSAN_OPTIONSto the full CTest run) so findings fail CI.
set(compile_options -fsanitize=address,undefined -fno-omit-frame-pointer)
set(link_options -fsanitize=address,undefined)
- Files reviewed: 18/18 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Updated ExpectSanitizerFailure.cmake to check for specific error messages related to heap misuse and data races. - Introduced ExpectSanitizerReconfigure.cmake to validate sanitizer configurations and ensure proper flags are set. - Enhanced ExpectSanitizerReleaseExclusion.cmake to include BUILD_DIRECTORY in error checks. - Modified ExpectSanitizerSuppression.cmake to require both MANIFEST and EXPECTED_DIAGNOSTIC for validation. - Improved LaghuSanitizers.cmake to handle varying field counts in suppression manifests and added checks for technical reasons. - Added new test case for whitespace reason in sanitizer suppression.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved sanitizer build, CI coverage, artifact-check, and suppression-validation issues remain.
Review details
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
cmake/ExpectSanitizerReleaseExclusion.cmake:25
- This check searches the symbol table for the CMake target-name prefix
laghu_sanitizer_, but target names are not emitted as symbols. The fixture sources only define symbols such asmain,shared_value, andincrement, so the test passes even if a fixture object is accidentally included in an installed archive or executable. Check archive/object membership or add a uniquely named fixture marker symbol instead.
.github/workflows/toolchain.yml:168
- This job installs the same
clang-19/libc++ packages as the matrix job above, but it omits theppa:ubuntu-toolchain-r/testrepository setup used at.github/workflows/toolchain.yml:31-35. On theubuntu-24.04runner those packages are not guaranteed to be available from the default repositories, so the sanitizer job can fail before configuration. Add the repository beforeapt-get update, matching the existing toolchain setup.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
.github/workflows/toolchain.yml:182
- This job installs the same
clang-19/libc++ packages as the matrix job above, but it omits theppa:ubuntu-toolchain-r/testrepository setup used at.github/workflows/toolchain.yml:31-35. On theubuntu-24.04runner those packages are not guaranteed to be available from the default repositories, so the sanitizer job can fail before configuration. Add the repository beforeapt-get update, matching the existing toolchain setup.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
.github/workflows/toolchain.yml:172
- This preset inherits the default
MINIMALbuild profile, so this sanitizer job never buildslaghu_cryptoorlaghu_crypto_provider_test. The removed matrix step was the only ASan/UBSan coverage for the TLS adapter; retain a sanitizer TLS build (for example, a secondCUSTOM/tlsconfigure) so this refactor does not silently drop that coverage.
cmake --preset linux-clang-asan-ubsan
cmake --build build/linux-clang-asan-ubsan --target laghu_verify_toolchain
cmake/LaghuSanitizers.cmake:44
- For
src/...entries this only validates the pathname shape; a suppression such assrc/core/no_such_file.cppis accepted even though it can never apply. The warning-suppression validator rejects nonexistent governed sources (cmake/LaghuToolchain.cmake:189-190); apply the same existence check here, passing the source root explicitly to the standalone validation script rather than relying on its working directory.
if(NOT target_or_source MATCHES "^(laghu_[A-Za-z0-9_]+|src/(core|config|protocol|tls|proxy|cache|control|cli|observability|os)/[A-Za-z0-9_./-]+\\.(cpp|cc|cxx))$")
laghu_sanitizer_fail("target_or_source=${target_or_source}; expected=one_laghu_target_or_governed_source")
cmake/LaghuToolchain.cmake:280
target_link_optionscannot be applied to static or object libraries because those targets do not perform a link. This function is called forlaghu_core,laghu_os, the support static libraries, andlaghu_capability_header_parity, so any ASAN/UBSAN or TSAN configure will fail before building. Apply the sanitizer link options only to executable/shared/module targets; keep the compile options on all first-party targets.
if(NOT LAGHU_SANITIZER_LINK_OPTIONS STREQUAL "")
target_link_options("${target}" PRIVATE ${LAGHU_SANITIZER_LINK_OPTIONS})
tests/sanitizers/suppressions.tsv:2
- The canonical suppression manifest contains no valid entries, and every added suppression test is a rejection case. Consequently, a validator that rejects all valid target/source entries would still pass the suite. Add a positive fixture (covering at least a
laghu_*target and a governed source) and assert that it is accepted.
# sanitizer\tplatform\ttarget_or_source\ttechnical reason
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Improved the sanitizer suppression validation by adding checks for the expected number of fields. - Enhanced error handling for invalid sanitizer profiles to provide clearer feedback. - Streamlined the extraction of sanitizer details from the manifest for better readability and maintainability.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate CI, sanitizer-validation, and target-configuration issues remain.
Review details
Suppressed comments (13)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/toolchain.yml:171
- This preset leaves
LAGHU_BUILD_PROFILEat its defaultMINIMAL, sotlsis not requested andlaghu_crypto_provider_testis not built or run. The removed matrix step explicitly configuredCUSTOMwithtlsunder ASan/UBSan; replacing it with this job regresses sanitizer coverage for the crypto adapter. Preserve that coverage by configuring this job with-DLAGHU_BUILD_PROFILE=CUSTOM -DLAGHU_FEATURES=tls(or add a separate sanitized crypto invocation).
cmake/LaghuSanitizers.cmake:49 - The governed-source regex allows
.and..path components through[A-Za-z0-9_./-]+; for example,src/core/../tests/foo.cpppasses even though it resolves outside the allowed source directories. That bypasses the validator's narrow-suppression policy. Reject./..path segments or canonicalize the path before applying the directory allowlist.
.github/workflows/toolchain.yml:168
- These standalone jobs install
clang-19without adding the Ubuntu Toolchain PPA, while the existing clang matrix addsppa:ubuntu-toolchain-r/testbefore installing the same compiler. On the clean Ubuntu 24.04 runner,apt-get installcan therefore fail before either sanitizer gate runs; add the repository setup here or use a compiler available from the configured repositories.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
.github/workflows/toolchain.yml:182
- The TSan job has the same clean-runner dependency problem: it installs
clang-19but never adds the PPA used by the existing clang job. This can prevent the concurrency sanitizer gate from configuring at all; add the repository setup beforeapt-get updateor select an available compiler.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
CMakePresets.json:34
- The new sanitizer presets are public configure entry points, but both
README.md:19anddocs/normative/build.md:38still list only the pre-existing presets. Users following the documented preset list will not discover the ASan/UBSan or TSan profiles; update those lists with the new presets.
"name": "linux-clang-asan-ubsan",
"displayName": "Linux Clang ASan and UBSan",
"inherits": "linux-clang",
"cacheVariables": {
"LAGHU_SANITIZER_PROFILE": "ASAN_UBSAN"
}
cmake/ExpectSanitizerReleaseExclusion.cmake:25
- The fixtures do not define any symbol whose name contains
laghu_sanitizer_(heap_misuse.cppandundefined_behavior.cpponly definemain, while the race fixture's helpers are anonymous), so thisnmcheck passes regardless of whether fixture code is present. Add a stable, uniquely named marker symbol to each fixture and check those symbols, or validate the product's link inputs instead; otherwise the release-exclusion test is vacuous.
if(symbols MATCHES "laghu_sanitizer_")
message(FATAL_ERROR "Laghu sanitizer release exclusion failed: artifact=${artifact}; test_symbol=present")
cmake/LaghuSanitizers.cmake:49
- The source-form branch validates only the pathname pattern, so an entry such as
address<TAB>linux<TAB>src/core/not-a-file.cpp<TAB>reasonis accepted even though it can never suppress anything. The warning suppression validator rejects nonexistent governed sources (cmake/LaghuToolchain.cmake:189-190); apply the same existence check tosrc/...sanitizer entries and make the standalone validator receive the source root.
if(NOT target_or_source MATCHES "^(laghu_[A-Za-z0-9_]+|src/(core|config|protocol|tls|proxy|cache|control|cli|observability|os)/[A-Za-z0-9_./-]+\\.(cpp|cc|cxx))$")
laghu_sanitizer_fail("target_or_source=${target_or_source}; expected=one_laghu_target_or_governed_source")
endif()
cmake/LaghuToolchain.cmake:281
laghu_apply_first_party_contractis called for static and object libraries such aslaghu_coreandlaghu_capability_header_parity. CMake does not allowtarget_link_options()on targets without a link step, so any non-NONEprofile will fail during configuration before the sanitizer build can start. Apply these options only to executable/shared/module targets; the executable link options are sufficient to pull in the sanitizer runtime.
if(NOT LAGHU_SANITIZER_LINK_OPTIONS STREQUAL "")
target_link_options("${target}" PRIVATE ${LAGHU_SANITIZER_LINK_OPTIONS})
endif()
tests/sanitizers/negative/global.tsv:1
- All existing test manifest TSVs, including
tests/warnings/negative/*.tsv, begin with the repository's SPDX header, but this newly added manifest does not. Add the header so the fixture follows the repository licensing convention.
address linux all global suppression is forbidden
tests/sanitizers/negative/invalid-sanitizer.tsv:1
- All existing test manifest TSVs, including
tests/warnings/negative/*.tsv, begin with the repository's SPDX header, but this newly added manifest does not. Add the header so the fixture follows the repository licensing convention.
memory linux laghu_core invalid sanitizer is forbidden
tests/sanitizers/negative/missing-reason.tsv:1
- All existing test manifest TSVs, including
tests/warnings/negative/*.tsv, begin with the repository's SPDX header, but this newly added manifest does not. Add the header so the fixture follows the repository licensing convention.
thread linux laghu_core
tests/sanitizers/negative/third-party.tsv:1
- All existing test manifest TSVs, including
tests/warnings/negative/*.tsv, begin with the repository's SPDX header, but this newly added manifest does not. Add the header so the fixture follows the repository licensing convention.
address linux third_party/library.cpp third-party masking is forbidden
tests/sanitizers/negative/whitespace-reason.tsv:1
- All existing test manifest TSVs, including
tests/warnings/negative/*.tsv, begin with the repository's SPDX header, but this newly added manifest does not. Add the header so the fixture follows the repository licensing convention.
thread linux laghu_core
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate findings remain in CI toolchain setup, sanitizer link-option application, and positive suppression-parser coverage.
Review details
Suppressed comments (4)
.github/workflows/toolchain.yml:168
- This Ubuntu 24.04 job installs
clang-19andlibc++-19-devwithout adding theppa:ubuntu-toolchain-r/testsource that the existing Clang matrix uses at.github/workflows/toolchain.yml:33-35. Those packages are not supplied by the runner's default Noble repositories, so the job can fail during setup before any sanitizer test runs; add the same repository setup here.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
.github/workflows/toolchain.yml:196
- This Ubuntu 24.04 job installs
clang-19and the libc++ 19 development packages without adding theppa:ubuntu-toolchain-r/testsource that the existing Clang matrix uses at.github/workflows/toolchain.yml:33-35. The default Noble repositories do not provide this toolchain reliably, so the TSan job can fail during package installation before running its tests; mirror that repository setup here.
sudo apt-get update
sudo apt-get install --yes ninja-build clang-19 libc++-19-dev libc++abi-19-dev
cmake/LaghuSanitizers.cmake:38
- The committed
tests/sanitizers/suppressions.tsvcontains only a header, and every sanitizer manifest fixture is intentionally rejected, so the successful non-comment parsing path here (including a valid four-field target/source entry) is never exercised. A parser regression could therefore make all real suppressions invalid without failing CI; add a positive manifest/test case using an existing governed source or target.
string(REPLACE "\t" ";" fields "${line}")
list(LENGTH fields field_count)
if(field_count EQUAL 3)
list(GET fields 0 sanitizer)
list(GET fields 1 platform)
list(GET fields 2 target_or_source)
set(reason "")
elseif(NOT field_count EQUAL 4)
laghu_sanitizer_fail("manifest=${manifest}; expected=sanitizer-platform-target_or_source-reason")
else()
list(GET fields 0 sanitizer)
list(GET fields 1 platform)
list(GET fields 2 target_or_source)
list(GET fields 3 reason)
endif()
cmake/LaghuToolchain.cmake:280
laghu_apply_first_party_contractis also called for static libraries such aslaghu_core(CMakeLists.txt:41,48) and the sanitizer profile makes this branch execute for them. CMake does not permittarget_link_optionson static/object library targets because they have no link step, so configuring either sanitizer preset fails before the build starts. Apply sanitizer link options only to linkable targets (executables/shared/module libraries); the final executables already carry the runtime link flags.
if(NOT LAGHU_SANITIZER_LINK_OPTIONS STREQUAL "")
target_link_options("${target}" PRIVATE ${LAGHU_SANITIZER_LINK_OPTIONS})
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
closes: #74