From 5f5a947d6cf77d4ff7dbbe4aa3cb5696be0af2c1 Mon Sep 17 00:00:00 2001 From: Richard Holland Date: Fri, 17 Feb 2023 11:20:03 +0000 Subject: [PATCH] additional fixes for #8 #10 and #14 --- src/ripple/app/hook/Macro.h | 4 +- src/ripple/app/hook/applyHook.h | 2 +- src/ripple/app/hook/impl/applyHook.cpp | 58 +++++++++++------------- src/ripple/app/tx/impl/Transactor.cpp | 62 +++++++++++--------------- 4 files changed, 56 insertions(+), 70 deletions(-) diff --git a/src/ripple/app/hook/Macro.h b/src/ripple/app/hook/Macro.h index a2167a6bcf7..c7e9bb4e87c 100644 --- a/src/ripple/app/hook/Macro.h +++ b/src/ripple/app/hook/Macro.h @@ -181,8 +181,8 @@ // ptr = pointer inside the wasm memory space #define NOT_IN_BOUNDS(ptr, len, memory_length)\ - ((static_cast(ptr) > static_cast(memory_length)) || \ - ((static_cast(ptr) + static_cast(len)) >= static_cast(memory_length))) + ((static_cast(ptr) >= static_cast(memory_length)) || \ + ((static_cast(ptr) + static_cast(len)) > static_cast(memory_length))) #define HOOK_EXIT(read_ptr, read_len, error_code, exit_type)\ {\ diff --git a/src/ripple/app/hook/applyHook.h b/src/ripple/app/hook/applyHook.h index cebafaf0cc6..b08f38cc7c4 100644 --- a/src/ripple/app/hook/applyHook.h +++ b/src/ripple/app/hook/applyHook.h @@ -398,7 +398,7 @@ namespace hook bool /* retval of true means an error */ gatherHookParameters( std::shared_ptr const& hookDef, - ripple::STObject const* hookObj, + ripple::STObject const& hookObj, std::map, std::vector>& parameters, beast::Journal const& j_); diff --git a/src/ripple/app/hook/impl/applyHook.cpp b/src/ripple/app/hook/impl/applyHook.cpp index b65b72fbd87..8a714d3e608 100644 --- a/src/ripple/app/hook/impl/applyHook.cpp +++ b/src/ripple/app/hook/impl/applyHook.cpp @@ -632,9 +632,6 @@ get_free_slot(hook::HookContext& hookCtx) // slot ahead of when the counter gets there do { - if (hookCtx.slot_counter >= hook_api::max_slots) - return {}; - slot_into = ++hookCtx.slot_counter; } while (hookCtx.slot.find(slot_into) != hookCtx.slot.end() && @@ -642,6 +639,9 @@ get_free_slot(hook::HookContext& hookCtx) // then we will return the final slot every time. hookCtx.slot_counter <= hook_api::max_slots); + if (hookCtx.slot_counter > hook_api::max_slots) + return {}; + return slot_into; } @@ -1529,32 +1529,30 @@ DEFINE_HOOK_FUNCTION( // we do this by iterating the hooks installed on the foreign account and in turn their grants and namespaces auto const& hooks = sle->getFieldArray(sfHooks); - for (auto const& hook : hooks) + for (auto const& hookObj : hooks) { - STObject const* hookObj = dynamic_cast(&hook); - // skip blank entries - if (!hookObj->isFieldPresent(sfHookHash)) + if (!hookObj.isFieldPresent(sfHookHash)) continue; - if (!hookObj->isFieldPresent(sfHookGrants)) + if (!hookObj.isFieldPresent(sfHookGrants)) continue; - auto const& hookGrants = hookObj->getFieldArray(sfHookGrants); + auto const& hookGrants = hookObj.getFieldArray(sfHookGrants); if (hookGrants.size() < 1) continue; // the grant allows the hook to modify the granter's namespace only - if (hookObj->isFieldPresent(sfHookNamespace)) + if (hookObj.isFieldPresent(sfHookNamespace)) { - if (hookObj->getFieldH256(sfHookNamespace) != ns) + if (hookObj.getFieldH256(sfHookNamespace) != ns) continue; } else { // fetch the hook definition - auto const def = view.read(ripple::keylet::hookDefinition(hookObj->getFieldH256(sfHookHash))); + auto const def = view.read(ripple::keylet::hookDefinition(hookObj.getFieldH256(sfHookHash))); if (!def) // should never happen except in a rare race condition continue; if (def->getFieldH256(sfHookNamespace) != ns) @@ -1562,13 +1560,12 @@ DEFINE_HOOK_FUNCTION( } // this is expensive search so we'll disallow after one failed attempt - for (auto const& hookGrant : hookGrants) + for (auto const& hookGrantObj : hookGrants) { - STObject const* hookGrantObj = dynamic_cast(&hookGrant); - bool hasAuthorizedField = hookGrantObj->isFieldPresent(sfAuthorize); + bool hasAuthorizedField = hookGrantObj.isFieldPresent(sfAuthorize); - if (hookGrantObj->getFieldH256(sfHookHash) == hookCtx.result.hookHash && - (!hasAuthorizedField || hookGrantObj->getAccountID(sfAuthorize) == hookCtx.result.account)) + if (hookGrantObj.getFieldH256(sfHookHash) == hookCtx.result.hookHash && + (!hasAuthorizedField || hookGrantObj.getAccountID(sfAuthorize) == hookCtx.result.account)) { found_auth = true; break; @@ -1659,7 +1656,7 @@ bool /* retval of true means an error */ hook:: gatherHookParameters( std::shared_ptr const& hookDef, - ripple::STObject const* hookObj, + ripple::STObject const& hookObj, std::map, std::vector>& parameters, beast::Journal const& j_) { @@ -1672,22 +1669,20 @@ gatherHookParameters( // first defaults auto const& defaultParameters = hookDef->getFieldArray(sfHookParameters); - for (auto const& hookParameter : defaultParameters) + for (auto const& hookParameterObj : defaultParameters) { - auto const& hookParameterObj = dynamic_cast(&hookParameter); - parameters[hookParameterObj->getFieldVL(sfHookParameterName)] = - hookParameterObj->getFieldVL(sfHookParameterValue); + parameters[hookParameterObj.getFieldVL(sfHookParameterName)] = + hookParameterObj.getFieldVL(sfHookParameterValue); } // and then custom - if (hookObj->isFieldPresent(sfHookParameters)) + if (hookObj.isFieldPresent(sfHookParameters)) { - auto const& hookParameters = hookObj->getFieldArray(sfHookParameters); - for (auto const& hookParameter : hookParameters) + auto const& hookParameters = hookObj.getFieldArray(sfHookParameters); + for (auto const& hookParameterObj : hookParameters) { - auto const& hookParameterObj = dynamic_cast(&hookParameter); - parameters[hookParameterObj->getFieldVL(sfHookParameterName)] = - hookParameterObj->getFieldVL(sfHookParameterValue); + parameters[hookParameterObj.getFieldVL(sfHookParameterName)] = + hookParameterObj.getFieldVL(sfHookParameterValue); } } return false; @@ -5254,12 +5249,11 @@ DEFINE_HOOK_FUNCTION( ripple::STArray const& hooks = hookSLE->getFieldArray(sfHooks); bool found = false; - for (auto const& hook : hooks) + for (auto const& hookObj : hooks) { - auto const& hookObj = dynamic_cast(&hook); - if (hookObj->isFieldPresent(sfHookHash)) + if (hookObj.isFieldPresent(sfHookHash)) { - if (hookObj->getFieldH256(sfHookHash) == hash) + if (hookObj.getFieldH256(sfHookHash) == hash) { found = true; break; diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 897f63c5095..c14c023dd2e 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -216,14 +216,12 @@ calculateHookChainFee( auto const& hooks = hookSLE->getFieldArray(sfHooks); - for (auto const& hook : hooks) + for (auto const& hookObj : hooks) { - ripple::STObject const* hookObj = dynamic_cast(&hook); - - if (!hookObj->isFieldPresent(sfHookHash)) // skip blanks + if (!hookObj.isFieldPresent(sfHookHash)) // skip blanks continue; - uint256 const& hash = hookObj->getFieldH256(sfHookHash); + uint256 const& hash = hookObj.getFieldH256(sfHookHash); std::shared_ptr hookDef = view.read(keylet::hookDefinition(hash)); @@ -236,13 +234,13 @@ calculateHookChainFee( } // check if the hook can fire - uint256 hookOn = (hookObj->isFieldPresent(sfHookOn) - ? hookObj->getFieldH256(sfHookOn) + uint256 hookOn = (hookObj.isFieldPresent(sfHookOn) + ? hookObj.getFieldH256(sfHookOn) : hookDef->getFieldH256(sfHookOn)); uint32_t flags = 0; - if (hookObj->isFieldPresent(sfFlags)) - flags = hookObj->getFieldU32(sfFlags); + if (hookObj.isFieldPresent(sfFlags)) + flags = hookObj.getFieldU32(sfFlags); else flags = hookDef->getFieldU32(sfFlags); @@ -997,17 +995,15 @@ executeHookChain( auto const& hooks = hookSLE->getFieldArray(sfHooks); uint8_t hook_no = 0; - for (auto const& hook : hooks) + for (auto const& hookObj : hooks) { hook_no++; - ripple::STObject const* hookObj = dynamic_cast(&hook); - - if (!hookObj->isFieldPresent(sfHookHash)) // skip blanks + if (!hookObj.isFieldPresent(sfHookHash)) // skip blanks continue; // lookup hook definition - uint256 const& hookHash = hookObj->getFieldH256(sfHookHash); + uint256 const& hookHash = hookObj.getFieldH256(sfHookHash); if (hookSkips.find(hookHash) != hookSkips.end()) { @@ -1025,15 +1021,15 @@ executeHookChain( } // check if the hook can fire - uint256 hookOn = (hookObj->isFieldPresent(sfHookOn) - ? hookObj->getFieldH256(sfHookOn) + uint256 hookOn = (hookObj.isFieldPresent(sfHookOn) + ? hookObj.getFieldH256(sfHookOn) : hookDef->getFieldH256(sfHookOn)); if (!hook::canHook(ctx_.tx.getTxnType(), hookOn)) continue; // skip if it can't - uint32_t flags = (hookObj->isFieldPresent(sfFlags) ? - hookObj->getFieldU32(sfFlags) : hookDef->getFieldU32(sfFlags)); + uint32_t flags = (hookObj.isFieldPresent(sfFlags) ? + hookObj.getFieldU32(sfFlags) : hookDef->getFieldU32(sfFlags)); JLOG(j_.trace()) << "HookChainExecution: " << hookHash @@ -1045,8 +1041,8 @@ executeHookChain( // fetch the namespace either from the hook object of, if absent, the hook def uint256 const& ns = - (hookObj->isFieldPresent(sfHookNamespace) - ? hookObj->getFieldH256(sfHookNamespace) + (hookObj.isFieldPresent(sfHookNamespace) + ? hookObj.getFieldH256(sfHookNamespace) : hookDef->getFieldH256(sfHookNamespace)); // gather parameters @@ -1161,22 +1157,20 @@ Transactor::doHookCallback(std::shared_ptr const& provisionalMet bool found = false; auto const& hooks = hooksCallback->getFieldArray(sfHooks); uint8_t hook_no = 0; - for (auto const& hook : hooks) + for (auto const& hookObj : hooks) { hook_no++; - STObject const* hookObj = dynamic_cast(&hook); - - if (!hookObj->isFieldPresent(sfHookHash)) // skip blanks + if (!hookObj.isFieldPresent(sfHookHash)) // skip blanks continue; - if (hookObj->getFieldH256(sfHookHash) != callbackHookHash) + if (hookObj.getFieldH256(sfHookHash) != callbackHookHash) continue; // fetch the namespace either from the hook object of, if absent, the hook def uint256 const& ns = - (hookObj->isFieldPresent(sfHookNamespace) - ? hookObj->getFieldH256(sfHookNamespace) + (hookObj.isFieldPresent(sfHookNamespace) + ? hookObj.getFieldH256(sfHookNamespace) : hookDef->getFieldH256(sfHookNamespace)); executedHookCount_++; @@ -1433,18 +1427,16 @@ Transactor::doAgainAsWeak( auto const& hooks = hooksArray->getFieldArray(sfHooks); uint8_t hook_no = 0; - for (auto const& hook : hooks) + for (auto const& hookObj : hooks) { hook_no++; - STObject const* hookObj = dynamic_cast(&hook); - - if (!hookObj->isFieldPresent(sfHookHash)) // skip blanks + if (!hookObj.isFieldPresent(sfHookHash)) // skip blanks continue; - uint256 const& hookHash = hookObj->getFieldH256(sfHookHash); + uint256 const& hookHash = hookObj.getFieldH256(sfHookHash); - if (hookHashes.find(hookObj->getFieldH256(sfHookHash)) == hookHashes.end()) + if (hookHashes.find(hookObj.getFieldH256(sfHookHash)) == hookHashes.end()) continue; auto const& hookDef = view().peek(keylet::hookDefinition(hookHash)); @@ -1458,8 +1450,8 @@ Transactor::doAgainAsWeak( // fetch the namespace either from the hook object of, if absent, the hook def uint256 const& ns = - (hookObj->isFieldPresent(sfHookNamespace) - ? hookObj->getFieldH256(sfHookNamespace) + (hookObj.isFieldPresent(sfHookNamespace) + ? hookObj.getFieldH256(sfHookNamespace) : hookDef->getFieldH256(sfHookNamespace)); executedHookCount_++;