This is a follow up for #28113.
The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this comment.
Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this comment.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, MarcoFalke, jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
68 | @@ -69,6 +69,7 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue) 69 | try { 70 | (void)ParseSighashString(univalue); 71 | } catch (const UniValue&) { 72 | + } catch (const std::runtime_error&) { 73 | }
I don't think catching runtime errors here is the best approach. We never expect this to fail at runtime, only if the function is used the wrong way by a programmer. Will this not unnecessarily hide true bugs?
I think avoiding dead code makes sense, but I also like the function being self-contained and self-descriptive like this without too much overhead, so I guess overall I'm ~0 on the change.
As an alternative, how about instead of catching std::runtime_error&, we only call ParseSighashString if univalue is str? This way we make the assumptions explicit and self-documenting, too. Would also prefer adding a docstring to ParseSighasString documenting the input assumptions (potentially with "@pre"). As a public function, it's quite easy for a programmer to use this in the wrong way if not careful. I'm not too familiar with fuzzing best practices though, so maybe this has other (bigger) downsides?
try {
if (univalue.isNull() || univalue.isStr()) (void)ParseSighashString(univalue);
} catch (const UniValue&) {
}
Approach ACK on the release notes updates (nice catch MarcoFalke), not so sure about the code changes yet.
2 | @@ -3,5 +3,5 @@ RPC Wallet 3 | 4 | - The `signrawtransactionwithkey`, `signrawtransactionwithwallet`, 5 | `walletprocesspsbt` and `descriptorprocesspsbt` calls now return more 6 | - specific RPC_INVALID_PARAMETER instead of RPC_PARSE_ERROR if their 7 | - sighashtype argument is malformed or not a string. 8 | + specific RPC_INVALID_PARAMETER instead of RPC_MISC_ERROR if their
suggestion: s/now return more specific RPC_INVALID_PARAMETER/now raise the more specific RPC_INVALID_PARAMETER error/
It's a JSON RPC response, returning seems correct to me.
if you are set on "return", then
s/return more specific RPC_INVALID_PARAMETER/return the more specific RPC_INVALID_PARAMETER error/
I'm not set on it, it's just the language the JSON RPC spec uses. Otherwise I'm okay with your suggestion.
Updated cc266f9ccd669866ee3121c8c03e08cd836ffba6 -> 06199a995f20c55583f6948cfe99e608679fcdf1 (kernelRmUnivalueFollowup_0 -> kernelRmUnivalueFollowup_1, compare)
66 | @@ -67,8 +67,9 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue) 67 | } catch (const std::runtime_error&) { 68 | } 69 | try { 70 | - (void)ParseSighashString(univalue); 71 | + if (univalue.isNull() || univalue.isStr()) (void)ParseSighashString(univalue); 72 | } catch (const UniValue&) { 73 | + } catch (const std::runtime_error&) {
I think you forgot to remove this?
Whoops :(
This check is already done by the rpc parser. Re-doing it is adding dead
code. Instead, throwing an exception when the assumption does not hold
is the already correct behavior.
To make the fuzz test more accurate and not swallow all runtime errors,
add a check that the passed in UniValue sighash argument is either a
string or null.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
ACK 06199a995f20c55583f6948cfe99e608679fcdf1
lgtm ACK 06199a995f20c55583f6948cfe99e608679fcdf1
66 | @@ -67,7 +67,7 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue) 67 | } catch (const std::runtime_error&) { 68 | } 69 | try { 70 | - (void)ParseSighashString(univalue); 71 | + if (univalue.isNull() || univalue.isStr()) (void)ParseSighashString(univalue);
Ooh actually, would this be better? Equally clear (I think), but we still feed it unexpected types.
diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp
index a3d6ab6375..df3410eab8 100644
--- a/src/test/fuzz/parse_univalue.cpp
+++ b/src/test/fuzz/parse_univalue.cpp
@@ -67,8 +67,10 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
} catch (const std::runtime_error&) {
}
try {
- if (univalue.isNull() || univalue.isStr()) (void)ParseSighashString(univalue);
+ (void)ParseSighashString(univalue);
} catch (const UniValue&) {
+ } catch (std::runtime_error&) {
+ if(univalue.isNull() || univalue.isStr()) throw; //a non-null, non-string univalue is expected to throw on .get_str()
}
try {
(void)AmountFromValue(univalue);
Might also want to catch it NOT throwing an error when it ought to.
Tested ACK 06199a995f20c55583f6948cfe99e608679fcdf1
Verified relevant tests still pass after cherry-picking 647d95a from #28166 to this branch.
I don't mind re-ACKing if you like the suggestion in #28162 (review) (mind the spaces on the last line of its diff or run clang-format on it).
Milestone
26.0