From c47b4f366752f26cb73b7cf5bd87847c3b5146c1 Mon Sep 17 00:00:00 2001 From: seelabs Date: Tue, 1 Dec 2020 13:58:36 -0500 Subject: [PATCH] Improve canonicalization of serialized amounts: The existing code that deserialized an STAmount was sub-optimal and performed poorly. In some rare cases the operation could result in otherwise valid serialized amounts overflowing during deserialization. This commit will help detect error conditions more quickly and eliminate the problematic corner cases. --- src/ripple/app/misc/impl/TxQ.cpp | 4 ++++ src/ripple/app/tx/impl/Transactor.cpp | 2 ++ src/ripple/app/tx/impl/apply.cpp | 2 ++ src/ripple/protocol/Feature.h | 5 ++++- src/ripple/protocol/STAmount.h | 27 +++++++++++++++++++++++++++ src/ripple/protocol/impl/Feature.cpp | 4 +++- src/ripple/protocol/impl/STAmount.cpp | 22 +++++++++++++++++++++- 7 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 07668c1a6..2d6768552 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -290,6 +290,8 @@ TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j) { // If the rules or flags change, preflight again assert(pfresult); + STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)}; + if (pfresult->rules != view.rules() || pfresult->flags != flags) { JLOG(j.debug()) << "Queued transaction " << txID @@ -728,6 +730,8 @@ TxQ::apply( ApplyFlags flags, beast::Journal j) { + STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)}; + // See if the transaction paid a high enough fee that it can go straight // into the ledger. if (auto directApplied = tryDirectApply(app, view, tx, flags, j)) diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 5c777afd1..bc7a98308 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -746,6 +746,8 @@ Transactor::operator()() { JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID(); + STAmountSO stAmountSO{view().rules().enabled(fixSTAmountCanonicalize)}; + #ifdef DEBUG { Serializer ser; diff --git a/src/ripple/app/tx/impl/apply.cpp b/src/ripple/app/tx/impl/apply.cpp index e42333275..7ccba7826 100644 --- a/src/ripple/app/tx/impl/apply.cpp +++ b/src/ripple/app/tx/impl/apply.cpp @@ -113,6 +113,8 @@ apply( ApplyFlags flags, beast::Journal j) { + STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)}; + auto pfresult = preflight(app, view.rules(), tx, flags, j); auto pcresult = preclaim(pfresult, app, view); return doApply(pcresult, app, view); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index cb7a1c06a..05506614f 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -114,7 +114,9 @@ class FeatureCollections "fixAmendmentMajorityCalc", // Fix Amendment majority calculation "NegativeUNL", "TicketBatch", - "FlowSortStrands"}; + "FlowSortStrands", + "fixSTAmountCanonicalize", + }; std::vector features; boost::container::flat_map featureToIndex; @@ -373,6 +375,7 @@ extern uint256 const fixAmendmentMajorityCalc; extern uint256 const featureNegativeUNL; extern uint256 const featureTicketBatch; extern uint256 const featureFlowSortStrands; +extern uint256 const fixSTAmountCanonicalize; } // namespace ripple diff --git a/src/ripple/protocol/STAmount.h b/src/ripple/protocol/STAmount.h index 1f70f5f02..01bd4934b 100644 --- a/src/ripple/protocol/STAmount.h +++ b/src/ripple/protocol/STAmount.h @@ -21,6 +21,7 @@ #define RIPPLE_PROTOCOL_STAMOUNT_H_INCLUDED #include +#include #include #include #include @@ -471,6 +472,32 @@ isXRP(STAmount const& amount) return isXRP(amount.issue().currency); } +// Since `canonicalize` does not have access to a ledger, this is needed to put +// the low-level routine stAmountCanonicalize on an amendment switch. Only +// transactions need to use this switchover. Outside of a transaction it's safe +// to unconditionally use the new behavior. +extern LocalValue stAmountCanonicalizeSwitchover; + +/** RAII class to set and restore the STAmount canonicalize switchover. + */ + +class STAmountSO +{ +public: + explicit STAmountSO(bool v) : saved_(*stAmountCanonicalizeSwitchover) + { + *stAmountCanonicalizeSwitchover = v; + } + + ~STAmountSO() + { + *stAmountCanonicalizeSwitchover = saved_; + } + +private: + bool saved_; +}; + } // namespace ripple #endif diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 17386f7a5..eaf1cbf71 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -134,6 +134,7 @@ detail::supportedAmendments() //"NegativeUNL", // Commented out to prevent automatic enablement "TicketBatch", "FlowSortStrands", + "fixSTAmountCanonicalize", }; return supported; } @@ -188,7 +189,8 @@ uint256 const fixAmendmentMajorityCalc = *getRegisteredFeature("fixAmendmentMajorityCalc"), featureNegativeUNL = *getRegisteredFeature("NegativeUNL"), featureTicketBatch = *getRegisteredFeature("TicketBatch"), - featureFlowSortStrands = *getRegisteredFeature("FlowSortStrands"); + featureFlowSortStrands = *getRegisteredFeature("FlowSortStrands"), + fixSTAmountCanonicalize = *getRegisteredFeature("fixSTAmountCanonicalize"); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/ripple/protocol/impl/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp index 37a010c3f..40132e3b4 100644 --- a/src/ripple/protocol/impl/STAmount.cpp +++ b/src/ripple/protocol/impl/STAmount.cpp @@ -34,6 +34,8 @@ namespace ripple { +LocalValue stAmountCanonicalizeSwitchover(true); + static const std::uint64_t tenTo14 = 100000000000000ull; static const std::uint64_t tenTo14m1 = tenTo14 - 1; static const std::uint64_t tenTo17 = tenTo14 * 1000; @@ -662,13 +664,23 @@ STAmount::canonicalize() // native currency amounts should always have an offset of zero mIsNative = true; - if (mValue == 0) + // log(2^64,10) ~ 19.2 + if (mValue == 0 || mOffset <= -20) { + mValue = 0; mOffset = 0; mIsNegative = false; return; } + if (*stAmountCanonicalizeSwitchover) + { + // log(cMaxNativeN, 10) == 17 + if (mOffset > 17) + Throw( + "Native currency amount out of range"); + } + while (mOffset < 0) { mValue /= 10; @@ -677,6 +689,14 @@ STAmount::canonicalize() while (mOffset > 0) { + if (*stAmountCanonicalizeSwitchover) + { + // N.B. do not move the overflow check to after the + // multiplication + if (mValue > cMaxNativeN) + Throw( + "Native currency amount out of range"); + } mValue *= 10; --mOffset; }