fix: Silence UBSan diagnostics in the ubsan build config#7531
fix: Silence UBSan diagnostics in the ubsan build config#7531pratikmankawde wants to merge 8 commits into
Conversation
The debian-trixie-clang-22-amd64-debug-ubsan job logs many non-fatal
UBSan `runtime error:` lines (it passes because halt_on_error=false).
These come from two places that needed different fixes.
Build-time: the protocol code-gen and build steps run instrumented
dependency tools (protoc, grpc) before the "Set sanitizer options" step
exported UBSAN_OPTIONS, so the suppression list never applied to them.
Move that step to run right after "Configure CMake" — before code-gen
and build — reusing the existing ${GITHUB_WORKSPACE}/$GITHUB_ENV
mechanism so the path resolves correctly inside the container job.
Test-time: extend ubsan.supp. unsigned-integer-overflow and
float-divide-by-zero are separate checks, not part of the `undefined`
group, so undefined:<x> keys do not cover them. Add broad
unsigned-integer-overflow keys for absl and protobuf (matching the
existing boost handling), a few libstdc++ headers, and narrowly
function-scoped entries for rippled's intentional wraparound (hashing,
PRNG, counters guarded by explicit overflow checks) plus test
arithmetic, each with a rationale comment.
Fix the one genuine issue in code: a diagnostic JLOG line in
doLedgerGrpc divided the extract time by the object / transaction
counts, which are zero for an empty ledger. Guard the per-item rates.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The function-name suppressions for rippled's own code never matched: this UBSan build runs without symbol information, so the runtime only reports the file:line of each diagnostic, not the enclosing function. The CI log still showed ~486 unsuppressed `runtime error:` lines, all from own-code files whose entries were written as function-name globs. Re-key every rippled own-code entry by source file (the only granularity the runtime can match here), covering all files flagged by the ubsan job: amount/number arithmetic, hashing and PRNG, counter/sentinel wraparound guarded by explicit checks, codec index math, and intentional test arithmetic. Each entry keeps a comment naming the construct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7531 +/- ##
=======================================
Coverage 82.0% 82.0%
=======================================
Files 1009 1009
Lines 76779 76780 +1
Branches 8937 8937
=======================================
+ Hits 62980 62982 +2
+ Misses 13790 13789 -1
Partials 9 9
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mathbunnyru
left a comment
There was a problem hiding this comment.
Can we make halt_on_error=True?
If I understand correctly, right now even if there is an error, unless someone notices it in logs, in won't crash anything and tests will still tell "passed"?
The current state of the code is not clean enough to allow |
Address review feedback: the Set sanitizer options step repeated
${GITHUB_WORKSPACE}/sanitizers/suppressions/ for every sanitizer.
Extract it into a SUPP shell variable for readability. No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…' into pratik/ubsan-suppression-cleanup
Now that every UBSan diagnostic in the build and test phases is either fixed or suppressed, flip halt_on_error to true so any new undefined behavior fails the job instead of being logged and ignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Seems to work now 🎉 |
Already done earlier when runs were failing and I had to set this to false. |
High Level Overview of Change
The
debian-trixie-clang-22-amd64-debug-ubsanjob logs many non-fatal UBSanruntime error:lines. The job already passes (UBSan runs withhalt_on_error=false); this PR silences the noise. Three changes: move the workflow's sanitizer-options step earlier so suppressions apply to instrumented dependency tools, extendubsan.supp, and fix one genuine divide-by-zero in code.Context of Change
This is a CI/build-config cleanup, not a behavior change.
Why existing suppressions didn't catch these. The build uses
-fsanitize=undefined,float-divide-by-zero,unsigned-integer-overflow. The latter two are separate checks, not part of theundefinedgroup (unsigned wraparound and IEEE divide-by-zero are well-defined, not UB). So anundefined:<x>suppression does not silence them — each needs its own check-name key. This is whyundefined:protobufleft protobuf'sunsigned-integer-overflowleaks unsuppressed.Build-time fix (workflow). The protocol code-gen and build steps run instrumented dependency tools (
protoc,grpc) before theSet sanitizer optionsstep exportedUBSAN_OPTIONS, so the suppression list never applied to them. The step is moved to run right afterConfigure CMake— before code-gen and build — reusing the existing${GITHUB_WORKSPACE}→$GITHUB_ENVmechanism so the path resolves correctly inside the container job.Test-time fix (
ubsan.supp). Broadunsigned-integer-overflowkeys forabslandprotobuf(matching the existingboosthandling), a few libstdc++ headers, and entries for rippled's intentional wraparound (hashing, PRNG, guarded counters) and test arithmetic. Own-code entries are keyed by source file: this UBSan build runs without symbol info, so the runtime reports onlyfile:line, not the enclosing function — function-name globs never matched.Code fix. A diagnostic
JLOGline indoLedgerGrpcdivided the extract time by the object / transaction counts, which are zero for an empty ledger (float-divide-by-zero). The per-item rates are now guarded.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
…-ubsanjob: confirm zeroruntime error:lines in both the build and the embedded-test steps.