ai review nits fixes of host functions#6963
Open
pwang200 wants to merge 2 commits intoXRPLF:ripple/wasmi-host-functionsfrom
Open
ai review nits fixes of host functions#6963pwang200 wants to merge 2 commits intoXRPLF:ripple/wasmi-host-functionsfrom
pwang200 wants to merge 2 commits intoXRPLF:ripple/wasmi-host-functionsfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ripple/wasmi-host-functions #6963 +/- ##
===========================================================
Coverage 82.0% 82.0%
===========================================================
Files 1026 1026
Lines 78199 78199
Branches 7663 7661 -2
===========================================================
+ Hits 64144 64146 +2
+ Misses 14055 14053 -2
🚀 New features to boost your workflow:
|
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(1) Unchecked Expected dereference in isAmendmentEnabled_wrap
Location: src/libxrpl/tx/wasm/HostFuncWrapper.cpp:489
Impact: Low (+1 point)
Confidence: Medium
Category: Code quality / Defensive programming
Description: The code dereferences ret via *ret without checking whether the Expected contains a value. If hf->isAmendmentEnabled(uint256) ever returned an Unexpected (error), *ret would throw bad_expected_access. The current production implementation (WasmHostFunctionsImpl::isAmendmentEnabled) always returns a value (calling rules().enabled() which returns bool), so this cannot trigger today. However, the pattern is inconsistent with the defensive style used elsewhere in the host function wrappers.
Why fix: The current implementation can't return an error, so this is safe today. However, the pattern is genuinely inconsistent with every other host function wrapper in the file, which all check for errors before dereferencing. If someone changes isAmendmentEnabled to return an error in the future, this would crash the node with an uncaught exception. It's a real defensive coding gap worth closing.
Suggested Fix: Add ret && before the *ret == 1 check to guard against future changes that might return an error.
(2) Data race on j_ member before mutex acquisition
Location: src/libxrpl/tx/wasm/WasmiVM.cpp:759 and src/libxrpl/tx/wasm/WasmiVM.cpp:864
Impact: Low (+1 point)
Confidence: High
Category: Code quality / Theoretical UB
Branch: ripple/wasmi-host-functions
Description: Both WasmiEngine::run() and WasmiEngine::check() assign j_ = j BEFORE acquiring the mutex (acquired inside runHlp/checkHlp). Since WasmEngine is a process-wide singleton, concurrent calls create a data race on j_. However, Journal is just a single Sink* pointer — on 64-bit platforms pointer writes are hardware-atomic. Furthermore, j_ is only used for logging and never affects transaction outcomes. The reads of j_ inside the critical section (under mutex) are safe; the only concerning reads are in the catch blocks after the mutex is released, but even there the worst case is logging to a slightly wrong journal. Technically UB per the C++ standard, but practically harmless.
Suggested Fix: Move j_ = j inside runHlp/checkHlp after the mutex is acquired, or pass j as a parameter to eliminate the shared state.
Note — pattern comparison with rest of codebase: This is another area where WasmiEngine diverges from the established rippled pattern. In all normal transactors, beast::Journal is handled as:
PreflightContext, PreclaimContext: beast::Journal const j — a const member, passed as parameter, per-context instance
Transactor: beast::Journal const j_ — a const member, initialized in constructor, per-instance
Every normal transactor gets its own instance with its own journal. There is no shared mutable journal anywhere in the transaction processing pipeline. WasmiEngine is the only place in the tx processing code where j_ is a mutable member on a singleton, overwritten on every call. The fix aligns with the rest of the codebase: pass j as a parameter through the call chain rather than storing it on shared state.