From cbad4e53d26ea8e7b9a601e3eef0e09945c42cec Mon Sep 17 00:00:00 2001 From: Richard Holland Date: Mon, 23 Jan 2023 10:29:55 +0000 Subject: [PATCH] add a hard cap to the number of modified state entries there can be in a set of hook executions --- src/ripple/app/hook/Enum.h | 2 + src/ripple/app/hook/applyHook.h | 23 +++++++---- src/ripple/app/hook/impl/applyHook.cpp | 57 +++++++++++++------------- src/ripple/app/misc/impl/TxQ.cpp | 2 - src/test/rpc/LedgerRPC_test.cpp | 1 + 5 files changed, 46 insertions(+), 39 deletions(-) diff --git a/src/ripple/app/hook/Enum.h b/src/ripple/app/hook/Enum.h index c5a74e4ef..e7ed4e112 100644 --- a/src/ripple/app/hook/Enum.h +++ b/src/ripple/app/hook/Enum.h @@ -227,6 +227,7 @@ namespace hook_api INVALID_KEY = -41, // user supplied key was not valid NOT_A_STRING = -42, // nul terminator missing from a string argument MEM_OVERLAP = -43, // one or more specified buffers are the same memory + TOO_MANY_STATE_MODIFICATIONS = -44, // more than 5000 modified state entires in the combined hook chains }; enum ExitType : uint8_t @@ -237,6 +238,7 @@ namespace hook_api ACCEPT = 3, }; + const uint16_t max_state_modifications = 5000; const uint8_t max_slots = 255; const uint8_t max_nonce = 255; const uint8_t max_emit = 255; diff --git a/src/ripple/app/hook/applyHook.h b/src/ripple/app/hook/applyHook.h index b40bceea7..7b49b3bcc 100644 --- a/src/ripple/app/hook/applyHook.h +++ b/src/ripple/app/hook/applyHook.h @@ -27,16 +27,21 @@ namespace hook // and is preserved across the execution of the set of hook chains // being executed in the current transaction. It is committed to lgr // only upon tesSuccess for the otxn. - using HookStateMap = - std::map< - ripple::AccountID, // account that owns the state - std::pair< - int64_t, // remaining available ownercount - std::map>>>>; // the value + int64_t, // remaining available ownercount + std::map>>>> // the value + { + public: + uint32_t modified_entry_count = 0; // track the number of total modified + }; + using namespace ripple; diff --git a/src/ripple/app/hook/impl/applyHook.cpp b/src/ripple/app/hook/impl/applyHook.cpp index 62a5c2f28..db0e58f00 100644 --- a/src/ripple/app/hook/impl/applyHook.cpp +++ b/src/ripple/app/hook/impl/applyHook.cpp @@ -1204,7 +1204,7 @@ lookup_state_cache( // update the state cache inline -bool // true unless a new hook state was required and the acc has insufficent reserve +int64_t // if negative a hook return code, if == 1 then success set_state_cache( hook::HookContext& hookCtx, ripple::AccountID const& acc, @@ -1213,8 +1213,11 @@ set_state_cache( ripple::Blob& data, bool modified) { - auto& stateMap = hookCtx.result.stateMap; + + if (modified && stateMap.modified_entry_count > max_state_modifications) + return TOO_MANY_STATE_MODIFICATIONS; + if (stateMap.find(acc) == stateMap.end()) { @@ -1224,7 +1227,7 @@ set_state_cache( auto const accSLE = hookCtx.applyCtx.view().read(ripple::keylet::account(acc)); if (!accSLE) - return false; + return DOESNT_EXIST; STAmount bal = accSLE->getFieldAmount(sfBalance); @@ -1239,7 +1242,9 @@ set_state_cache( availableForReserves /= increment; if (availableForReserves < 1 && modified) - return false; + return RESERVE_INSUFFICIENT; + + stateMap.modified_entry_count++; stateMap[acc] = { @@ -1259,7 +1264,7 @@ set_state_cache( } } }; - return true; + return 1; } auto& stateMapAcc = stateMap[acc].second; @@ -1272,8 +1277,10 @@ set_state_cache( if (modified) { if (!canReserveNew) - return false; + return RESERVE_INSUFFICIENT; + availableForReserves--; + stateMap.modified_entry_count++; } stateMapAcc[ns] = @@ -1283,7 +1290,7 @@ set_state_cache( } }; - return true; + return 1; } auto& stateMapNs = stateMapAcc[ns]; @@ -1292,13 +1299,14 @@ set_state_cache( if (modified) { if (!canReserveNew) - return false; + return RESERVE_INSUFFICIENT; availableForReserves--; + stateMap.modified_entry_count++; } stateMapNs[key] = { modified, data }; hookCtx.result.changedStateCount++; - return true; + return 1; } if (modified) @@ -1306,11 +1314,12 @@ set_state_cache( if (!stateMapNs[key].first) hookCtx.result.changedStateCount++; + stateMap.modified_entry_count++; stateMapNs[key].first = true; } stateMapNs[key].second = data; - return true; + return 1; } DEFINE_HOOK_FUNCTION( @@ -1400,8 +1409,8 @@ DEFINE_HOOK_FUNCTION( // local modifications are always allowed if (aread_len == 0 || acc == hookCtx.result.account) { - if (!set_state_cache(hookCtx, acc, ns, *key, data, true)) - return RESERVE_INSUFFICIENT; + if (int64_t ret = set_state_cache(hookCtx, acc, ns, *key, data, true); ret < 0) + return ret; return read_len; } @@ -1415,8 +1424,8 @@ DEFINE_HOOK_FUNCTION( if (cacheEntry && cacheEntry->get().first) { // if a cache entry already exists and it has already been modified don't check grants again - if (!set_state_cache(hookCtx, acc, ns, *key, data, true)) - return RESERVE_INSUFFICIENT; + if (int64_t ret = set_state_cache(hookCtx, acc, ns, *key, data, true); ret < 0) + return ret; return read_len; } @@ -1463,11 +1472,6 @@ DEFINE_HOOK_FUNCTION( continue; } - - - //if (hookObj->isFieldPresent(sfHookNamespace) && hookObj->getFieldH256(sfHookNamespace) != ns) - // continue; - // this is expensive search so we'll disallow after one failed attempt for (auto const& hookGrant : hookGrants) { @@ -1493,14 +1497,14 @@ DEFINE_HOOK_FUNCTION( return NOT_AUTHORIZED; } - if (!set_state_cache( + if (int64_t ret = set_state_cache( hookCtx, acc, ns, *key, data, - true)) - return RESERVE_INSUFFICIENT; + true); ret < 0) + return ret; return read_len; } @@ -1839,7 +1843,7 @@ DEFINE_HOOK_FUNCTION( Blob b = hsSLE->getFieldVL(sfHookStateData); // it exists add it to cache and return it - if (!set_state_cache(hookCtx, acc, ns, *key, b, false)) + if (set_state_cache(hookCtx, acc, ns, *key, b, false) < 0) return INTERNAL_ERROR; // should never happen if (write_ptr == 0) @@ -3690,8 +3694,8 @@ DEFINE_HOOK_FUNCTION( if (read_len > 49) return TOO_BIG; - // RH TODO we shouldn't need to slice this input but the base58 routine fails if we dont... maybe - // some encoding or padding that shouldnt be there or maybe something that should be there + // RH TODO we shouldn't need to slice this input but the base58 routine fails if we dont... + // maybe some encoding or padding that shouldnt be there or maybe something that should be there char buffer[50]; for (int i = 0; i < read_len; ++i) @@ -3700,13 +3704,10 @@ DEFINE_HOOK_FUNCTION( std::string raddr{buffer}; - //std::string raddr((char*)(memory + read_ptr), read_len); - auto const result = decodeBase58Token(raddr, TokenType::AccountID); if (result.empty()) return INVALID_ARGUMENT; - WRITE_WASM_MEMORY_AND_RETURN( write_ptr, write_len, result.data(), 20, diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index eee970373..f27d3f754 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -1517,8 +1517,6 @@ TxQ::accept(Application& app, OpenView& view) auto s = std::make_shared(); efTx.add(*s); - - // RH TODO: should this txn be added in a different way to prevent any chance of failure app.getHashRouter().setFlags(txID, SF_PRIVATE2); app.getHashRouter().setFlags(txID, SF_EMITTED); view.rawTxInsert(txID, std::move(s), nullptr); diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 2580c4bfe..d6248686e 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -1547,6 +1547,7 @@ class LedgerRPC_test : public beast::unit_test::suite BEAST_EXPECT(txj["retries_remaining"] == 10); BEAST_EXPECT(txj.isMember(jss::tx)); auto const& tx = txj[jss::tx]; + std::cout << tx << "\n"; BEAST_EXPECT(tx[jss::Account] == alice.human()); BEAST_EXPECT(tx[jss::TransactionType] == jss::AccountSet); return tx[jss::hash].asString();