fix: Fix ubsan flagged issues (#6151)

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>

.github/scripts/strategy-matrix/generate.py

@@ -51,20 +51,21 @@ def generate_strategy_matrix(all: bool, config: Config) -> list:
         # Only generate a subset of configurations in PRs.
         if not all:
             # Debian:
-            # - Bookworm using GCC 13: Release on linux/amd64, set the reference
-            #   fee to 500.
-            # - Bookworm using GCC 15: Debug on linux/amd64, enable code
-            #   coverage (which will be done below).
+            # - Bookworm using GCC 13: Debug on linux/amd64, set the reference
+            #   fee to 500 and enable code coverage (which will be done below).
+            # - Bookworm using GCC 15: Debug on linux/amd64, enable Address and
+            #   UB sanitizers (which will be done below).
             # - Bookworm using Clang 16: Debug on linux/amd64, enable voidstar.
             # - Bookworm using Clang 17: Release on linux/amd64, set the
             #   reference fee to 1000.
-            # - Bookworm using Clang 20: Debug on linux/amd64.
+            # - Bookworm using Clang 20: Debug on linux/amd64, enable Address
+            #   and UB sanitizers (which will be done below).
             if os["distro_name"] == "debian":
                 skip = True
                 if os["distro_version"] == "bookworm":
                     if (
                         f"{os['compiler_name']}-{os['compiler_version']}" == "gcc-13"
-                        and build_type == "Release"
+                        and build_type == "Debug"
                         and architecture["platform"] == "linux/amd64"
                     ):
                         cmake_args = f"-DUNIT_TEST_REFERENCE_FEE=500 {cmake_args}"
@@ -193,11 +194,11 @@ def generate_strategy_matrix(all: bool, config: Config) -> list:
         ):
             continue
 
-        # Enable code coverage for Debian Bookworm using GCC 15 in Debug on
-        # linux/amd64
+        # Enable code coverage for Debian Bookworm using GCC 13 in Debug on
+        # linux/amd64.
         if (
             f"{os['distro_name']}-{os['distro_version']}" == "debian-bookworm"
-            and f"{os['compiler_name']}-{os['compiler_version']}" == "gcc-15"
+            and f"{os['compiler_name']}-{os['compiler_version']}" == "gcc-13"
             and build_type == "Debug"
             and architecture["platform"] == "linux/amd64"
         ):
@@ -234,23 +235,39 @@ def generate_strategy_matrix(all: bool, config: Config) -> list:
         # Add the configuration to the list, with the most unique fields first,
         # so that they are easier to identify in the GitHub Actions UI, as long
         # names get truncated.
-        # Add Address and Thread (both coupled with UB) sanitizers for specific bookworm distros.
+        # Add Address and UB sanitizers as separate configurations for specific
+        # bookworm distros. Thread sanitizer is currently disabled (see below).
         # GCC-Asan xrpld-embedded tests are failing because of https://github.com/google/sanitizers/issues/856
-        if (
-            os["distro_version"] == "bookworm"
-            and f"{os['compiler_name']}-{os['compiler_version']}" == "clang-20"
-        ):
-            # Add ASAN + UBSAN configuration.
+        if os[
+            "distro_version"
+        ] == "bookworm" and f"{os['compiler_name']}-{os['compiler_version']}" in [
+            "gcc-15",
+            "clang-20",
+        ]:
+            # Add ASAN configuration.
             configurations.append(
                 {
-                    "config_name": config_name + "-asan-ubsan",
+                    "config_name": config_name + "-asan",
                     "cmake_args": cmake_args,
                     "cmake_target": cmake_target,
                     "build_only": build_only,
                     "build_type": build_type,
                     "os": os,
                     "architecture": architecture,
-                    "sanitizers": "address,undefinedbehavior",
+                    "sanitizers": "address",
+                }
+            )
+            # Add UBSAN configuration.
+            configurations.append(
+                {
+                    "config_name": config_name + "-ubsan",
+                    "cmake_args": cmake_args,
+                    "cmake_target": cmake_target,
+                    "build_only": build_only,
+                    "build_type": build_type,
+                    "os": os,
+                    "architecture": architecture,
+                    "sanitizers": "undefinedbehavior",
                 }
             )
             # TSAN is deactivated due to seg faults with latest compilers.

include/xrpl/basics/DecayingSample.h

@@ -67,8 +67,10 @@ private:
             }
             else
             {
-                while ((elapsed--) != 0u)
+                for (; elapsed > 0; --elapsed)
+                {
                     m_value -= (m_value + Window - 1) / Window;
+                }
             }
         }
 

include/xrpl/beast/test/yield_to.h

@@ -43,8 +43,10 @@ public:
         : work_(boost::asio::make_work_guard(ios_))
     {
         threads_.reserve(concurrency);
-        while ((concurrency--) != 0u)
+        for (std::size_t i = 0; i < concurrency; ++i)
+        {
             threads_.emplace_back([&] { ios_.run(); });
+        }
     }
 
     ~enable_yield_to()

include/xrpl/nodestore/detail/varint.h

@@ -54,8 +54,9 @@ read_varint(void const* buf, std::size_t buflen, std::size_t& t)
         return 1;
     }
     auto const used = n;
-    while (n--)
+    while (n > 0)
     {
+        --n;
         auto const d = p[n];
         auto const t0 = t;
         t *= 127;

sanitizers/suppressions/ubsan.supp

@@ -11,7 +11,6 @@ float-cast-overflow:external
 float-divide-by-zero:external
 function:external
 implicit-integer-sign-change:external
-implicit-signed-integer-truncation::external
 implicit-signed-integer-truncation:external
 implicit-unsigned-integer-truncation:external
 integer-divide-by-zero:external
@@ -71,145 +70,15 @@ vla-bound:boost
 vptr_check:boost
 vptr:boost
 
-# Google protobuf
+# Google protobuf - intentional overflows in hash functions
 undefined:protobuf
-
-# Suppress UBSan errors in xrpld code by source file path
-undefined:src/libxrpl/basics/base64.cpp
-undefined:src/libxrpl/basics/Number.cpp
-undefined:src/libxrpl/beast/utility/beast_Journal.cpp
-undefined:src/libxrpl/crypto/RFC1751.cpp
-undefined:src/libxrpl/ledger/ApplyView.cpp
-undefined:src/libxrpl/ledger/View.cpp
-undefined:src/libxrpl/protocol/Permissions.cpp
-undefined:src/libxrpl/protocol/STAmount.cpp
-undefined:src/libxrpl/protocol/STPathSet.cpp
-undefined:src/libxrpl/protocol/tokens.cpp
-undefined:src/libxrpl/shamap/SHAMap.cpp
-undefined:src/test/app/Batch_test.cpp
-undefined:src/test/app/Invariants_test.cpp
-undefined:src/test/app/NFToken_test.cpp
-undefined:src/test/app/Offer_test.cpp
-undefined:src/test/app/Path_test.cpp
-undefined:src/test/basics/XRPAmount_test.cpp
-undefined:src/test/beast/LexicalCast_test.cpp
-undefined:src/test/jtx/impl/acctdelete.cpp
-undefined:src/test/ledger/SkipList_test.cpp
-undefined:src/test/rpc/Subscribe_test.cpp
-undefined:src/tests/libxrpl/basics/RangeSet.cpp
-undefined:src/xrpld/app/main/BasicApp.cpp
-undefined:src/xrpld/app/main/BasicApp.cpp
-undefined:src/xrpld/app/misc/detail/AmendmentTable.cpp
-undefined:src/xrpld/app/misc/NetworkOPs.cpp
-undefined:src/libxrpl/json/json_value.cpp
-undefined:src/xrpld/app/paths/detail/StrandFlow.h
-undefined:src/xrpld/app/tx/detail/NFTokenMint.cpp
-undefined:src/xrpld/app/tx/detail/OracleSet.cpp
-undefined:src/xrpld/core/detail/JobQueue.cpp
-undefined:src/xrpld/core/detail/Workers.cpp
-undefined:src/xrpld/rpc/detail/Role.cpp
-undefined:src/xrpld/rpc/handlers/GetAggregatePrice.cpp
-undefined:xrpl/basics/base_uint.h
-undefined:xrpl/basics/DecayingSample.h
-undefined:xrpl/beast/test/yield_to.h
-undefined:xrpl/beast/xor_shift_engine.h
-undefined:xrpl/nodestore/detail/varint.h
-undefined:xrpl/peerfinder/detail/Counts.h
-undefined:xrpl/protocol/nft.h
-
-# basic_string.h:483:51: runtime error: unsigned integer overflow
-unsigned-integer-overflow:basic_string.h
-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/uniform_int_dist.h
-unsigned-integer-overflow:string_view
-
-# runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'std::size_t' (aka 'unsigned long')
-unsigned-integer-overflow:src/libxrpl/basics/base64.cpp
-unsigned-integer-overflow:src/libxrpl/basics/Number.cpp
-unsigned-integer-overflow:src/libxrpl/crypto/RFC1751.cpp
-unsigned-integer-overflow:rc/libxrpl/json/json_value.cpp
-unsigned-integer-overflow:src/libxrpl/ledger/ApplyView.cpp
-unsigned-integer-overflow:src/libxrpl/ledger/View.cpp
-unsigned-integer-overflow:src/libxrpl/protocol/Permissions.cpp
-unsigned-integer-overflow:src/libxrpl/protocol/STAmount.cpp
-unsigned-integer-overflow:src/libxrpl/protocol/STPathSet.cpp
-unsigned-integer-overflow:src/libxrpl/protocol/tokens.cpp
-unsigned-integer-overflow:src/libxrpl/shamap/SHAMap.cpp
-unsigned-integer-overflow:src/test/app/Batch_test.cpp
-unsigned-integer-overflow:src/test/app/Invariants_test.cpp
-unsigned-integer-overflow:src/test/app/NFToken_test.cpp
-unsigned-integer-overflow:src/test/app/Offer_test.cpp
-unsigned-integer-overflow:src/test/app/Path_test.cpp
-unsigned-integer-overflow:src/test/basics/XRPAmount_test.cpp
-unsigned-integer-overflow:src/test/beast/LexicalCast_test.cpp
-unsigned-integer-overflow:src/test/jtx/impl/acctdelete.cpp
-unsigned-integer-overflow:src/test/ledger/SkipList_test.cpp
-unsigned-integer-overflow:src/test/rpc/Subscribe_test.cpp
-unsigned-integer-overflow:src/tests/libxrpl/basics/RangeSet.cpp
-unsigned-integer-overflow:src/xrpld/app/main/BasicApp.cpp
-unsigned-integer-overflow:src/xrpld/app/misc/detail/AmendmentTable.cpp
-unsigned-integer-overflow:src/xrpld/app/misc/NetworkOPs.cpp
-unsigned-integer-overflow:src/xrpld/app/paths/detail/StrandFlow.h
-unsigned-integer-overflow:src/xrpld/app/tx/detail/NFTokenMint.cpp
-unsigned-integer-overflow:src/xrpld/app/tx/detail/OracleSet.cpp
-unsigned-integer-overflow:src/xrpld/rpc/detail/Role.cpp
-unsigned-integer-overflow:src/xrpld/rpc/handlers/GetAggregatePrice.cpp
-unsigned-integer-overflow:xrpl/basics/base_uint.h
-unsigned-integer-overflow:xrpl/basics/DecayingSample.h
-unsigned-integer-overflow:xrpl/beast/test/yield_to.h
-unsigned-integer-overflow:xrpl/beast/xor_shift_engine.h
-unsigned-integer-overflow:xrpl/nodestore/detail/varint.h
-unsigned-integer-overflow:xrpl/peerfinder/detail/Counts.h
-unsigned-integer-overflow:xrpl/protocol/nft.h
-
-# Xrpld intentional overflows and operations
-# STAmount uses intentional negation of INT64_MIN and overflow in arithmetic
-signed-integer-overflow:src/libxrpl/protocol/STAmount.cpp
-unsigned-integer-overflow:src/libxrpl/protocol/STAmount.cpp
-
-# XRPAmount test intentional overflows
-signed-integer-overflow:src/test/basics/XRPAmount_test.cpp
-
-# Peerfinder intentional overflow in counter arithmetic
-unsigned-integer-overflow:src/xrpld/peerfinder/detail/Counts.h
-
-# Signed integer overflow suppressions
-signed-integer-overflow:src/test/beast/LexicalCast_test.cpp
-
-# External library suppressions
-unsigned-integer-overflow:nudb/detail/xxhash.hpp
-
-# Loan_test.cpp intentional underflow in test arithmetic
-unsigned-integer-overflow:src/test/app/Loan_test.cpp
-undefined:src/test/app/Loan_test.cpp
-
-# Source tree restructured paths (libxrpl/tx/transactors/)
-# These duplicate the xrpld/app/tx/detail entries above for the new layout
-unsigned-integer-overflow:src/libxrpl/tx/transactors/oracle/OracleSet.cpp
-undefined:src/libxrpl/tx/transactors/oracle/OracleSet.cpp
-unsigned-integer-overflow:src/libxrpl/tx/transactors/nft/NFTokenMint.cpp
-undefined:src/libxrpl/tx/transactors/nft/NFTokenMint.cpp
-
-# Protobuf intentional overflows in hash functions
-# Protobuf uses intentional unsigned overflow for hash computation (stringpiece.h:393)
 unsigned-integer-overflow:google/protobuf/stubs/stringpiece.h
 
-# gRPC intentional overflows
-# gRPC uses intentional overflow in timer calculations
+# gRPC intentional overflows in timer calculations
 unsigned-integer-overflow:grpc
 unsigned-integer-overflow:timer_manager.cc
 
-# Standard library intentional overflows
-# These are intentional overflows in random number generation and character conversion
-unsigned-integer-overflow:__random/seed_seq.h
-unsigned-integer-overflow:__charconv/traits.h
-
-
-# Suppress errors in RocksDB
-# RocksDB uses intentional unsigned integer overflows in hash functions and CRC calculations
+# RocksDB intentional unsigned integer overflows in hash functions and CRC calculations
 unsigned-integer-overflow:rocks*/*/util/xxhash.h
 unsigned-integer-overflow:rocks*/*/util/xxph3.h
 unsigned-integer-overflow:rocks*/*/util/hash.cc
@@ -221,13 +90,14 @@ unsigned-integer-overflow:rocks*/*/table/format.cc
 unsigned-integer-overflow:rocks*/*/table/block_based/block_based_table_builder.cc
 unsigned-integer-overflow:rocks*/*/table/block_based/reader_common.cc
 unsigned-integer-overflow:rocks*/*/db/version_set.cc
-
-# RocksDB misaligned loads (intentional for performance on ARM64)
 alignment:rocks*/*/util/crc32c_arm64.cc
+undefined:rocks*/*/util/crc32c_arm64.cc
+undefined:rocks*/*/util/xxhash.h
 
 # nudb intentional overflows in hash functions
 unsigned-integer-overflow:nudb/detail/xxhash.hpp
 alignment:nudb/detail/xxhash.hpp
+undefined:nudb
 
 # Snappy compression library intentional overflows
 unsigned-integer-overflow:snappy.cc
@@ -239,10 +109,40 @@ 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
 
-# Standard library intentional overflows in chrono duration arithmetic
+# Standard library intentional overflows
+unsigned-integer-overflow:basic_string.h
+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/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
 
-# Suppress undefined errors in RocksDB and nudb
-undefined:rocks.*/*/util/crc32c_arm64.cc
-undefined:rocks.*/*/util/xxhash.h
-undefined:nudb
+# =============================================================================
+# Rippled code suppressions
+# =============================================================================
+
+# Signed integer negation (-value) in amount types.
+# INT64_MIN cannot occur in practice due to domain invariants (mantissa ranges
+# are well within int64_t bounds), but UBSan flags the pattern as potential
+# signed overflow. Narrowed to operator- to avoid suppressing unrelated
+# overflows anywhere in a stack trace containing these type names.
+signed-integer-overflow:operator-*IOUAmount*
+signed-integer-overflow:operator-*XRPAmount*
+signed-integer-overflow:operator-*MPTAmount*
+signed-integer-overflow:operator-*STAmount*
+
+# STAmount::operator+ signed addition — operands are bounded by total supply
+# (~10^17 for XRP, ~10^18 for MPT) so overflow cannot occur in practice.
+signed-integer-overflow:operator+*STAmount*
+
+# STAmount::getRate uses unsigned shift and addition
+unsigned-integer-overflow:*STAmount*getRate*
+# STAmount::serialize uses unsigned bitwise operations
+unsigned-integer-overflow:*STAmount*serialize*
+
+# nft::cipheredTaxon uses intentional uint32 wraparound (LCG permutation)
+unsigned-integer-overflow:cipheredTaxon

src/libxrpl/basics/base64.cpp

@@ -107,7 +107,7 @@ encode(void* dest, void const* src, std::size_t len)
     char const* in = static_cast<char const*>(src);
     auto const tab = base64::get_alphabet();
 
-    for (auto n = len / 3; n != 0u; --n)
+    for (auto n = len / 3; n > 0; --n)
     {
         *out++ = tab[(in[0] & 0xfc) >> 2];
         *out++ = tab[((in[0] & 0x03) << 4) + ((in[1] & 0xf0) >> 4)];

src/test/beast/LexicalCast_test.cpp

@@ -24,7 +24,7 @@ public:
     testInteger(IntType in)
     {
         std::string s;
-        IntType out(in + 1);
+        IntType out = static_cast<IntType>(~in);  // Ensure out != in
 
         expect(lexicalCastChecked(s, in));
         expect(lexicalCastChecked(out, s));

src/xrpld/app/main/BasicApp.cpp

@@ -12,10 +12,10 @@ BasicApp::BasicApp(std::size_t numberOfThreads)
     work_.emplace(boost::asio::make_work_guard(io_context_));
     threads_.reserve(numberOfThreads);
 
-    while ((numberOfThreads--) != 0u)
+    for (std::size_t i = 0; i < numberOfThreads; ++i)
     {
-        threads_.emplace_back([this, numberOfThreads]() {
-            beast::setCurrentThreadName("io svc #" + std::to_string(numberOfThreads));
+        threads_.emplace_back([this, i]() {
+            beast::setCurrentThreadName("io svc #" + std::to_string(i));
             this->io_context_.run();
         });
     }

src/xrpld/peerfinder/detail/Counts.h

@@ -8,6 +8,9 @@
 
 namespace xrpl::PeerFinder {
 
+/** Direction of a slot count adjustment. */
+enum class CountAdjustment : int { Decrement = -1, Increment = 1 };
+
 /** Manages the count of available connections for the various slots. */
 class Counts
 {
@@ -16,14 +19,14 @@ public:
     void
     add(Slot const& s)
     {
-        adjust(s, 1);
+        adjust(s, CountAdjustment::Increment);
     }
 
     /** Removes the slot state and properties from the slot counts. */
     void
     remove(Slot const& s)
     {
-        adjust(s, -1);
+        adjust(s, CountAdjustment::Decrement);
     }
 
     /** Returns `true` if the slot can become active. */
@@ -207,21 +210,40 @@ public:
 
     //--------------------------------------------------------------------------
 private:
+    /** Increments or decrements a counter based on the adjustment direction. */
+    template <typename T>
+    static void
+    adjustCounter(T& counter, CountAdjustment dir)
+    {
+        switch (dir)
+        {
+            case CountAdjustment::Increment:
+                ++counter;
+                break;
+            case CountAdjustment::Decrement:
+                --counter;
+                break;
+        }
+    }
+
     // Adjusts counts based on the specified slot, in the direction indicated.
+    // Using ++/-- instead of += on std::size_t counters avoids UBSan
+    // unsigned-integer-overflow from implicit conversion of -1 to SIZE_MAX.
+    // A decrement on a zero counter is a real bug that UBSan should catch.
     void
-    adjust(Slot const& s, int const n)
+    adjust(Slot const& s, CountAdjustment const dir)
     {
         if (s.fixed())
-            m_fixed += n;
+            adjustCounter(m_fixed, dir);
 
         if (s.reserved())
-            m_reserved += n;
+            adjustCounter(m_reserved, dir);
 
         switch (s.state())
         {
             case Slot::accept:
                 XRPL_ASSERT(s.inbound(), "xrpl::PeerFinder::Counts::adjust : input is inbound");
-                m_acceptCount += n;
+                adjustCounter(m_acceptCount, dir);
                 break;
 
             case Slot::connect:
@@ -230,28 +252,28 @@ private:
                     !s.inbound(),
                     "xrpl::PeerFinder::Counts::adjust : input is not "
                     "inbound");
-                m_attempts += n;
+                adjustCounter(m_attempts, dir);
                 break;
 
             case Slot::active:
                 if (s.fixed())
-                    m_fixed_active += n;
+                    adjustCounter(m_fixed_active, dir);
                 if (!s.fixed() && !s.reserved())
                 {
                     if (s.inbound())
                     {
-                        m_in_active += n;
+                        adjustCounter(m_in_active, dir);
                     }
                     else
                     {
-                        m_out_active += n;
+                        adjustCounter(m_out_active, dir);
                     }
                 }
-                m_active += n;
+                adjustCounter(m_active, dir);
                 break;
 
             case Slot::closing:
-                m_closingCount += n;
+                adjustCounter(m_closingCount, dir);
                 break;
 
             // LCOV_EXCL_START