From 773dcd1d488bf70c205196176a216e0e49ca636e Mon Sep 17 00:00:00 2001 From: Howard Hinnant Date: Thu, 23 May 2019 19:29:25 -0400 Subject: [PATCH] Modernize base_uint: * Add construction and assignment from a generic contiguous container. Both compile-time and run time safety checks are made to ensure the safety of this conversion. * Remove base_uint::copyFrom. The generic copy assignment operator now does this functionality with enhanced safety and better syntax. * Remove construction from and dedendence on Blob. The generic constructor and assignment now handle this functionality. * Fix client code to adhere to this new API. * Removed the use of fromVoid in PeerImp.cpp as it was an inappropriate use of this dangerous API. The generic container constructors do it with enhanced safety and better syntax. * Rename data member pn to data_ and make it private. * Remove constraint from hash_append * Remove array_type alias --- src/ripple/app/ledger/OrderBookDB.cpp | 12 +- src/ripple/app/paths/NodeDirectory.h | 6 +- src/ripple/app/paths/cursor/AdvanceNode.cpp | 4 +- src/ripple/basics/base_uint.h | 111 ++++++++++++------- src/ripple/crypto/GenerateDeterministicKey.h | 1 + src/ripple/nodestore/NodeObject.h | 1 + src/ripple/overlay/impl/PeerImp.cpp | 4 +- src/ripple/protocol/STAccount.h | 8 +- src/ripple/protocol/STBitString.h | 2 +- src/ripple/protocol/Serializer.h | 1 + src/ripple/protocol/impl/AccountID.cpp | 2 +- src/ripple/protocol/impl/STAccount.cpp | 2 +- src/ripple/protocol/impl/STAmount.cpp | 4 +- src/ripple/protocol/impl/STPathSet.cpp | 6 +- src/ripple/protocol/impl/UintTypes.cpp | 2 +- src/test/basics/base_uint_test.cpp | 4 + src/test/protocol/SecretKey_test.cpp | 2 +- src/test/protocol/digest_test.cpp | 7 +- 18 files changed, 105 insertions(+), 74 deletions(-) diff --git a/src/ripple/app/ledger/OrderBookDB.cpp b/src/ripple/app/ledger/OrderBookDB.cpp index 77bbd9349..11d8bacaa 100644 --- a/src/ripple/app/ledger/OrderBookDB.cpp +++ b/src/ripple/app/ledger/OrderBookDB.cpp @@ -114,14 +114,10 @@ void OrderBookDB::update( sle->getFieldH256 (sfRootIndex) == sle->key()) { Book book; - book.in.currency.copyFrom(sle->getFieldH160( - sfTakerPaysCurrency)); - book.in.account.copyFrom(sle->getFieldH160 ( - sfTakerPaysIssuer)); - book.out.account.copyFrom(sle->getFieldH160( - sfTakerGetsIssuer)); - book.out.currency.copyFrom (sle->getFieldH160( - sfTakerGetsCurrency)); + book.in.currency = sle->getFieldH160(sfTakerPaysCurrency); + book.in.account = sle->getFieldH160(sfTakerPaysIssuer); + book.out.account = sle->getFieldH160(sfTakerGetsIssuer); + book.out.currency = sle->getFieldH160(sfTakerGetsCurrency); uint256 index = getBookBase (book); if (seen.insert (index).second) diff --git a/src/ripple/app/paths/NodeDirectory.h b/src/ripple/app/paths/NodeDirectory.h index fc3b69c54..cba69741b 100644 --- a/src/ripple/app/paths/NodeDirectory.h +++ b/src/ripple/app/paths/NodeDirectory.h @@ -51,7 +51,7 @@ public: void restart (bool multiQuality) { if (multiQuality) - current = 0; // Restart book searching. + current = beast::zero; // Restart book searching. else restartNeeded = true; // Restart at same quality. } @@ -61,8 +61,8 @@ public: if (current != beast::zero) return false; - current.copyFrom (getBookBase (book)); - next.copyFrom (getQualityNext (current)); + current = getBookBase(book); + next = getQualityNext(current); // TODO(tom): it seems impossible that any actual offers with // quality == 0 could occur - we should disallow them, and clear diff --git a/src/ripple/app/paths/cursor/AdvanceNode.cpp b/src/ripple/app/paths/cursor/AdvanceNode.cpp index 9cd05a814..7d0133a5f 100644 --- a/src/ripple/app/paths/cursor/AdvanceNode.cpp +++ b/src/ripple/app/paths/cursor/AdvanceNode.cpp @@ -89,7 +89,7 @@ TER PathCursor::advanceNode (bool const bReverse) const JLOG (j_.trace()) << "advanceNode: No more offers."; - node().offerIndex_ = 0; + node().offerIndex_ = beast::zero; break; } else @@ -177,7 +177,7 @@ TER PathCursor::advanceNode (bool const bReverse) const { // Ran off end of offers. node().bEntryAdvance = false; // Done. - node().offerIndex_ = 0; // Report no more entries. + node().offerIndex_ = beast::zero; // Report no more entries. } } else diff --git a/src/ripple/basics/base_uint.h b/src/ripple/basics/base_uint.h index c3bad19eb..d45a28d1b 100644 --- a/src/ripple/basics/base_uint.h +++ b/src/ripple/basics/base_uint.h @@ -25,9 +25,9 @@ #ifndef RIPPLE_BASICS_BASE_UINT_H_INCLUDED #define RIPPLE_BASICS_BASE_UINT_H_INCLUDED -#include #include #include +#include #include #include #include @@ -37,6 +37,27 @@ namespace ripple { +namespace detail +{ + +template > +struct is_contiguous_container + : std::false_type +{}; + +template +struct is_contiguous_container().size()), + decltype(std::declval().data()), + typename Container::value_type + >> + : std::true_type +{}; + +} // namespace detail + // This class stores its values internally in big-endian form template @@ -48,14 +69,12 @@ class base_uint static_assert (Bits >= 64, "The length of a base_uint in bits must be at least 64."); -protected: static constexpr std::size_t WIDTH = Bits / 32; // This is really big-endian in byte order. // We sometimes use std::uint32_t for speed. - using array_type = std::array; - array_type pn; + std::array data_; public: //-------------------------------------------------------------------------- @@ -64,7 +83,7 @@ public: // static std::size_t constexpr bytes = Bits / 8; - static_assert(sizeof(array_type) == bytes, ""); + static_assert(sizeof(data_) == bytes, ""); using size_type = std::size_t; using difference_type = std::ptrdiff_t; @@ -79,8 +98,8 @@ public: using const_reverse_iterator = std::reverse_iterator; using tag_type = Tag; - pointer data() { return reinterpret_cast(pn.data ()); } - const_pointer data() const { return reinterpret_cast(pn.data ()); } + pointer data() { return reinterpret_cast(data_.data ()); } + const_pointer data() const { return reinterpret_cast(data_.data ()); } iterator begin() { return data(); } @@ -112,7 +131,7 @@ private: explicit base_uint (void const* data, VoidHelper) { - memcpy (pn.data (), data, bytes); + memcpy (data_.data (), data, bytes); } public: @@ -126,25 +145,35 @@ public: *this = beast::zero; } - explicit base_uint (Blob const& vch) - { - assert (vch.size () == size ()); - - if (vch.size () == size ()) - memcpy (pn.data (), vch.data (), size ()); - else - *this = beast::zero; - } - explicit base_uint (std::uint64_t b) { *this = b; } - template - void copyFrom (base_uint const& other) + template ::value && + std::is_trivially_copyable::value + >> + explicit base_uint(Container const& c) { - memcpy (pn.data (), other.data(), bytes); + assert(c.size()*sizeof(typename Container::value_type) == size()); + std::memcpy(data_.data(), c.data(), size()); + } + + template + std::enable_if_t + < + detail::is_contiguous_container::value && + std::is_trivially_copyable::value, + base_uint& + > + operator=(Container const& c) + { + assert(c.size()*sizeof(typename Container::value_type) == size()); + std::memcpy(data_.data(), c.data(), size()); + return *this; } /* Construct from a raw pointer. @@ -159,7 +188,7 @@ public: int signum() const { for (int i = 0; i < WIDTH; i++) - if (pn[i] != 0) + if (data_[i] != 0) return 1; return 0; @@ -175,7 +204,7 @@ public: base_uint ret; for (int i = 0; i < WIDTH; i++) - ret.pn[i] = ~pn[i]; + ret.data_[i] = ~data_[i]; return ret; } @@ -190,15 +219,15 @@ public: }; // Put in least significant bits. ul = boost::endian::native_to_big(uHost); - pn[WIDTH-2] = u[0]; - pn[WIDTH-1] = u[1]; + data_[WIDTH-2] = u[0]; + data_[WIDTH-1] = u[1]; return *this; } base_uint& operator^= (const base_uint& b) { for (int i = 0; i < WIDTH; i++) - pn[i] ^= b.pn[i]; + data_[i] ^= b.data_[i]; return *this; } @@ -206,7 +235,7 @@ public: base_uint& operator&= (const base_uint& b) { for (int i = 0; i < WIDTH; i++) - pn[i] &= b.pn[i]; + data_[i] &= b.data_[i]; return *this; } @@ -214,7 +243,7 @@ public: base_uint& operator|= (const base_uint& b) { for (int i = 0; i < WIDTH; i++) - pn[i] |= b.pn[i]; + data_[i] |= b.data_[i]; return *this; } @@ -224,9 +253,9 @@ public: // prefix operator for (int i = WIDTH - 1; i >= 0; --i) { - pn[i] = boost::endian::native_to_big (boost::endian::big_to_native(pn[i]) + 1); - - if (pn[i] != 0) + data_[i] = boost::endian::native_to_big + (boost::endian::big_to_native(data_[i]) + 1); + if (data_[i] != 0) break; } @@ -246,8 +275,9 @@ public: { for (int i = WIDTH - 1; i >= 0; --i) { - auto prev = pn[i]; - pn[i] = boost::endian::native_to_big (boost::endian::big_to_native(pn[i]) - 1); + auto prev = data_[i]; + data_[i] = + boost::endian::native_to_big(boost::endian::big_to_native(data_[i]) - 1); if (prev != 0) break; @@ -271,23 +301,22 @@ public: for (int i = WIDTH; i--;) { - std::uint64_t n = carry + boost::endian::big_to_native(pn[i]) + - boost::endian::big_to_native(b.pn[i]); + std::uint64_t n = carry + boost::endian::big_to_native(data_[i]) + + boost::endian::big_to_native(b.data_[i]); - pn[i] = boost::endian::native_to_big (static_cast(n)); + data_[i] = boost::endian::native_to_big (static_cast(n)); carry = n >> 32; } return *this; } - template > + template friend void hash_append( Hasher& h, base_uint const& a) noexcept { // Do not allow any endian transformations on this memory - h(a.pn.data (), sizeof(a.pn)); + h(a.data_.data (), sizeof(a.data_)); } /** Parse a hex string into a base_uint @@ -298,7 +327,7 @@ public: { unsigned char* pOut = begin (); - for (int i = 0; i < sizeof (pn); ++i) + for (int i = 0; i < sizeof (data_); ++i) { auto hi = charUnHex(*psz++); if (hi == -1) @@ -391,7 +420,7 @@ public: base_uint& operator=(beast::Zero) { - pn.fill(0); + data_.fill(0); return *this; } diff --git a/src/ripple/crypto/GenerateDeterministicKey.h b/src/ripple/crypto/GenerateDeterministicKey.h index 26a4e1811..d23a8482a 100644 --- a/src/ripple/crypto/GenerateDeterministicKey.h +++ b/src/ripple/crypto/GenerateDeterministicKey.h @@ -26,6 +26,7 @@ #define RIPPLE_CRYPTO_GENERATEDETERMINISTICKEY_H_INCLUDED #include +#include namespace ripple { diff --git a/src/ripple/nodestore/NodeObject.h b/src/ripple/nodestore/NodeObject.h index 7da09a5e6..46e204e2d 100644 --- a/src/ripple/nodestore/NodeObject.h +++ b/src/ripple/nodestore/NodeObject.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_NODESTORE_NODEOBJECT_H_INCLUDED #define RIPPLE_NODESTORE_NODEOBJECT_H_INCLUDED +#include #include #include diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 40f915d3f..11f20396b 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -1578,8 +1578,8 @@ PeerImp::onMessage (std::shared_ptr const& m) return; } - auto const proposeHash = uint256::fromVoid(set.currenttxhash().data()); - auto const prevLedger = uint256::fromVoid(set.previousledger().data()); + uint256 const proposeHash{set.currenttxhash()}; + uint256 const prevLedger{set.previousledger()}; PublicKey const publicKey {makeSlice(set.nodepubkey())}; NetClock::time_point const closeTime { NetClock::duration{set.closetime()} }; diff --git a/src/ripple/protocol/STAccount.h b/src/ripple/protocol/STAccount.h index fc0101e61..1cb39f77b 100644 --- a/src/ripple/protocol/STAccount.h +++ b/src/ripple/protocol/STAccount.h @@ -34,7 +34,7 @@ private: // But an STAccount is always 160 bits, so we can store it with less // overhead in a ripple::uint160. However, so the serialized format of the // STAccount stays unchanged, we serialize and deserialize like an STBlob. - uint160 value_; + AccountID value_; bool default_; public: @@ -101,14 +101,12 @@ public: AccountID value() const noexcept { - AccountID result; - result.copyFrom (value_); - return result; + return value_; } void setValue (AccountID const& v) { - value_.copyFrom (v); + value_ = v; default_ = false; } }; diff --git a/src/ripple/protocol/STBitString.h b/src/ripple/protocol/STBitString.h index cfc308b6b..4cdca94dd 100644 --- a/src/ripple/protocol/STBitString.h +++ b/src/ripple/protocol/STBitString.h @@ -102,7 +102,7 @@ public: template void setValue (base_uint const& v) { - value_.copyFrom(v); + value_ = v; } value_type const& diff --git a/src/ripple/protocol/Serializer.h b/src/ripple/protocol/Serializer.h index f2c2890ae..bf468576f 100644 --- a/src/ripple/protocol/Serializer.h +++ b/src/ripple/protocol/Serializer.h @@ -21,6 +21,7 @@ #define RIPPLE_PROTOCOL_SERIALIZER_H_INCLUDED #include +#include #include #include #include diff --git a/src/ripple/protocol/impl/AccountID.cpp b/src/ripple/protocol/impl/AccountID.cpp index 2f8d5bb06..de19b7563 100644 --- a/src/ripple/protocol/impl/AccountID.cpp +++ b/src/ripple/protocol/impl/AccountID.cpp @@ -148,7 +148,7 @@ calcAccountID (PublicKey const& pk) AccountID const& xrpAccount() { - static AccountID const account(0); + static AccountID const account(beast::zero); return account; } diff --git a/src/ripple/protocol/impl/STAccount.cpp b/src/ripple/protocol/impl/STAccount.cpp index cfb8814c3..d34895a47 100644 --- a/src/ripple/protocol/impl/STAccount.cpp +++ b/src/ripple/protocol/impl/STAccount.cpp @@ -60,9 +60,9 @@ STAccount::STAccount (SerialIter& sit, SField const& name) STAccount::STAccount (SField const& n, AccountID const& v) : STBase (n) + , value_(v) , default_ (false) { - value_.copyFrom (v); } std::string STAccount::getText () const diff --git a/src/ripple/protocol/impl/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp index 9159b9749..73c4a84e1 100644 --- a/src/ripple/protocol/impl/STAmount.cpp +++ b/src/ripple/protocol/impl/STAmount.cpp @@ -106,12 +106,12 @@ STAmount::STAmount(SerialIter& sit, SField const& name) } Issue issue; - issue.currency.copyFrom (sit.get160 ()); + issue.currency = sit.get160(); if (isXRP (issue.currency)) Throw ("invalid native currency"); - issue.account.copyFrom (sit.get160 ()); + issue.account = sit.get160(); if (isXRP (issue.account)) Throw ("invalid native account"); diff --git a/src/ripple/protocol/impl/STPathSet.cpp b/src/ripple/protocol/impl/STPathSet.cpp index fa8f1b308..411550a4b 100644 --- a/src/ripple/protocol/impl/STPathSet.cpp +++ b/src/ripple/protocol/impl/STPathSet.cpp @@ -91,13 +91,13 @@ STPathSet::STPathSet (SerialIter& sit, SField const& name) AccountID issuer; if (hasAccount) - account.copyFrom (sit.get160 ()); + account = sit.get160(); if (hasCurrency) - currency.copyFrom (sit.get160 ()); + currency = sit.get160(); if (hasIssuer) - issuer.copyFrom (sit.get160 ()); + issuer = sit.get160(); path.emplace_back (account, currency, issuer, hasCurrency); } diff --git a/src/ripple/protocol/impl/UintTypes.cpp b/src/ripple/protocol/impl/UintTypes.cpp index 1a8c66be6..8ff9644ce 100644 --- a/src/ripple/protocol/impl/UintTypes.cpp +++ b/src/ripple/protocol/impl/UintTypes.cpp @@ -110,7 +110,7 @@ Currency to_currency(std::string const& code) Currency const& xrpCurrency() { - static Currency const currency(0); + static Currency const currency(beast::zero); return currency; } diff --git a/src/test/basics/base_uint_test.cpp b/src/test/basics/base_uint_test.cpp index 523c37cef..bb2fed80d 100644 --- a/src/test/basics/base_uint_test.cpp +++ b/src/test/basics/base_uint_test.cpp @@ -18,9 +18,11 @@ //============================================================================== #include +#include #include #include #include +#include #include @@ -57,6 +59,8 @@ struct base_uint_test : beast::unit_test::suite void run() override { + static_assert(!std::is_constructible>::value, ""); + static_assert(!std::is_assignable>::value, ""); // used to verify set insertion (hashing required) std::unordered_set> uset; diff --git a/src/test/protocol/SecretKey_test.cpp b/src/test/protocol/SecretKey_test.cpp index ad9f34891..4569df7dd 100644 --- a/src/test/protocol/SecretKey_test.cpp +++ b/src/test/protocol/SecretKey_test.cpp @@ -77,7 +77,7 @@ public: { blob b; hex_to_binary (s.begin (), s.end (), b); - return uint256::fromVoid(b.data()); + return uint256{b}; } static diff --git a/src/test/protocol/digest_test.cpp b/src/test/protocol/digest_test.cpp index 95a41107b..1c6e638b2 100644 --- a/src/test/protocol/digest_test.cpp +++ b/src/test/protocol/digest_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -115,12 +116,12 @@ public: digest_test () { beast::xor_shift_engine g(19207813); - std::uint8_t buf[32]; + std::array buf; for (int i = 0; i < 1000000; i++) { - beast::rngfill (buf, sizeof(buf), g); - dataset1.push_back (uint256::fromVoid (buf)); + beast::rngfill (buf.data(), buf.size(), g); + dataset1.push_back (uint256{buf}); } }