From 56c18f7768c3cec0cb6a2a86af466821d15db5c2 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Sun, 31 Aug 2014 00:07:02 -0700 Subject: [PATCH] Cleanups and fixes (RIPD-532): * Properly handle sfWalletLocator field * Plug a tiny memory leak * Avoid naked pointers * Remove unused variables * Other small cleanups --- src/ripple/common/KeyCache.h | 10 ++++---- src/ripple/http/impl/ServerImpl.cpp | 2 +- src/ripple/json/impl/json_reader.cpp | 4 ++-- src/ripple/module/app/ledger/InboundLedger.h | 4 ++-- src/ripple/module/app/ledger/LedgerMaster.cpp | 1 - .../module/app/ledger/OrderBookIterator.h | 2 +- .../module/app/main/LocalCredentials.cpp | 4 ---- src/ripple/module/app/paths/PathState.cpp | 2 +- src/ripple/module/app/paths/PathState.h | 6 +++-- src/ripple/module/app/paths/RippleState.h | 1 - .../module/app/transactors/SetAccount.cpp | 2 +- src/ripple/module/app/tx/LocalTxs.cpp | 10 ++++---- .../module/data/crypto/CKeyDeterministic.cpp | 2 +- src/ripple/module/data/crypto/RFC1751.cpp | 2 +- src/ripple/module/data/crypto/RFC1751.h | 2 +- src/ripple/module/data/protocol/STAmount.cpp | 24 +++++++++++-------- src/ripple/module/data/protocol/STAmount.h | 9 +++++-- .../module/data/protocol/STAmountRound.cpp | 6 ++--- src/ripple/module/rpc/impl/LookupLedger.cpp | 5 ---- src/ripple/peerfinder/impl/Bootcache.cpp | 1 + src/ripple/types/impl/UintTypes.cpp | 13 ++++------ 21 files changed, 54 insertions(+), 58 deletions(-) diff --git a/src/ripple/common/KeyCache.h b/src/ripple/common/KeyCache.h index 4673d3f662..63f53617a5 100644 --- a/src/ripple/common/KeyCache.h +++ b/src/ripple/common/KeyCache.h @@ -85,17 +85,19 @@ private: typedef typename map_type::iterator iterator; typedef std::lock_guard lock_guard; +public: + typedef typename map_type::size_type size_type; + +private: Mutex mutable m_mutex; map_type m_map; Stats mutable m_stats; clock_type& m_clock; std::string const m_name; - unsigned int m_target_size; + size_type m_target_size; clock_type::duration m_target_age; public: - typedef typename map_type::size_type size_type; - /** Construct with the specified name. @param size The initial target size. @@ -112,7 +114,6 @@ public: , m_target_size (target_size) , m_target_age (std::chrono::seconds (expiration_seconds)) { - assert (m_target_size >= 0); } // VFALCO TODO Use a forwarding constructor call here @@ -126,7 +127,6 @@ public: , m_target_size (target_size) , m_target_age (std::chrono::seconds (expiration_seconds)) { - assert (m_target_size >= 0); } //-------------------------------------------------------------------------- diff --git a/src/ripple/http/impl/ServerImpl.cpp b/src/ripple/http/impl/ServerImpl.cpp index 632a6d2e50..8832f27bd5 100644 --- a/src/ripple/http/impl/ServerImpl.cpp +++ b/src/ripple/http/impl/ServerImpl.cpp @@ -132,7 +132,7 @@ void ServerImpl::remove (Door& door) { std::lock_guard lock (mutex_); - state_.doors.push_back (door); + state_.doors.erase (state_.doors.iterator_to (door)); } //-------------------------------------------------------------------------- diff --git a/src/ripple/json/impl/json_reader.cpp b/src/ripple/json/impl/json_reader.cpp index 955c3f3eda..8951d0988b 100644 --- a/src/ripple/json/impl/json_reader.cpp +++ b/src/ripple/json/impl/json_reader.cpp @@ -643,8 +643,8 @@ Reader::readArray ( Token& tokenStart ) ok = readToken ( token ); } - bool badTokenType = ( token.type_ == tokenArraySeparator && - token.type_ == tokenArrayEnd ); + bool badTokenType = ( token.type_ != tokenArraySeparator && + token.type_ != tokenArrayEnd ); if ( !ok || badTokenType ) { diff --git a/src/ripple/module/app/ledger/InboundLedger.h b/src/ripple/module/app/ledger/InboundLedger.h index b327022746..48d7633daf 100644 --- a/src/ripple/module/app/ledger/InboundLedger.h +++ b/src/ripple/module/app/ledger/InboundLedger.h @@ -66,7 +66,7 @@ public: { return mAborted || isComplete () || isFailed (); } - Ledger::ref getLedger () + Ledger::ref getLedger () const { return mLedger; } @@ -74,7 +74,7 @@ public: { mAborted = true; } - std::uint32_t getSeq () + std::uint32_t getSeq () const { return mSeq; } diff --git a/src/ripple/module/app/ledger/LedgerMaster.cpp b/src/ripple/module/app/ledger/LedgerMaster.cpp index e370d7d59e..ce726d163a 100644 --- a/src/ripple/module/app/ledger/LedgerMaster.cpp +++ b/src/ripple/module/app/ledger/LedgerMaster.cpp @@ -68,7 +68,6 @@ public: int mFillInProgress; int mPathFindThread; // Pathfinder jobs dispatched - bool mPathFindNewLedger; bool mPathFindNewRequest; std::atomic mPubLedgerClose; diff --git a/src/ripple/module/app/ledger/OrderBookIterator.h b/src/ripple/module/app/ledger/OrderBookIterator.h index ce6493688f..b9a03019e1 100644 --- a/src/ripple/module/app/ledger/OrderBookIterator.h +++ b/src/ripple/module/app/ledger/OrderBookIterator.h @@ -215,7 +215,7 @@ public: return std::addressof(mEntrySet) == std::addressof(other.mEntrySet) && mDirectoryIterator == other.mDirectoryIterator && - mOfferIterator == mOfferIterator; + mOfferIterator == other.mOfferIterator; } bool diff --git a/src/ripple/module/app/main/LocalCredentials.cpp b/src/ripple/module/app/main/LocalCredentials.cpp index 96620bbae6..aaa48153bb 100644 --- a/src/ripple/module/app/main/LocalCredentials.cpp +++ b/src/ripple/module/app/main/LocalCredentials.cpp @@ -158,14 +158,10 @@ bool LocalCredentials::dataStore (std::string const& strKey, std::string const& auto sl (getApp().getRpcDB ()->lock ()); - bool bSuccess = false; - return (db->executeSQL (str (boost::format ("REPLACE INTO RPCData (Key, Value) VALUES (%s,%s);") % sqlEscape (strKey) % sqlEscape (strValue) ))); - - return bSuccess; } } // ripple diff --git a/src/ripple/module/app/paths/PathState.cpp b/src/ripple/module/app/paths/PathState.cpp index cafa0c0182..899b703652 100644 --- a/src/ripple/module/app/paths/PathState.cpp +++ b/src/ripple/module/app/paths/PathState.cpp @@ -64,7 +64,7 @@ void PathState::reset(STAmount const& in, STAmount const& out) } // Return true, iff lhs has less priority than rhs. -bool PathState::lessPriority (PathState& lhs, PathState& rhs) +bool PathState::lessPriority (PathState const& lhs, PathState const& rhs) { // First rank is quality. if (lhs.uQuality != rhs.uQuality) diff --git a/src/ripple/module/app/paths/PathState.h b/src/ripple/module/app/paths/PathState.h index ffb9365a86..be1cca6403 100644 --- a/src/ripple/module/app/paths/PathState.h +++ b/src/ripple/module/app/paths/PathState.h @@ -34,7 +34,9 @@ class PathState : public CountedObject typedef std::vector List; PathState (STAmount const& saSend, STAmount const& saSendMax) - : saInReq (saSendMax) + : mIndex (0) + , uQuality (0) + , saInReq (saSendMax) , saOutReq (saSend) { } @@ -99,7 +101,7 @@ class PathState : public CountedObject Account const& sourceAccountID); void checkFreeze (); - static bool lessPriority (PathState& lhs, PathState& rhs); + static bool lessPriority (PathState const& lhs, PathState const& rhs); LedgerEntrySet& ledgerEntries() { return lesEntries; } diff --git a/src/ripple/module/app/paths/RippleState.h b/src/ripple/module/app/paths/RippleState.h index a1dde560a7..90001d2b61 100644 --- a/src/ripple/module/app/paths/RippleState.h +++ b/src/ripple/module/app/paths/RippleState.h @@ -106,7 +106,6 @@ public: std::uint32_t getQualityIn () const { return ((std::uint32_t) (mViewLowest ? mLowQualityIn : mHighQualityIn)); - return ((std::uint32_t) (mViewLowest ? mLowQualityIn : mHighQualityIn)); } std::uint32_t getQualityOut () const diff --git a/src/ripple/module/app/transactors/SetAccount.cpp b/src/ripple/module/app/transactors/SetAccount.cpp index a9f83ae8aa..69987c3227 100644 --- a/src/ripple/module/app/transactors/SetAccount.cpp +++ b/src/ripple/module/app/transactors/SetAccount.cpp @@ -234,7 +234,7 @@ public: if (!uHash) { m_journal.trace << "unset wallet locator"; - mTxnAccount->makeFieldAbsent (sfEmailHash); + mTxnAccount->makeFieldAbsent (sfWalletLocator); } else { diff --git a/src/ripple/module/app/tx/LocalTxs.cpp b/src/ripple/module/app/tx/LocalTxs.cpp index c77b1b990d..0f4bee4c5e 100644 --- a/src/ripple/module/app/tx/LocalTxs.cpp +++ b/src/ripple/module/app/tx/LocalTxs.cpp @@ -69,27 +69,27 @@ public: } } - uint256 const& getID () + uint256 const& getID () const { return m_id; } - std::uint32_t getSeq () + std::uint32_t getSeq () const { return m_seq; } - bool isExpired (LedgerIndex i) + bool isExpired (LedgerIndex i) const { return i > m_expire; } - SerializedTransaction::ref getTX () + SerializedTransaction::ref getTX () const { return m_txn; } - RippleAddress const& getAccount () + RippleAddress const& getAccount () const { return m_account; } diff --git a/src/ripple/module/data/crypto/CKeyDeterministic.cpp b/src/ripple/module/data/crypto/CKeyDeterministic.cpp index 4d0375327a..2b657758d3 100644 --- a/src/ripple/module/data/crypto/CKeyDeterministic.cpp +++ b/src/ripple/module/data/crypto/CKeyDeterministic.cpp @@ -189,7 +189,7 @@ EC_KEY* CKey::GenerateRootPubKey (BIGNUM* pubGenerator) } // --> public generator -static BIGNUM* makeHash (RippleAddress const& pubGen, int seq, BIGNUM* order) +static BIGNUM* makeHash (RippleAddress const& pubGen, int seq, BIGNUM const* order) { int subSeq = 0; BIGNUM* ret = nullptr; diff --git a/src/ripple/module/data/crypto/RFC1751.cpp b/src/ripple/module/data/crypto/RFC1751.cpp index 10451c7023..b60118b0df 100644 --- a/src/ripple/module/data/crypto/RFC1751.cpp +++ b/src/ripple/module/data/crypto/RFC1751.cpp @@ -251,7 +251,7 @@ char const* RFC1751::s_dictionary [2048] = /* Extract 'length' bits from the char array 's' starting with bit 'start' */ -unsigned long RFC1751::extract (char* s, int start, int length) +unsigned long RFC1751::extract (char const* s, int start, int length) { unsigned char cl; unsigned char cc; diff --git a/src/ripple/module/data/crypto/RFC1751.h b/src/ripple/module/data/crypto/RFC1751.h index c8649267aa..6054ee5e0f 100644 --- a/src/ripple/module/data/crypto/RFC1751.h +++ b/src/ripple/module/data/crypto/RFC1751.h @@ -39,7 +39,7 @@ public: static beast::String getWordFromBlob (void const* data, size_t bytes); private: - static unsigned long extract (char* s, int start, int length); + static unsigned long extract (char const* s, int start, int length); static void btoe (std::string& strHuman, std::string const& strData); static void insert (char* s, int x, int start, int length); static void standard (std::string& strWord); diff --git a/src/ripple/module/data/protocol/STAmount.cpp b/src/ripple/module/data/protocol/STAmount.cpp index 7995580658..4a421c8a31 100644 --- a/src/ripple/module/data/protocol/STAmount.cpp +++ b/src/ripple/module/data/protocol/STAmount.cpp @@ -438,19 +438,23 @@ int STAmount::compare (STAmount const& a) const return 0; } -STAmount* STAmount::construct (SerializerIterator& sit, SField::ref name) +std::unique_ptr +STAmount::construct (SerializerIterator& sit, SField::ref name) { std::uint64_t value = sit.get64 (); + // native if ((value & cNotNative) == 0) { - // native + // positive if ((value & cPosNative) != 0) - return new STAmount (name, value & ~cPosNative, false); // positive - else if (value == 0) + return std::make_unique (name, value & ~cPosNative, false); + + // negative + if (value == 0) throw std::runtime_error ("negative zero is not canonical"); - return new STAmount (name, value, true); // negative + return std::make_unique (name, value, true); } Issue issue; @@ -482,13 +486,13 @@ STAmount* STAmount::construct (SerializerIterator& sit, SField::ref name) throw std::runtime_error ("invalid currency value"); } - return new STAmount (name, issue, value, offset, isNegative); + return std::make_unique (name, issue, value, offset, isNegative); } if (offset != 512) throw std::runtime_error ("invalid currency value"); - return new STAmount (name, issue); + return std::make_unique (name, issue); } std::int64_t STAmount::getSNValue () const @@ -1046,12 +1050,12 @@ STAmount STAmount::getPay ( STAmount STAmount::deserialize (SerializerIterator& it) { - auto s = dynamic_cast (construct (it, sfGeneric)); + auto s = construct (it, sfGeneric); + if (!s) throw std::runtime_error("Deserialization error"); - STAmount ret (*s); - return ret; + return STAmount (*s); } std::string STAmount::getFullText () const diff --git a/src/ripple/module/data/protocol/STAmount.h b/src/ripple/module/data/protocol/STAmount.h index 05240c43a9..0efc99da11 100644 --- a/src/ripple/module/data/protocol/STAmount.h +++ b/src/ripple/module/data/protocol/STAmount.h @@ -24,6 +24,8 @@ #include #include +#include // + namespace ripple { // Internal form: @@ -128,7 +130,7 @@ public: static std::unique_ptr deserialize ( SerializerIterator& sit, SField::ref name) { - return std::unique_ptr (construct (sit, name)); + return construct (sit, name); } bool bSetJson (Json::Value const& jvSource); @@ -406,7 +408,10 @@ private: { return new STAmount (*this); } - static STAmount* construct (SerializerIterator&, SField::ref name); + + static + std::unique_ptr + construct (SerializerIterator&, SField::ref name); STAmount (SField::ref name, Issue const& issue, std::uint64_t val, int off, bool isNat, bool negative) diff --git a/src/ripple/module/data/protocol/STAmountRound.cpp b/src/ripple/module/data/protocol/STAmountRound.cpp index dd28ed75cb..97c8dc28c7 100644 --- a/src/ripple/module/data/protocol/STAmountRound.cpp +++ b/src/ripple/module/data/protocol/STAmountRound.cpp @@ -26,8 +26,7 @@ void STAmount::canonicalizeRound ( return; WriteLog (lsTRACE, STAmount) - << "canonicalize< " << value - << ":" << offset << (roundUp ? " up" : " down"); + << "canonicalizeRound< " << value << ":" << offset; if (isNative) { @@ -61,8 +60,7 @@ void STAmount::canonicalizeRound ( } WriteLog (lsTRACE, STAmount) - << "canonicalize> " << value - << ":" << offset << (roundUp ? " up" : " down"); + << "canonicalizeRound> " << value << ":" << offset; } STAmount STAmount::addRound (STAmount const& v1, STAmount const& v2, bool roundUp) diff --git a/src/ripple/module/rpc/impl/LookupLedger.cpp b/src/ripple/module/rpc/impl/LookupLedger.cpp index c73fa3bd96..018819810b 100644 --- a/src/ripple/module/rpc/impl/LookupLedger.cpp +++ b/src/ripple/module/rpc/impl/LookupLedger.cpp @@ -59,11 +59,6 @@ Json::Value lookupLedger ( jsonHash = params[jss::ledger]; jsonIndex = Json::Value (""); } - else if (params[jss::ledger].isNumeric ()) - { - jsonIndex = params[jss::ledger]; - jsonHash = Json::Value ("0"); - } else { jsonIndex = params[jss::ledger]; diff --git a/src/ripple/peerfinder/impl/Bootcache.cpp b/src/ripple/peerfinder/impl/Bootcache.cpp index 526607cd83..3ff4d77f4f 100644 --- a/src/ripple/peerfinder/impl/Bootcache.cpp +++ b/src/ripple/peerfinder/impl/Bootcache.cpp @@ -30,6 +30,7 @@ Bootcache::Bootcache ( , m_clock (clock) , m_journal (journal) , m_whenUpdate (m_clock.now ()) + , m_needsUpdate (false) { } diff --git a/src/ripple/types/impl/UintTypes.cpp b/src/ripple/types/impl/UintTypes.cpp index 7c608fe098..25f5ba9a7f 100644 --- a/src/ripple/types/impl/UintTypes.cpp +++ b/src/ripple/types/impl/UintTypes.cpp @@ -33,9 +33,9 @@ std::string to_string(Currency const& currency) { static Currency const sIsoBits ("FFFFFFFFFFFFFFFFFFFFFFFF000000FFFFFFFFFF"); - // Characters we are willing to include the ASCII representation - // of a three-letter currency code - static std::string legalASCIICurrencyCharacters = + // Characters we are willing to allow in the ASCII representation of a + // three-letter currency code. + static std::string const allowed_characters = "abcdefghijklmnopqrstuvwxyz" "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "0123456789" @@ -59,16 +59,13 @@ std::string to_string(Currency const& currency) // Specifying the system currency code using ISO-style representation // is not allowed. if ((iso != systemCurrencyCode()) && - (iso.find_first_not_of (legalASCIICurrencyCharacters) == std::string::npos)) + (iso.find_first_not_of (allowed_characters) == std::string::npos)) { return iso; } } - uint160 simple; - simple.copyFrom(currency); - - return to_string(simple); + return strHex (currency.begin (), currency.size ()); } bool to_currency(Currency& currency, std::string const& code)