From ecfb570ea45d154e126ecbcc57b3a2606f175db4 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 11 Jun 2026 16:42:14 +0100 Subject: [PATCH] ci: Silence UBSan diagnostics in the ubsan build config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 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 --- .../workflows/reusable-build-test-config.yml | 34 +++++--- sanitizers/suppressions/ubsan.supp | 85 +++++++++++++++++-- src/xrpld/rpc/handlers/ledger/Ledger.cpp | 14 +-- 3 files changed, 107 insertions(+), 26 deletions(-) diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 8cb5f8c46a..cf5e608927 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -164,6 +164,26 @@ jobs: ${CMAKE_ARGS} \ .. + # Export the sanitizer options before any instrumented binary runs. The + # protocol code-gen and build steps below invoke instrumented dependency + # tools (protoc, grpc), so setting UBSAN_OPTIONS here lets the UBSan + # suppression list silence their diagnostics too, not just at test time. + # GITHUB_WORKSPACE (not the github.workspace context) is used so the path + # resolves correctly inside the container job. + - name: Set sanitizer options + if: ${{ !inputs.build_only && env.SANITIZERS_ENABLED == 'true' }} + env: + CONFIG_NAME: ${{ inputs.config_name }} + run: | + ASAN_OPTS="include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-asan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/asan.supp" + if [[ "${CONFIG_NAME}" == *gcc* ]]; then + ASAN_OPTS="${ASAN_OPTS}:alloc_dealloc_mismatch=0" + fi + echo "ASAN_OPTIONS=${ASAN_OPTS}" >>${GITHUB_ENV} + echo "TSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-tsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >>${GITHUB_ENV} + echo "UBSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-ubsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/ubsan.supp" >>${GITHUB_ENV} + echo "LSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-lsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/lsan.supp" >>${GITHUB_ENV} + - name: Check protocol autogen files are up-to-date working-directory: ${{ env.BUILD_DIR }} env: @@ -279,20 +299,6 @@ jobs: run: | ./xrpld --version | grep libvoidstar - - name: Set sanitizer options - if: ${{ !inputs.build_only && env.SANITIZERS_ENABLED == 'true' }} - env: - CONFIG_NAME: ${{ inputs.config_name }} - run: | - ASAN_OPTS="include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-asan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/asan.supp" - if [[ "${CONFIG_NAME}" == *gcc* ]]; then - ASAN_OPTS="${ASAN_OPTS}:alloc_dealloc_mismatch=0" - fi - echo "ASAN_OPTIONS=${ASAN_OPTS}" >>${GITHUB_ENV} - echo "TSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-tsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >>${GITHUB_ENV} - echo "UBSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-ubsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/ubsan.supp" >>${GITHUB_ENV} - echo "LSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-lsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/lsan.supp" >>${GITHUB_ENV} - - name: Run the separate tests if: ${{ !inputs.build_only }} working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', env.BUILD_DIR, inputs.build_type) || env.BUILD_DIR }} diff --git a/sanitizers/suppressions/ubsan.supp b/sanitizers/suppressions/ubsan.supp index 88d8e82e33..0f12b58efd 100644 --- a/sanitizers/suppressions/ubsan.supp +++ b/sanitizers/suppressions/ubsan.supp @@ -71,7 +71,12 @@ vptr_check:boost vptr:boost # Google protobuf - intentional overflows in hash functions +# unsigned-integer-overflow is a separate check that is NOT part of the +# `undefined` group, so the `undefined:protobuf` line above does not cover it. +# Match protobuf broadly for this check (arena.cc unsigned negation, +# serial_arena.h pointer-size arithmetic). undefined:protobuf +unsigned-integer-overflow:protobuf unsigned-integer-overflow:google/protobuf/stubs/stringpiece.h # gRPC intentional overflows in timer calculations @@ -102,24 +107,29 @@ undefined:nudb # Snappy compression library intentional overflows unsigned-integer-overflow:snappy.cc -# Abseil intentional overflows -unsigned-integer-overflow:absl/strings/numbers.cc -unsigned-integer-overflow:absl/strings/internal/cord_rep_flat.h -unsigned-integer-overflow:absl/base/internal/low_level_alloc.cc -unsigned-integer-overflow:absl/hash/internal/hash.h -unsigned-integer-overflow:absl/container/internal/raw_hash_set.h +# Abseil intentional overflows in hashing, RNG and time arithmetic. +# Matched at library scope (like boost above): the wraparound is by design +# across many absl files (hash mixing, raw_hash_set probing, duration math, +# int128, uniform_int_distribution), so listing individual files just churns. +unsigned-integer-overflow:absl # Standard library intentional overflows unsigned-integer-overflow:basic_string.h +unsigned-integer-overflow:bits/align.h +unsigned-integer-overflow:bits/basic_string.tcc unsigned-integer-overflow:bits/chrono.h unsigned-integer-overflow:bits/random.h unsigned-integer-overflow:bits/random.tcc unsigned-integer-overflow:bits/stl_algobase.h +unsigned-integer-overflow:bits/string_view.tcc unsigned-integer-overflow:bits/uniform_int_dist.h unsigned-integer-overflow:string_view unsigned-integer-overflow:__random/seed_seq.h unsigned-integer-overflow:__charconv/traits.h unsigned-integer-overflow:__chrono/duration.h +# libstdc++ (std::__bit_ceil etc.) negates an unsigned width; is a +# distinct header from the bits/ directory so it needs its own entry. +unsigned-integer-overflow:include/c++/*/bit # ============================================================================= # Rippled code suppressions @@ -146,3 +156,66 @@ unsigned-integer-overflow:*STAmount*serialize* # nft::cipheredTaxon uses intentional uint32 wraparound (LCG permutation) unsigned-integer-overflow:cipheredTaxon + +# Signed negation of INT64_MIN in STAmount accessors. xrp()/iou()/mpt() and +# getInt64Value negate the mantissa when isNegative_; the mantissa is bounded +# well within int64_t by canonicalize(), so INT64_MIN cannot occur in practice. +# (The operator-*STAmount* entry above only covers binary/unary operator-.) +signed-integer-overflow:*STAmount*xrp* +signed-integer-overflow:*STAmount*iou* +signed-integer-overflow:*STAmount*mpt* +signed-integer-overflow:*STAmount*canonicalize* +signed-integer-overflow:getInt64Value* + +# Hash helpers using intentional unsigned wraparound (non-secure, speed-first). +# STPathElement::getHash multiplies/adds accumulators; XorShiftEngine is a PRNG. +unsigned-integer-overflow:*STPathElement*getHash* +unsigned-integer-overflow:*XorShiftEngine* + +# Number::normalizeToRange multiplies the mantissa by powers of ten; the result +# is intentionally allowed to wrap while searching for the in-range value. +unsigned-integer-overflow:*Number*normalizeToRange* + +# Counter / sequence arithmetic with intentional unsigned wraparound, each +# guarded by an explicit overflow or domain check at the call site: +# base_uint operator++/-- wrap by definition; +# ApplyView::insertPage ++page is asserted to wrap to 0 (page exhaustion); +# confineOwnerCount documents "overflow is well defined on unsigned"; +# NFTokenMint checks tokenSeq + 1u == 0u; AmendmentTable does (seq - 1) / 256. +unsigned-integer-overflow:*base_uint*operator++* +unsigned-integer-overflow:*base_uint*operator--* +unsigned-integer-overflow:*insertPage* +unsigned-integer-overflow:confineOwnerCount* +unsigned-integer-overflow:*NFTokenMint*doApply* +unsigned-integer-overflow:*AmendmentTableImpl*needValidatedLedger* + +# Sentinel / bounded subtractions that wrap by design (loop counters, reverse +# iteration, "not found" sentinels, balance math bounded by issuance invariants). +unsigned-integer-overflow:*SHAMap*lowerBound* +unsigned-integer-overflow:*permissionToTxType* +unsigned-integer-overflow:b256ToB58Be* +unsigned-integer-overflow:*base64*decode* +unsigned-integer-overflow:*NetworkOPsImp*apply* +unsigned-integer-overflow:*NetworkOPsImp*getBookPage* +unsigned-integer-overflow:*OracleSet*preclaim* +unsigned-integer-overflow:*availableMPTAmount* +unsigned-integer-overflow:RFC1751*getWordFromBlob* +unsigned-integer-overflow:*StrandFlow* +unsigned-integer-overflow:*ValueProxy*operator+* + +# GetAggregatePrice negates an unsigned trim count to step a reverse iterator; +# trimCount is bounded by the price set size. +unsigned-integer-overflow:*doGetAggregatePrice* + +# Test-only intentional overflow/underflow in fixture and unit-test arithmetic. +unsigned-integer-overflow:test/app/Batch_test.cpp +unsigned-integer-overflow:test/app/Invariants_test.cpp +unsigned-integer-overflow:test/app/Loan_test.cpp +unsigned-integer-overflow:test/app/NFToken_test.cpp +unsigned-integer-overflow:test/app/OfferMPT_test.cpp +unsigned-integer-overflow:test/app/Offer_test.cpp +unsigned-integer-overflow:test/app/Path_test.cpp +unsigned-integer-overflow:test/jtx/impl/acctdelete.cpp +unsigned-integer-overflow:test/ledger/SkipList_test.cpp +unsigned-integer-overflow:test/rpc/Subscribe_test.cpp +signed-integer-overflow:test/basics/XRPAmount_test.cpp diff --git a/src/xrpld/rpc/handlers/ledger/Ledger.cpp b/src/xrpld/rpc/handlers/ledger/Ledger.cpp index 5938c8c9c5..69fcf03d9a 100644 --- a/src/xrpld/rpc/handlers/ledger/Ledger.cpp +++ b/src/xrpld/rpc/handlers/ledger/Ledger.cpp @@ -349,13 +349,15 @@ doLedgerGrpc(RPC::GRPCContext& context) auto end = std::chrono::system_clock::now(); auto duration = std::chrono::duration_cast(end - begin).count() * 1.0; + // Guard the per-item rates: an empty ledger has zero objects and/or zero + // transactions, and dividing by zero is undefined for these doubles. + auto const numObjects = response.ledger_objects().objects_size(); + auto const numTxns = response.transactions_list().transactions_size(); + std::string const msPerObj = numObjects > 0 ? std::to_string(duration / numObjects) : "n/a"; + std::string const msPerTxn = numTxns > 0 ? std::to_string(duration / numTxns) : "n/a"; JLOG(context.j.warn()) << __func__ << " - Extract time = " << duration - << " - num objects = " << response.ledger_objects().objects_size() - << " - num txns = " << response.transactions_list().transactions_size() - << " - ms per obj " - << duration / response.ledger_objects().objects_size() - << " - ms per txn " - << duration / response.transactions_list().transactions_size(); + << " - num objects = " << numObjects << " - num txns = " << numTxns + << " - ms per obj " << msPerObj << " - ms per txn " << msPerTxn; return {response, status}; }