Skip to content

feat: add sanitizer profiles and validation tests - #918

Open
niteshpurohit wants to merge 4 commits into
feat/test-deterministic-time-entropyfrom
feat/sanitizer-profiles
Open

niteshpurohit wants to merge 4 commits into
feat/test-deterministic-time-entropyfrom
feat/sanitizer-profiles

Conversation

@niteshpurohit

Copy link
Copy Markdown
Member
  • 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

- 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
@niteshpurohit niteshpurohit self-assigned this Sep 11, 2026
Copilot AI lite review requested due to automatic review settings September 11, 2026 00:49
@niteshpurohit
niteshpurohit added this pull request to stack #915 September 11, 2026 00:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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-19 on ubuntu-24.04 without adding the ubuntu-toolchain-r/test PPA used by the existing Clang 19 setup at .github/workflows/toolchain.yml:33-35. The job can therefore fail at apt-get install before 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 as unexpected memory mapping also has that prefix. This can make CI pass without observing a race. Require the specific ThreadSanitizer: data race diagnostic.
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 AddressSanitizer label. 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 specific heap-buffer-overflow diagnostic 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=undefined uses recoverable checks by default. The only UBSAN_OPTIONS=halt_on_error=1 here is passed to the seeded fixture wrapper; the normal-suite workflow invokes laghu_verify_toolchain without it, so an UBSan report in an ordinary test can be printed while the test still exits successfully. Add -fno-sanitize-recover=undefined (or propagate UBSAN_OPTIONS to 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.

Comment thread .github/workflows/toolchain.yml
Comment thread cmake/LaghuToolchain.cmake
Comment thread tests/sanitizers/heap_misuse.cpp
Comment thread cmake/LaghuSanitizers.cmake Outdated
- 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.
Copilot AI review requested due to automatic review settings September 11, 2026 01:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 as main, shared_value, and increment, 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 the ppa:ubuntu-toolchain-r/test repository setup used at .github/workflows/toolchain.yml:31-35. On the ubuntu-24.04 runner those packages are not guaranteed to be available from the default repositories, so the sanitizer job can fail before configuration. Add the repository before apt-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 the ppa:ubuntu-toolchain-r/test repository setup used at .github/workflows/toolchain.yml:31-35. On the ubuntu-24.04 runner those packages are not guaranteed to be available from the default repositories, so the sanitizer job can fail before configuration. Add the repository before apt-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 MINIMAL build profile, so this sanitizer job never builds laghu_crypto or laghu_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 second CUSTOM/tls configure) 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 as src/core/no_such_file.cpp is 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_options cannot be applied to static or object libraries because those targets do not perform a link. This function is called for laghu_core, laghu_os, the support static libraries, and laghu_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.
Copilot AI review requested due to automatic review settings September 11, 2026 01:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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_PROFILE at its default MINIMAL, so tls is not requested and laghu_crypto_provider_test is not built or run. The removed matrix step explicitly configured CUSTOM with tls under 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.cpp passes 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-19 without adding the Ubuntu Toolchain PPA, while the existing clang matrix adds ppa:ubuntu-toolchain-r/test before installing the same compiler. On the clean Ubuntu 24.04 runner, apt-get install can 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-19 but 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 before apt-get update or 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:19 and docs/normative/build.md:38 still 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.cpp and undefined_behavior.cpp only define main, while the race fixture's helpers are anonymous), so this nm check 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>reason is 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 to src/... 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_contract is called for static and object libraries such as laghu_core and laghu_capability_header_parity. CMake does not allow target_link_options() on targets without a link step, so any non-NONE profile 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

Copilot AI review requested due to automatic review settings September 11, 2026 02:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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-19 and libc++-19-dev without adding the ppa:ubuntu-toolchain-r/test source 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-19 and the libc++ 19 development packages without adding the ppa:ubuntu-toolchain-r/test source 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.tsv contains 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_contract is also called for static libraries such as laghu_core (CMakeLists.txt:41,48) and the sanitizer profile makes this branch execute for them. CMake does not permit target_link_options on 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ASan, UBSan, and TSan profiles

3 participants