From f72b14ec369880e0a12bb45785ce283d5641552b Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Fri, 30 Oct 2015 17:48:42 -0700 Subject: [PATCH] Make STAccount only constructible with 160 bits (RIPD-994): If someone attempts to construct an STAccount with something other than 160 bits the constructor now throws. Since an STAccount now enforces that it always stores exactly 160 bits, we use a fixed-sized uint160 for the storage, replacing a variable sized STBlob. In order to leave the ledger and wire formats unaffected, the STAccount still serializes and deserializes itself as though it were variable length. --- src/ripple/protocol/STAccount.h | 78 +++++++++++++------- src/ripple/protocol/impl/STAccount.cpp | 58 +++++++++++---- src/ripple/protocol/tests/STAccount.test.cpp | 12 ++- 3 files changed, 104 insertions(+), 44 deletions(-) diff --git a/src/ripple/protocol/STAccount.h b/src/ripple/protocol/STAccount.h index 21ec92e4f..aabe66650 100644 --- a/src/ripple/protocol/STAccount.h +++ b/src/ripple/protocol/STAccount.h @@ -21,45 +21,42 @@ #define RIPPLE_PROTOCOL_STACCOUNT_H_INCLUDED #include -#include +#include #include #include namespace ripple { class STAccount final - : public STBlob + : public STBase { +private: + // The original implementation of STAccount kept the value in an STBlob. + // 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_; + bool default_; + public: using value_type = AccountID; - STAccount (SField const& n, Buffer&& v) - : STBlob (n, std::move(v)) - { - ; - } - STAccount (SField const& n, AccountID const& v); - STAccount (SField const& n) : STBlob (n) - { - ; - } - STAccount () - { - ; - } - + STAccount (); + STAccount (SField const& n); + STAccount (SField const& n, Buffer&& v); STAccount (SerialIter& sit, SField const& name); + STAccount (SField const& n, AccountID const& v); STBase* copy (std::size_t n, void* buf) const override { - return emplace(n, buf, *this); + return emplace (n, buf, *this); } STBase* move (std::size_t n, void* buf) override { - return emplace(n, buf, std::move(*this)); + return emplace (n, buf, std::move(*this)); } SerializedTypeID getSType () const override @@ -69,14 +66,40 @@ public: std::string getText () const override; + void + add (Serializer& s) const override + { + assert (fName->isBinary ()); + assert (fName->fieldType == STI_ACCOUNT); + + // Preserve the serialization behavior of an STBlob: + // o If we are default (all zeros) serialize as an empty blob. + // o Otherwise serialize 160 bits. + int const size = isDefault() ? 0 : uint160::bytes; + s.addVL (value_.data(), size); + } + + bool + isEquivalent (const STBase& t) const override + { + auto const* const tPtr = dynamic_cast(&t); + return tPtr && (default_ == tPtr->default_) && (value_ == tPtr->value_); + } + + bool + isDefault () const override + { + return default_; + } + STAccount& - operator= (value_type const& value) + operator= (AccountID const& value) { setValueH160(value); return *this; } - value_type + AccountID value() const noexcept { AccountID result; @@ -87,8 +110,8 @@ public: template void setValueH160 (base_uint<160, Tag> const& v) { - peekValue () = Buffer (v.data (), v.size ()); - assert (peekValue ().size () == (160 / 8)); + value_.copyFrom (v); + default_ = false; } // VFALCO This is a clumsy interface, it should return @@ -98,13 +121,16 @@ public: template bool getValueH160 (base_uint<160, Tag>& v) const { - auto success = isValueH160 (); + bool const success = isValueH160(); if (success) - memcpy (v.begin (), peekValue ().data (), (160 / 8)); + v.copyFrom (value_); return success; } - bool isValueH160 () const; + bool isValueH160 () const + { + return ! isDefault(); + } }; } // ripple diff --git a/src/ripple/protocol/impl/STAccount.cpp b/src/ripple/protocol/impl/STAccount.cpp index e7b06382e..f371c451e 100644 --- a/src/ripple/protocol/impl/STAccount.cpp +++ b/src/ripple/protocol/impl/STAccount.cpp @@ -19,32 +19,58 @@ #include #include -#include namespace ripple { +STAccount::STAccount () + : STBase () + , value_ (beast::zero) + , default_ (true) +{ +} + +STAccount::STAccount (SField const& n) + : STBase (n) + , value_ (beast::zero) + , default_ (true) +{ +} + +STAccount::STAccount (SField const& n, Buffer&& v) + : STAccount (n) +{ + if (v.empty()) + return; // Zero is a valid size for a defaulted STAccount. + + // Is it safe to throw from this constructor? Today (November 2015) + // the only place that calls this constructor is + // STVar::STVar (SerialIter&, SField const&) + // which throws. If STVar can throw in its constructor, then so can + // STAccount. + if (v.size() != uint160::bytes) + throw std::runtime_error ("Invalid STAccount size"); + + default_ = false; + memcpy (value_.begin(), v.data (), uint160::bytes); +} + STAccount::STAccount (SerialIter& sit, SField const& name) : STAccount(name, sit.getVLBuffer()) { } +STAccount::STAccount (SField const& n, AccountID const& v) + : STBase (n) + , default_ (false) +{ + value_.copyFrom (v); +} + std::string STAccount::getText () const { - AccountID u; - RippleAddress a; - if (! getValueH160 (u)) - return STBlob::getText (); - return toBase58(u); -} - -STAccount::STAccount (SField const& n, AccountID const& v) - : STBlob (n, v.data (), v.size ()) -{ -} - -bool STAccount::isValueH160 () const -{ - return peekValue ().size () == (160 / 8); + if (isDefault()) + return ""; + return toBase58 (value()); } } // ripple diff --git a/src/ripple/protocol/tests/STAccount.test.cpp b/src/ripple/protocol/tests/STAccount.test.cpp index fe1d1f4f0..96f239cf9 100644 --- a/src/ripple/protocol/tests/STAccount.test.cpp +++ b/src/ripple/protocol/tests/STAccount.test.cpp @@ -102,8 +102,16 @@ struct STAccount_test : public beast::unit_test::suite const std::uint8_t bits128[] {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0}; s.addVL (bits128, sizeof (bits128)); SerialIter sit (s.slice ()); - STAccount const deserializedBadSize (sit, sfAccount); - expect (! deserializedBadSize.isValueH160()); + try + { + // Constructing an STAccount with a bad size should throw. + STAccount const deserializedBadSize (sit, sfAccount); + } + catch (std::runtime_error const& ex) + { + expect (ex.what() == std::string("Invalid STAccount size")); + } + } // Interestingly, equal values but different types are equivalent!