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: