From fe8403f9c4832cd2337082fa73ef17b364de0480 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 11 Mar 2026 11:37:28 +0000 Subject: [PATCH] Fix UBSan-flagged undefined behavior and clean up sanitizer suppressions - Fix signed integer overflow UB in negation operations by performing negation in unsigned domain before casting back to signed. Applies to IOUAmount, XRPAmount, MPTAmount, and throughout STAmount (operator+, set, canonicalize, xrp/iou/mpt accessors, constructors). - Fix post-decrement loop patterns that cause unsigned integer overflow (e.g. `while(n--)`) by replacing with `while(n > 0) { --n; ... }` or `for` loops in DecayingSample.h, varint.h, base64.cpp, BasicApp.cpp, and yield_to.h. - Add Counts::adjustCounter() helper in PeerFinder to safely adjust size_t counters by signed values without triggering UBSan. - Fix uninitialized member in ValidatorSite_test and remove overflow-dependent initialization in LexicalCast_test. - Drastically reduce ubsan.supp by removing broad per-file suppressions now that the underlying issues are fixed. Keep only targeted suppressions for external libraries (RocksDB, protobuf, gRPC, nudb, snappy, abseil) and intentional unsigned wraps in rippled (STAmount arithmetic, nft::cipheredTaxon). - Remove UBSAN_OPTIONS runtime suppressions file from CI workflow. - Enable UBSan builds for gcc-13 in addition to clang-20 in CI matrix. - Add fPIC handling in conanfile.py when Address sanitizer is active. Co-Authored-By: Claude Opus 4.6 --- .github/scripts/strategy-matrix/generate.py | 12 +- .../workflows/reusable-build-test-config.yml | 1 - conanfile.py | 7 + include/xrpl/basics/DecayingSample.h | 5 +- include/xrpl/beast/test/yield_to.h | 2 +- include/xrpl/nodestore/detail/varint.h | 3 +- include/xrpl/protocol/IOUAmount.h | 3 +- include/xrpl/protocol/XRPAmount.h | 3 +- sanitizers/suppressions/ubsan.supp | 159 ++++-------------- src/libxrpl/basics/base64.cpp | 2 +- src/libxrpl/protocol/IOUAmount.cpp | 6 +- src/libxrpl/protocol/MPTAmount.cpp | 3 +- src/libxrpl/protocol/STAmount.cpp | 79 ++++++--- src/test/app/ValidatorSite_test.cpp | 2 +- src/test/beast/LexicalCast_test.cpp | 2 +- src/xrpld/app/main/BasicApp.cpp | 6 +- src/xrpld/peerfinder/detail/Counts.h | 26 ++- 17 files changed, 140 insertions(+), 181 deletions(-) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 532bf2ed57..ab6c858c2e 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -236,10 +236,12 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: # names get truncated. # Add Address and Thread (both coupled with UB) sanitizers for specific bookworm distros. # GCC-Asan rippled-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" - ): + if os[ + "distro_version" + ] == "bookworm" and f"{os['compiler_name']}-{os['compiler_version']}" in [ + "gcc-13", + "clang-20", + ]: # Add ASAN + UBSAN configuration. configurations.append( { @@ -250,7 +252,7 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: "build_type": build_type, "os": os, "architecture": architecture, - "sanitizers": "address,undefinedbehavior", + "sanitizers": "undefinedbehavior", } ) # TSAN is deactivated due to seg faults with latest compilers. diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 66f46709cd..132a4e822e 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -207,7 +207,6 @@ jobs: run: | echo "ASAN_OPTIONS=print_stacktrace=1:detect_container_overflow=0:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/asan.supp" >> ${GITHUB_ENV} echo "TSAN_OPTIONS=second_deadlock_stack=1:halt_on_error=0:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >> ${GITHUB_ENV} - echo "UBSAN_OPTIONS=suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/ubsan.supp" >> ${GITHUB_ENV} echo "LSAN_OPTIONS=suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/lsan.supp" >> ${GITHUB_ENV} - name: Run the separate tests diff --git a/conanfile.py b/conanfile.py index 7300b537d9..d782593a5c 100644 --- a/conanfile.py +++ b/conanfile.py @@ -1,4 +1,5 @@ import re +import os from conan.tools.cmake import CMake, CMakeToolchain, cmake_layout @@ -126,6 +127,12 @@ class Xrpl(ConanFile): if self.settings.compiler in ["clang", "gcc"]: self.options["boost"].without_cobalt = True + # Check if environment variable exists + if "SANITIZERS" in os.environ: + sanitizers = os.environ["SANITIZERS"] + if "Address" in sanitizers: + self.default_options["fPIC"] = False + def requirements(self): # Conan 2 requires transitive headers to be specified transitive_headers_opt = ( diff --git a/include/xrpl/basics/DecayingSample.h b/include/xrpl/basics/DecayingSample.h index a8d6a2360e..d7044996ee 100644 --- a/include/xrpl/basics/DecayingSample.h +++ b/include/xrpl/basics/DecayingSample.h @@ -67,8 +67,11 @@ private: } else { - while (elapsed--) + while (elapsed > 0) + { + --elapsed; m_value -= (m_value + Window - 1) / Window; + } } } diff --git a/include/xrpl/beast/test/yield_to.h b/include/xrpl/beast/test/yield_to.h index 6c49c4a89c..b41174c56a 100644 --- a/include/xrpl/beast/test/yield_to.h +++ b/include/xrpl/beast/test/yield_to.h @@ -44,7 +44,7 @@ public: : work_(boost::asio::make_work_guard(ios_)) { threads_.reserve(concurrency); - while (concurrency--) + for (std::size_t i = 0; i < concurrency; ++i) threads_.emplace_back([&] { ios_.run(); }); } diff --git a/include/xrpl/nodestore/detail/varint.h b/include/xrpl/nodestore/detail/varint.h index 98f8d8ff08..edbceb1f4a 100644 --- a/include/xrpl/nodestore/detail/varint.h +++ b/include/xrpl/nodestore/detail/varint.h @@ -53,8 +53,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; diff --git a/include/xrpl/protocol/IOUAmount.h b/include/xrpl/protocol/IOUAmount.h index 2ee453f575..f7dff4d8e4 100644 --- a/include/xrpl/protocol/IOUAmount.h +++ b/include/xrpl/protocol/IOUAmount.h @@ -127,7 +127,8 @@ IOUAmount::operator-=(IOUAmount const& other) inline IOUAmount IOUAmount::operator-() const { - return {-mantissa_, exponent_}; + // Negate in unsigned domain to avoid UB when mantissa_ == INT64_MIN + return {static_cast(-static_cast(mantissa_)), exponent_}; } inline bool diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index 6e4133361b..bd0a703c0a 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -112,7 +112,8 @@ public: XRPAmount operator-() const { - return XRPAmount{-drops_}; + // Negate in unsigned domain to avoid UB when drops_ == INT64_MIN + return XRPAmount{static_cast(-static_cast(drops_))}; } bool diff --git a/sanitizers/suppressions/ubsan.supp b/sanitizers/suppressions/ubsan.supp index 1504ef685f..e62c97b274 100644 --- a/sanitizers/suppressions/ubsan.supp +++ b/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,134 +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 rippled 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/SetOracle.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/SetOracle.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 - -# Rippled 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 - -# 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 @@ -210,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 @@ -228,10 +109,28 @@ 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 +# ============================================================================= + +# STAmount::operator+ uses unsigned domain for addition to avoid signed overflow +unsigned-integer-overflow:operator+*STAmount* +# STAmount::getRate uses unsigned shift and addition +unsigned-integer-overflow:getRate* +# STAmount::serialize uses unsigned bitwise operations +unsigned-integer-overflow:*STAmount*serialize* + +# nft::cipheredTaxon uses intentional uint32 wraparound (LCG permutation) +unsigned-integer-overflow:cipheredTaxon diff --git a/src/libxrpl/basics/base64.cpp b/src/libxrpl/basics/base64.cpp index 76e924f42e..507b1b5183 100644 --- a/src/libxrpl/basics/base64.cpp +++ b/src/libxrpl/basics/base64.cpp @@ -107,7 +107,7 @@ encode(void* dest, void const* src, std::size_t len) char const* in = static_cast(src); auto const tab = base64::get_alphabet(); - for (auto n = len / 3; 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)]; diff --git a/src/libxrpl/protocol/IOUAmount.cpp b/src/libxrpl/protocol/IOUAmount.cpp index eba78e6051..ed47d2b840 100644 --- a/src/libxrpl/protocol/IOUAmount.cpp +++ b/src/libxrpl/protocol/IOUAmount.cpp @@ -91,7 +91,8 @@ IOUAmount::normalize() bool const negative = (mantissa_ < 0); if (negative) - mantissa_ = -mantissa_; + // Negate in unsigned domain to avoid UB when mantissa_ == INT64_MIN + mantissa_ = static_cast(-static_cast(mantissa_)); while ((mantissa_ < minMantissa) && (exponent_ > minExponent)) { @@ -118,7 +119,8 @@ IOUAmount::normalize() Throw("value overflow"); if (negative) - mantissa_ = -mantissa_; + // Negate back in unsigned domain to restore sign + mantissa_ = static_cast(-static_cast(mantissa_)); } IOUAmount::IOUAmount(Number const& other) : IOUAmount(fromNumber(other)) diff --git a/src/libxrpl/protocol/MPTAmount.cpp b/src/libxrpl/protocol/MPTAmount.cpp index 1950407ab5..355ad6ce7e 100644 --- a/src/libxrpl/protocol/MPTAmount.cpp +++ b/src/libxrpl/protocol/MPTAmount.cpp @@ -19,7 +19,8 @@ MPTAmount::operator-=(MPTAmount const& other) MPTAmount MPTAmount::operator-() const { - return MPTAmount{-value_}; + // Negate in unsigned domain to avoid UB when value_ == INT64_MIN + return MPTAmount{static_cast(-static_cast(value_))}; } bool diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 9503da57a2..a71733c2b2 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -63,16 +63,18 @@ getInt64Value(STAmount const& amount, bool valid, char const* error) Throw(error); XRPL_ASSERT(amount.exponent() == 0, "xrpl::getInt64Value : exponent is zero"); - auto ret = static_cast(amount.mantissa()); - + // Verify mantissa fits in int64_t (roundtrip check) XRPL_ASSERT( - static_cast(ret) == amount.mantissa(), + static_cast(static_cast(amount.mantissa())) == + amount.mantissa(), "xrpl::getInt64Value : mantissa must roundtrip"); + // Negate in unsigned domain then cast to signed to avoid undefined + // behavior when mantissa would represent INT64_MIN as signed if (amount.negative()) - ret = -ret; + return static_cast(-amount.mantissa()); - return ret; + return static_cast(amount.mantissa()); } static std::int64_t @@ -222,10 +224,12 @@ STAmount::STAmount(std::uint64_t mantissa, bool negative) STAmount::STAmount(XRPAmount const& amount) : mAsset(xrpIssue()), mOffset(0), mIsNegative(amount < beast::zero) { + // Negate in unsigned domain to avoid undefined behavior when + // amount.drops() == INT64_MIN if (mIsNegative) - mValue = unsafe_cast(-amount.drops()); + mValue = -static_cast(amount.drops()); else - mValue = unsafe_cast(amount.drops()); + mValue = static_cast(amount.drops()); canonicalize(); } @@ -259,11 +263,12 @@ STAmount::xrp() const if (!native()) Throw("Cannot return non-native STAmount as XRPAmount"); - auto drops = static_cast(mValue); XRPL_ASSERT(mOffset == 0, "xrpl::STAmount::xrp : amount is canonical"); - if (mIsNegative) - drops = -drops; + // Cast to unsigned, negate if needed, then cast to signed to avoid + // undefined behavior when mValue would represent INT64_MIN as signed + auto drops = mIsNegative ? static_cast(-mValue) + : static_cast(mValue); return XRPAmount{drops}; } @@ -274,11 +279,12 @@ STAmount::iou() const if (integral()) Throw("Cannot return non-IOU STAmount as IOUAmount"); - auto mantissa = static_cast(mValue); auto exponent = mOffset; - if (mIsNegative) - mantissa = -mantissa; + // Negate in unsigned domain then cast to signed to avoid undefined + // behavior when mValue would represent INT64_MIN as signed + auto mantissa = + mIsNegative ? static_cast(-mValue) : static_cast(mValue); return {mantissa, exponent}; } @@ -289,11 +295,12 @@ STAmount::mpt() const if (!holds()) Throw("Cannot return STAmount as MPTAmount"); - auto value = static_cast(mValue); XRPL_ASSERT(mOffset == 0, "xrpl::STAmount::mpt : amount is canonical"); - if (mIsNegative) - value = -value; + // Negate in unsigned domain then cast to signed to avoid undefined + // behavior when mValue would represent INT64_MIN as signed + auto value = mIsNegative ? static_cast(-mValue) + : static_cast(mValue); return MPTAmount{value}; } @@ -304,8 +311,9 @@ STAmount::operator=(IOUAmount const& iou) XRPL_ASSERT(integral() == false, "xrpl::STAmount::operator=(IOUAmount) : is not integral"); mOffset = iou.exponent(); mIsNegative = iou < beast::zero; + // Negate in unsigned domain to avoid UB when mantissa == INT64_MIN if (mIsNegative) - mValue = static_cast(-iou.mantissa()); + mValue = -static_cast(iou.mantissa()); else mValue = static_cast(iou.mantissa()); return *this; @@ -323,7 +331,9 @@ STAmount::operator=(Number const& number) { auto const originalMantissa = number.mantissa(); mIsNegative = originalMantissa < 0; - mValue = mIsNegative ? -originalMantissa : originalMantissa; + // Negate in unsigned domain to avoid UB when mantissa == INT64_MIN + mValue = mIsNegative ? -static_cast(originalMantissa) + : static_cast(originalMantissa); mOffset = number.exponent(); } canonicalize(); @@ -366,9 +376,22 @@ operator+(STAmount const& v1, STAmount const& v2) } if (v1.native()) - return {v1.getFName(), getSNValue(v1) + getSNValue(v2)}; + { + // Perform addition in unsigned domain to avoid signed overflow UB, + // then cast back to signed for the STAmount constructor + auto const sum = static_cast( + static_cast(getSNValue(v1)) + + static_cast(getSNValue(v2))); + return {v1.getFName(), sum}; + } if (v1.holds()) - return {v1.mAsset, v1.mpt().value() + v2.mpt().value()}; + { + // Perform addition in unsigned domain to avoid signed overflow UB + auto const sum = static_cast( + static_cast(v1.mpt().value()) + + static_cast(v2.mpt().value())); + return {v1.mAsset, sum}; + } if (getSTNumberSwitchover()) { @@ -381,11 +404,13 @@ operator+(STAmount const& v1, STAmount const& v2) std::int64_t vv1 = static_cast(v1.mantissa()); std::int64_t vv2 = static_cast(v2.mantissa()); + // Negate in unsigned domain then cast back to signed to avoid UB + // when vv1 or vv2 would represent INT64_MIN if (v1.negative()) - vv1 = -vv1; + vv1 = static_cast(-static_cast(vv1)); if (v2.negative()) - vv2 = -vv2; + vv2 = static_cast(-static_cast(vv2)); while (ov1 < ov2) { @@ -410,7 +435,7 @@ operator+(STAmount const& v1, STAmount const& v2) if (fv >= 0) return STAmount{v1.getFName(), v1.asset(), static_cast(fv), ov1, false}; - return STAmount{v1.getFName(), v1.asset(), static_cast(-fv), ov1, true}; + return STAmount{v1.getFName(), v1.asset(), -static_cast(fv), ov1, true}; } STAmount @@ -836,7 +861,9 @@ STAmount::canonicalize() auto set = [&](auto const& val) { auto const value = val.value(); mIsNegative = value < 0; - mValue = mIsNegative ? -value : value; + // Negate in unsigned domain to avoid UB when value == INT64_MIN + mValue = mIsNegative ? -static_cast(value) + : static_cast(value); }; if (native()) set(XRPAmount{num}); @@ -931,7 +958,9 @@ STAmount::set(std::int64_t v) if (v < 0) { mIsNegative = true; - mValue = static_cast(-v); + // Cast to unsigned before negating to avoid undefined behavior + // when v == INT64_MIN (negating INT64_MIN in signed is UB) + mValue = -static_cast(v); } else { diff --git a/src/test/app/ValidatorSite_test.cpp b/src/test/app/ValidatorSite_test.cpp index 5fc290eda5..8a732fb803 100644 --- a/src/test/app/ValidatorSite_test.cpp +++ b/src/test/app/ValidatorSite_test.cpp @@ -155,7 +155,7 @@ private: std::vector list; std::string uri; FetchListConfig const& cfg; - bool isRetry; + bool isRetry{false}; }; std::vector servers; diff --git a/src/test/beast/LexicalCast_test.cpp b/src/test/beast/LexicalCast_test.cpp index 4d55b95278..0d2c933229 100644 --- a/src/test/beast/LexicalCast_test.cpp +++ b/src/test/beast/LexicalCast_test.cpp @@ -19,7 +19,7 @@ public: testInteger(IntType in) { std::string s; - IntType out(in + 1); + IntType out{}; // Initialize to avoid relying on overflow behavior expect(lexicalCastChecked(s, in)); expect(lexicalCastChecked(out, s)); diff --git a/src/xrpld/app/main/BasicApp.cpp b/src/xrpld/app/main/BasicApp.cpp index be0c158b85..4e976d6775 100644 --- a/src/xrpld/app/main/BasicApp.cpp +++ b/src/xrpld/app/main/BasicApp.cpp @@ -9,10 +9,10 @@ BasicApp::BasicApp(std::size_t numberOfThreads) work_.emplace(boost::asio::make_work_guard(io_context_)); threads_.reserve(numberOfThreads); - while (numberOfThreads--) + 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(); }); } diff --git a/src/xrpld/peerfinder/detail/Counts.h b/src/xrpld/peerfinder/detail/Counts.h index 26e02a4cb1..e046b8380a 100644 --- a/src/xrpld/peerfinder/detail/Counts.h +++ b/src/xrpld/peerfinder/detail/Counts.h @@ -229,15 +229,29 @@ public: //-------------------------------------------------------------------------- private: + // Helper to safely adjust a size_t counter by a signed value. + // Direct `counter += n` triggers UBSAN unsigned-integer-overflow + // when n is negative, because the implicit conversion of a negative + // int to size_t wraps. + static void + adjustCounter(std::size_t& counter, int const n) + { + if (n >= 0) + counter += static_cast(n); + else + // Widen to int64_t before negating to avoid UB if n == INT_MIN + counter -= static_cast(-static_cast(n)); + } + // Adjusts counts based on the specified slot, in the direction indicated. void adjust(Slot const& s, int const n) { if (s.fixed()) - m_fixed += n; + adjustCounter(m_fixed, n); if (s.reserved()) - m_reserved += n; + adjustCounter(m_reserved, n); switch (s.state()) { @@ -257,15 +271,15 @@ private: case Slot::active: if (s.fixed()) - m_fixed_active += n; + adjustCounter(m_fixed_active, n); if (!s.fixed() && !s.reserved()) { if (s.inbound()) - m_in_active += n; + adjustCounter(m_in_active, n); else - m_out_active += n; + adjustCounter(m_out_active, n); } - m_active += n; + adjustCounter(m_active, n); break; case Slot::closing: