From ce2586c0395301d094b7d7cf51b491f64e10c451 Mon Sep 17 00:00:00 2001 From: Olek <115580134+oleks-rip@users.noreply.github.com> Date: Mon, 20 Apr 2026 14:03:39 -0400 Subject: [PATCH] Review fixes (#6512) --- include/xrpl/tx/wasm/HostFuncImpl.h | 2 +- include/xrpl/tx/wasm/ParamsHelper.h | 31 +--------- include/xrpl/tx/wasm/WasmVM.h | 24 ++++---- include/xrpl/tx/wasm/WasmiVM.h | 10 +++- src/libxrpl/tx/wasm/HostFuncImplGetter.cpp | 56 ++++++++--------- src/libxrpl/tx/wasm/HostFuncWrapper.cpp | 70 +++++++++++++++++++--- src/libxrpl/tx/wasm/WasmVM.cpp | 3 - src/libxrpl/tx/wasm/WasmiVM.cpp | 2 +- 8 files changed, 111 insertions(+), 87 deletions(-) diff --git a/include/xrpl/tx/wasm/HostFuncImpl.h b/include/xrpl/tx/wasm/HostFuncImpl.h index 30ae1720a9..a8d8e2da64 100644 --- a/include/xrpl/tx/wasm/HostFuncImpl.h +++ b/include/xrpl/tx/wasm/HostFuncImpl.h @@ -81,7 +81,7 @@ public: checkSelf() const override { return !currentLedgerObj_ && !data_ && - std::ranges::find_if(cache_, [](auto& p) { return !!p; }) == cache_.end(); + std::ranges::none_of(cache_, [](auto const& p) { return !!p; }); } std::optional const& diff --git a/include/xrpl/tx/wasm/ParamsHelper.h b/include/xrpl/tx/wasm/ParamsHelper.h index 82863e730a..d48663ef23 100644 --- a/include/xrpl/tx/wasm/ParamsHelper.h +++ b/include/xrpl/tx/wasm/ParamsHelper.h @@ -48,7 +48,7 @@ struct WasmRuntimeWrapper //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// -enum WasmTypes { WT_I32, WT_I64, WT_U8V }; +enum WasmTypes { WT_I32, WT_I64 }; struct WasmImportFunc { @@ -142,22 +142,15 @@ WasmImpFunc( //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// -struct WasmParamVec -{ - std::uint8_t const* d = nullptr; - std::int32_t sz = 0; -}; - struct WasmParam { + // We are not supporting float/double + WasmTypes type = WT_I32; union { std::int32_t i32; std::int64_t i64 = 0; - float f32; - double f64; - WasmParamVec u8v; } of; }; @@ -177,24 +170,6 @@ wasmParamsHlp(std::vector& v, std::int64_t p, Types&&... args) wasmParamsHlp(v, std::forward(args)...); } -// We are not supporting float/double for now -// Leaving this code here so that it is easier to add later if needed -// template -// inline void -// wasmParamsHlp(std::vector& v, float p, Types&&... args) -// { -// v.push_back({.type = WT_F32, .of = {.f32 = p}}); -// wasmParamsHlp(v, std::forward(args)...); -// } - -// template -// inline void -// wasmParamsHlp(std::vector& v, double p, Types&&... args) -// { -// v.push_back({.type = WT_F64, .of = {.f64 = p}}); -// wasmParamsHlp(v, std::forward(args)...); -// } - inline void wasmParamsHlp(std::vector& v) { diff --git a/include/xrpl/tx/wasm/WasmVM.h b/include/xrpl/tx/wasm/WasmVM.h index b872adfc95..e001ee6112 100644 --- a/include/xrpl/tx/wasm/WasmVM.h +++ b/include/xrpl/tx/wasm/WasmVM.h @@ -6,19 +6,19 @@ namespace xrpl { -static std::string_view const W_ENV = "env"; -static std::string_view const W_HOST_LIB = "host_lib"; -static std::string_view const W_MEM = "memory"; -static std::string_view const W_STORE = "store"; -static std::string_view const W_LOAD = "load"; -static std::string_view const W_SIZE = "size"; -static std::string_view const W_ALLOC = "allocate"; -static std::string_view const W_DEALLOC = "deallocate"; -static std::string_view const W_PROC_EXIT = "proc_exit"; +std::string_view inline constexpr W_ENV = "env"; +std::string_view inline constexpr W_HOST_LIB = "host_lib"; +std::string_view inline constexpr W_MEM = "memory"; +std::string_view inline constexpr W_STORE = "store"; +std::string_view inline constexpr W_LOAD = "load"; +std::string_view inline constexpr W_SIZE = "size"; +std::string_view inline constexpr W_ALLOC = "allocate"; +std::string_view inline constexpr W_DEALLOC = "deallocate"; +std::string_view inline constexpr W_PROC_EXIT = "proc_exit"; -static std::string_view const ESCROW_FUNCTION_NAME = "finish"; +std::string_view inline constexpr ESCROW_FUNCTION_NAME = "finish"; -uint32_t const MAX_PAGES = 128; // 8MB = 64KB*128 +uint32_t inline constexpr MAX_PAGES = 128; // 8MB = 64KB*128 class WasmiEngine; @@ -36,8 +36,6 @@ class WasmEngine operator=(WasmEngine&&) = delete; public: - ~WasmEngine() = default; - static WasmEngine& instance(); diff --git a/include/xrpl/tx/wasm/WasmiVM.h b/include/xrpl/tx/wasm/WasmiVM.h index e74cbb62c3..cb56e64026 100644 --- a/include/xrpl/tx/wasm/WasmiVM.h +++ b/include/xrpl/tx/wasm/WasmiVM.h @@ -125,6 +125,7 @@ struct WasmiResult { } + WasmiResult() = delete; ~WasmiResult() = default; WasmiResult(WasmiResult&& o) = default; WasmiResult& @@ -158,14 +159,17 @@ private: public: InstanceWrapper(); + InstanceWrapper(InstanceWrapper const&) = delete; + InstanceWrapper(InstanceWrapper&& o); + InstanceWrapper(StorePtr& s, ModulePtr& m, WasmExternVec const& imports, beast::Journal j); + InstanceWrapper& operator=(InstanceWrapper&& o); - InstanceWrapper(StorePtr& s, ModulePtr& m, WasmExternVec const& imports, beast::Journal j); - - ~InstanceWrapper() = default; + InstanceWrapper& + operator=(InstanceWrapper const&) = delete; operator bool() const; diff --git a/src/libxrpl/tx/wasm/HostFuncImplGetter.cpp b/src/libxrpl/tx/wasm/HostFuncImplGetter.cpp index 7b175845ce..0053af3400 100644 --- a/src/libxrpl/tx/wasm/HostFuncImplGetter.cpp +++ b/src/libxrpl/tx/wasm/HostFuncImplGetter.cpp @@ -6,19 +6,17 @@ namespace xrpl { typedef std::variant FieldValue; -namespace detail { - template Bytes getIntBytes(STBase const* obj) { - static_assert(std::is_integral::value, "Only integral types"); + static_assert(std::is_integral_v, "Only integral types"); + XRPL_ASSERT(obj, "getIntBytes null pointer"); - auto const& num(static_cast const*>(obj)); // NOLINT + auto const* num(static_cast const*>(obj)); // NOLINT T const data = adjustWasmEndianess(num->value()); - auto const* b = reinterpret_cast(&data); - auto const* e = reinterpret_cast(&data + 1); - return Bytes{b, e}; + uint8_t const* b = reinterpret_cast(&data); + return Bytes{b, b + sizeof(T)}; } static Expected @@ -217,8 +215,6 @@ getArrayLen(FieldValue const& variantField) return Unexpected(HostFunctionError::NO_ARRAY); // LCOV_EXCL_LINE } -} // namespace detail - Expected WasmHostFunctionsImpl::cacheLedgerObj(uint256 const& objId, int32_t cacheIdx) { @@ -253,7 +249,7 @@ WasmHostFunctionsImpl::cacheLedgerObj(uint256 const& objId, int32_t cacheIdx) Expected WasmHostFunctionsImpl::getTxField(SField const& fname) const { - return detail::getAnyFieldData(ctx_.tx.peekAtPField(fname)); + return getAnyFieldData(ctx_.tx.peekAtPField(fname)); } Expected @@ -262,7 +258,7 @@ WasmHostFunctionsImpl::getCurrentLedgerObjField(SField const& fname) const auto const sle = getCurrentLedgerObj(); if (!sle.has_value()) return Unexpected(sle.error()); - return detail::getAnyFieldData(sle.value()->peekAtPField(fname)); + return getAnyFieldData(sle.value()->peekAtPField(fname)); } Expected @@ -271,7 +267,7 @@ WasmHostFunctionsImpl::getLedgerObjField(int32_t cacheIdx, SField const& fname) auto const normalizedIdx = normalizeCacheIndex(cacheIdx); if (!normalizedIdx.has_value()) return Unexpected(normalizedIdx.error()); - return detail::getAnyFieldData(cache_[normalizedIdx.value()]->peekAtPField(fname)); + return getAnyFieldData(cache_[normalizedIdx.value()]->peekAtPField(fname)); } // Subsection: nested getters @@ -279,11 +275,11 @@ WasmHostFunctionsImpl::getLedgerObjField(int32_t cacheIdx, SField const& fname) Expected WasmHostFunctionsImpl::getTxNestedField(Slice const& locator) const { - auto const r = detail::locateField(ctx_.tx, locator); + auto const r = locateField(ctx_.tx, locator); if (!r) return Unexpected(r.error()); - return detail::getAnyFieldData(r.value()); + return getAnyFieldData(r.value()); } Expected @@ -293,11 +289,11 @@ WasmHostFunctionsImpl::getCurrentLedgerObjNestedField(Slice const& locator) cons if (!sle.has_value()) return Unexpected(sle.error()); - auto const r = detail::locateField(*sle.value(), locator); + auto const r = locateField(*sle.value(), locator); if (!r) return Unexpected(r.error()); - return detail::getAnyFieldData(r.value()); + return getAnyFieldData(r.value()); } Expected @@ -307,11 +303,11 @@ WasmHostFunctionsImpl::getLedgerObjNestedField(int32_t cacheIdx, Slice const& lo if (!normalizedIdx.has_value()) return Unexpected(normalizedIdx.error()); - auto const r = detail::locateField(*cache_[normalizedIdx.value()], locator); + auto const r = locateField(*cache_[normalizedIdx.value()], locator); if (!r) return Unexpected(r.error()); - return detail::getAnyFieldData(r.value()); + return getAnyFieldData(r.value()); } // Subsection: array length getters @@ -323,10 +319,10 @@ WasmHostFunctionsImpl::getTxArrayLen(SField const& fname) const return Unexpected(HostFunctionError::NO_ARRAY); auto const* field = ctx_.tx.peekAtPField(fname); - if (detail::noField(field)) + if (noField(field)) return Unexpected(HostFunctionError::FIELD_NOT_FOUND); - return detail::getArrayLen(field); + return getArrayLen(field); } Expected @@ -340,10 +336,10 @@ WasmHostFunctionsImpl::getCurrentLedgerObjArrayLen(SField const& fname) const return Unexpected(sle.error()); auto const* field = sle.value()->peekAtPField(fname); - if (detail::noField(field)) + if (noField(field)) return Unexpected(HostFunctionError::FIELD_NOT_FOUND); - return detail::getArrayLen(field); + return getArrayLen(field); } Expected @@ -357,10 +353,10 @@ WasmHostFunctionsImpl::getLedgerObjArrayLen(int32_t cacheIdx, SField const& fnam return Unexpected(normalizedIdx.error()); auto const* field = cache_[normalizedIdx.value()]->peekAtPField(fname); - if (detail::noField(field)) + if (noField(field)) return Unexpected(HostFunctionError::FIELD_NOT_FOUND); - return detail::getArrayLen(field); + return getArrayLen(field); } // Subsection: nested array length getters @@ -368,12 +364,12 @@ WasmHostFunctionsImpl::getLedgerObjArrayLen(int32_t cacheIdx, SField const& fnam Expected WasmHostFunctionsImpl::getTxNestedArrayLen(Slice const& locator) const { - auto const r = detail::locateField(ctx_.tx, locator); + auto const r = locateField(ctx_.tx, locator); if (!r) return Unexpected(r.error()); auto const& field = r.value(); - return detail::getArrayLen(field); + return getArrayLen(field); } Expected @@ -382,12 +378,12 @@ WasmHostFunctionsImpl::getCurrentLedgerObjNestedArrayLen(Slice const& locator) c auto const sle = getCurrentLedgerObj(); if (!sle.has_value()) return Unexpected(sle.error()); - auto const r = detail::locateField(*sle.value(), locator); + auto const r = locateField(*sle.value(), locator); if (!r) return Unexpected(r.error()); auto const& field = r.value(); - return detail::getArrayLen(field); + return getArrayLen(field); } Expected @@ -397,12 +393,12 @@ WasmHostFunctionsImpl::getLedgerObjNestedArrayLen(int32_t cacheIdx, Slice const& if (!normalizedIdx.has_value()) return Unexpected(normalizedIdx.error()); - auto const r = detail::locateField(*cache_[normalizedIdx.value()], locator); + auto const r = locateField(*cache_[normalizedIdx.value()], locator); if (!r) return Unexpected(r.error()); auto const& field = r.value(); - return detail::getArrayLen(field); + return getArrayLen(field); } } // namespace xrpl diff --git a/src/libxrpl/tx/wasm/HostFuncWrapper.cpp b/src/libxrpl/tx/wasm/HostFuncWrapper.cpp index 246b9ab4a3..460d19cd43 100644 --- a/src/libxrpl/tx/wasm/HostFuncWrapper.cpp +++ b/src/libxrpl/tx/wasm/HostFuncWrapper.cpp @@ -10,6 +10,8 @@ #include #include +#include + namespace xrpl { using SFieldCRef = std::reference_wrapper; @@ -486,10 +488,9 @@ isAmendmentEnabled_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* if (slice->size() == uint256::bytes) { - if (auto ret = hf->isAmendmentEnabled(uint256::fromVoid(slice->data())); *ret == 1) - { + if (auto const ret = hf->isAmendmentEnabled(uint256::fromVoid(slice->data())); + ret && *ret == 1) return returnResult(runtime, params, results, ret, index); - } } if (slice->size() > 64) @@ -1435,7 +1436,13 @@ trace_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* results) auto* runtime = reinterpret_cast(hf->getRT()); int index = 0; - if (params->data[1].of.i32 + params->data[3].of.i32 > maxWasmParamLength) + int64_t const len = (int64_t)params->data[1].of.i32 + params->data[3].of.i32; + if (len < 0) + { + return hfResult(results, HostFunctionError::INVALID_PARAMS); + } + + if (std::cmp_greater(len, maxWasmParamLength)) { return hfResult(results, HostFunctionError::DATA_FIELD_TOO_LARGE); } @@ -1457,6 +1464,7 @@ trace_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* results) { return hfResult(results, asHex.error()); // LCOV_EXCL_LINE } + if (*asHex != 0 && *asHex != 1) { return hfResult(results, HostFunctionError::INVALID_PARAMS); @@ -1473,7 +1481,14 @@ traceNum_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* results) auto* hf = getHF(env); auto* runtime = reinterpret_cast(hf->getRT()); int index = 0; - if (params->data[1].of.i32 > maxWasmParamLength) + + int32_t const len = params->data[1].of.i32; + if (len < 0) + { + return hfResult(results, HostFunctionError::INVALID_PARAMS); + } + + if (std::cmp_greater(len, maxWasmParamLength)) { return hfResult(results, HostFunctionError::DATA_FIELD_TOO_LARGE); } @@ -1501,17 +1516,29 @@ traceAccount_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* resul auto* hf = getHF(env); auto* runtime = reinterpret_cast(hf->getRT()); - if (params->data[1].of.i32 > maxWasmParamLength) + int32_t const len = params->data[1].of.i32; + if (len < 0) + { + return hfResult(results, HostFunctionError::INVALID_PARAMS); + } + + if (std::cmp_greater(len, maxWasmParamLength)) + { return hfResult(results, HostFunctionError::DATA_FIELD_TOO_LARGE); + } int i = 0; auto const msg = getDataString(runtime, params, i); if (!msg) + { return hfResult(results, msg.error()); + } auto const account = getDataAccountID(runtime, params, i); if (!account) + { return hfResult(results, account.error()); + } return returnResult(runtime, params, results, hf->traceAccount(*msg, *account), i); } @@ -1524,17 +1551,29 @@ traceFloat_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* results auto* hf = getHF(env); auto* runtime = reinterpret_cast(hf->getRT()); - if (params->data[1].of.i32 > maxWasmParamLength) + int32_t const len = params->data[1].of.i32; + if (len < 0) + { + return hfResult(results, HostFunctionError::INVALID_PARAMS); + } + + if (std::cmp_greater(len, maxWasmParamLength)) + { return hfResult(results, HostFunctionError::DATA_FIELD_TOO_LARGE); + } int i = 0; auto const msg = getDataString(runtime, params, i); if (!msg) + { return hfResult(results, msg.error()); + } auto const number = getDataSlice(runtime, params, i); if (!number) + { return hfResult(results, number.error()); + } return returnResult(runtime, params, results, hf->traceFloat(*msg, *number), i); } @@ -1547,17 +1586,29 @@ traceAmount_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* result auto* hf = getHF(env); auto* runtime = reinterpret_cast(hf->getRT()); - if (params->data[1].of.i32 > maxWasmParamLength) + int32_t const len = params->data[1].of.i32; + if (len < 0) + { + return hfResult(results, HostFunctionError::INVALID_PARAMS); + } + + if (std::cmp_greater(len, maxWasmParamLength)) + { return hfResult(results, HostFunctionError::DATA_FIELD_TOO_LARGE); + } int i = 0; auto const msg = getDataString(runtime, params, i); if (!msg) + { return hfResult(results, msg.error()); + } auto const amountSliceOpt = getDataSlice(runtime, params, i); if (!amountSliceOpt) + { return hfResult(results, amountSliceOpt.error()); + } auto const amountSlice = amountSliceOpt.value(); auto serialIter = SerialIter(amountSlice); @@ -1571,8 +1622,11 @@ traceAmount_wrap(void* env, wasm_val_vec_t const* params, wasm_val_vec_t* result { amount = std::nullopt; } + if (!amount) + { return hfResult(results, HostFunctionError::INVALID_PARAMS); + } return returnResult(runtime, params, results, hf->traceAmount(*msg, *amount), i); } diff --git a/src/libxrpl/tx/wasm/WasmVM.cpp b/src/libxrpl/tx/wasm/WasmVM.cpp index fdd9a437d7..d91e6832da 100644 --- a/src/libxrpl/tx/wasm/WasmVM.cpp +++ b/src/libxrpl/tx/wasm/WasmVM.cpp @@ -120,9 +120,6 @@ runEscrowWasm( auto const ret = vm.run(wasmCode, hfs, gasLimit, funcName, params, createWasmImport(hfs), hfs.getJournal()); - // std::cout << "runEscrowWasm, mod size: " << wasmCode.size() - // << ", gasLimit: " << gasLimit << ", funcName: " << funcName; - if (!ret) { #ifdef DEBUG_OUTPUT diff --git a/src/libxrpl/tx/wasm/WasmiVM.cpp b/src/libxrpl/tx/wasm/WasmiVM.cpp index adc0953a38..4a19be4a60 100644 --- a/src/libxrpl/tx/wasm/WasmiVM.cpp +++ b/src/libxrpl/tx/wasm/WasmiVM.cpp @@ -895,7 +895,7 @@ WasmiEngine::checkHlp( // Create and instantiate the module. if (wasmCode.empty()) - throw std::runtime_error("empty nodule"); + throw std::runtime_error("empty module"); int const m = addModule(wasmCode, false, imports, -1); if ((m < 0) || !moduleWrap_)