Problem: Before #36032, a partially parsed UniValue (with a valid output key but no value) reached a checked lookup and threw an exception.
We changed this to unchecked indexing there, which aborted the rpc fuzz target instead of throwing.
The triggering qa-assets input landed after the PR's CI so it only failed post-merge.
Fix: Reject mismatched output key and value counts before indexing them, with additional unit coverage for malformed and duplicate outputs.
`ParseOutputs()` reads transaction outputs from the parallel key and value vectors stored by `UniValue`.
A failed parse can leave an object partially populated, which the RPC fuzz target passes to this code.
Reject objects whose key and value counts differ before iterating over them.
Add unit coverage for the mismatch and the existing duplicate-output errors.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36086.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process. A summary of reviews will appear here.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
106 | @@ -107,6 +107,7 @@ std::vector<std::pair<CTxDestination, CAmount>> ParseOutputs(const UniValue& out 107 | bool has_data{false}; 108 | const auto& keys{outputs.getKeys()}; 109 | const auto& values{outputs.getValues()}; 110 | + if (keys.size() != values.size()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output key and value counts do not match");
nit: Since the line is long and the style in the function is to use multiple lines for other errors:
if (keys.size() != values.size()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output key and value counts do not match");
}
Seems fine to add this, but I am generally not a fan of having to fix classes of bugs one-by-one in the code manually in every instance. Those instances are basically impossible to enumerate or review.
It would be better to just avoid the whole class of bugs wholesale in one go.
Also, this is a fuzz-test-only code path, so it seems a bit odd to mix test-only code with real code.
Alternative in https://github.com/bitcoin/bitcoin/pull/36088
677 | @@ -676,4 +678,21 @@ BOOST_AUTO_TEST_CASE(rpc_arg_helper) 678 | CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_positional); 679 | } 680 | 681 | +BOOST_AUTO_TEST_CASE(parse_outputs_invalid) 682 | +{ 683 | + UniValue mismatched; 684 | + BOOST_REQUIRE(!mismatched.read(R"({"":)"));
It seems like the actual RPC subsystem does not allow invalid JSON through: https://github.com/bitcoin/bitcoin/blob/031175197f1b7f90397b838a381f0892a74ca62a/src/httprpc.cpp#L233-L237 This has me think that it is the rpc fuzz target which should be patched once instead of having to harden all RPC implementations against invalid JSON just to serve the fuzz target rather than the real use-case.
To me #36088 fixes this at the right level, in UniValue, rather than hardening each caller.
Ran this one with #36088 on top. The failed read() now leaves null rather than a mismatched object, so ParseOutputs throws type_error from getKeys() before the new check, and the first assertion in parse_outputs_invalid dies on it.
The other two pass with the first one dropped, and they add unit coverage for paths only the functional tests reach today.
