diff --git a/include/xrpl/protocol/IOUAmount.h b/include/xrpl/protocol/IOUAmount.h index f7dff4d8e4..2ee453f575 100644 --- a/include/xrpl/protocol/IOUAmount.h +++ b/include/xrpl/protocol/IOUAmount.h @@ -127,8 +127,7 @@ IOUAmount::operator-=(IOUAmount const& other) inline IOUAmount IOUAmount::operator-() const { - // Negate in unsigned domain to avoid UB when mantissa_ == INT64_MIN - return {static_cast(-static_cast(mantissa_)), exponent_}; + return {-mantissa_, exponent_}; } inline bool diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index bd0a703c0a..6e4133361b 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -112,8 +112,7 @@ public: XRPAmount operator-() const { - // Negate in unsigned domain to avoid UB when drops_ == INT64_MIN - return XRPAmount{static_cast(-static_cast(drops_))}; + return XRPAmount{-drops_}; } bool diff --git a/sanitizers/suppressions/ubsan.supp b/sanitizers/suppressions/ubsan.supp index e62c97b274..852138f0d7 100644 --- a/sanitizers/suppressions/ubsan.supp +++ b/sanitizers/suppressions/ubsan.supp @@ -125,8 +125,19 @@ unsigned-integer-overflow:__chrono/duration.h # Rippled code suppressions # ============================================================================= -# STAmount::operator+ uses unsigned domain for addition to avoid signed overflow -unsigned-integer-overflow:operator+*STAmount* +# 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. +signed-integer-overflow:IOUAmount +signed-integer-overflow:XRPAmount +signed-integer-overflow:MPTAmount +signed-integer-overflow: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:getRate* # STAmount::serialize uses unsigned bitwise operations diff --git a/src/libxrpl/protocol/IOUAmount.cpp b/src/libxrpl/protocol/IOUAmount.cpp index ed47d2b840..eba78e6051 100644 --- a/src/libxrpl/protocol/IOUAmount.cpp +++ b/src/libxrpl/protocol/IOUAmount.cpp @@ -91,8 +91,7 @@ IOUAmount::normalize() bool const negative = (mantissa_ < 0); if (negative) - // Negate in unsigned domain to avoid UB when mantissa_ == INT64_MIN - mantissa_ = static_cast(-static_cast(mantissa_)); + mantissa_ = -mantissa_; while ((mantissa_ < minMantissa) && (exponent_ > minExponent)) { @@ -119,8 +118,7 @@ IOUAmount::normalize() Throw("value overflow"); if (negative) - // Negate back in unsigned domain to restore sign - mantissa_ = static_cast(-static_cast(mantissa_)); + mantissa_ = -mantissa_; } IOUAmount::IOUAmount(Number const& other) : IOUAmount(fromNumber(other)) diff --git a/src/libxrpl/protocol/MPTAmount.cpp b/src/libxrpl/protocol/MPTAmount.cpp index 355ad6ce7e..1950407ab5 100644 --- a/src/libxrpl/protocol/MPTAmount.cpp +++ b/src/libxrpl/protocol/MPTAmount.cpp @@ -19,8 +19,7 @@ MPTAmount::operator-=(MPTAmount const& other) MPTAmount MPTAmount::operator-() const { - // Negate in unsigned domain to avoid UB when value_ == INT64_MIN - return MPTAmount{static_cast(-static_cast(value_))}; + return MPTAmount{-value_}; } bool diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 0c84ed1f90..92ce129825 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -63,18 +63,16 @@ getInt64Value(STAmount const& amount, bool valid, char const* error) Throw(error); XRPL_ASSERT(amount.exponent() == 0, "xrpl::getInt64Value : exponent is zero"); - // Verify mantissa fits in int64_t (roundtrip check) + auto ret = static_cast(amount.mantissa()); + XRPL_ASSERT( - static_cast(static_cast(amount.mantissa())) == - amount.mantissa(), + static_cast(ret) == 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()) - return static_cast(-amount.mantissa()); + ret = -ret; - return static_cast(amount.mantissa()); + return ret; } static std::int64_t @@ -224,12 +222,14 @@ 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 = -static_cast(amount.drops()); + { + mValue = unsafe_cast(-amount.drops()); + } else - mValue = static_cast(amount.drops()); + { + mValue = unsafe_cast(amount.drops()); + } canonicalize(); } @@ -263,12 +263,11 @@ 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"); - // 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); + if (mIsNegative) + drops = -drops; return XRPAmount{drops}; } @@ -279,12 +278,11 @@ STAmount::iou() const if (integral()) Throw("Cannot return non-IOU STAmount as IOUAmount"); + auto mantissa = static_cast(mValue); auto exponent = mOffset; - // 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); + if (mIsNegative) + mantissa = -mantissa; return {mantissa, exponent}; } @@ -295,12 +293,11 @@ 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"); - // 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); + if (mIsNegative) + value = -value; return MPTAmount{value}; } @@ -311,9 +308,10 @@ 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()); + } else { mValue = static_cast(iou.mantissa()); @@ -333,9 +331,7 @@ STAmount::operator=(Number const& number) { auto const originalMantissa = number.mantissa(); mIsNegative = originalMantissa < 0; - // Negate in unsigned domain to avoid UB when mantissa == INT64_MIN - mValue = mIsNegative ? -static_cast(originalMantissa) - : static_cast(originalMantissa); + mValue = mIsNegative ? -originalMantissa : originalMantissa; mOffset = number.exponent(); } canonicalize(); @@ -378,22 +374,9 @@ operator+(STAmount const& v1, STAmount const& v2) } if (v1.native()) - { - // 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}; - } + return {v1.getFName(), getSNValue(v1) + getSNValue(v2)}; if (v1.holds()) - { - // 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}; - } + return {v1.mAsset, v1.mpt().value() + v2.mpt().value()}; if (getSTNumberSwitchover()) { @@ -406,13 +389,11 @@ 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 = static_cast(-static_cast(vv1)); + vv1 = -vv1; if (v2.negative()) - vv2 = static_cast(-static_cast(vv2)); + vv2 = -vv2; while (ov1 < ov2) { @@ -437,7 +418,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 @@ -877,9 +858,7 @@ STAmount::canonicalize() auto set = [&](auto const& val) { auto const value = val.value(); mIsNegative = value < 0; - // Negate in unsigned domain to avoid UB when value == INT64_MIN - mValue = mIsNegative ? -static_cast(value) - : static_cast(value); + mValue = mIsNegative ? -value : value; }; if (native()) { @@ -988,9 +967,7 @@ STAmount::set(std::int64_t v) if (v < 0) { mIsNegative = true; - // Cast to unsigned before negating to avoid undefined behavior - // when v == INT64_MIN (negating INT64_MIN in signed is UB) - mValue = -static_cast(v); + mValue = static_cast(-v); } else { diff --git a/src/test/app/ValidatorSite_test.cpp b/src/test/app/ValidatorSite_test.cpp index e69d146568..c47c93c426 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{false}; + bool isRetry{}; }; std::vector servers;