Handle invalid hex encoding in ParseHex #25227
pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2205-reject-non-hex-🌲 changing 3 files +44 −11-
maflcko commented at 1:49 pm on May 27, 2022: memberSeems a bit confusing to happily accept random bytes and pretend they are hex encoded strings.
-
maflcko force-pushed on May 27, 2022
-
maflcko commented at 2:11 pm on May 27, 2022: memberLooks like this may be used by salvage wallet, but I am not sure if it is worth it to keep for that?
-
DrahtBot added the label Utils/log/libs on May 27, 2022
-
sipa commented at 4:04 pm on May 27, 2022: member
I don’t think the distinction matters for salvage; it looks like it’s bdb producing the hex dump output lines at runtime, so reading anything but hex data from it would imply buggy/incorrect use of bdb in the first place.
More generally… what about instead moving to a model where this hex parsing returns an
std::optional<std::vector<std::byte>>
, so callers are free to distinguish invalid input from empty input? -
laanwj commented at 6:26 pm on May 27, 2022: member
Concept ACK.
But if we’re going to change this I’d prefer to return an
optional<>
so that the empty vector can be distinguished from the error condition. -
DrahtBot commented at 3:40 pm on May 28, 2022: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
Type Reviewers ACK stickies-v Concept ACK laanwj, vincenzopalazzo Stale ACK pinheadmz If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
-
vincenzopalazzo commented at 6:04 pm on May 28, 2022: noneConcept ACK, and I also vote for the optional parameter, that is already used in the code base and we can try to unify the paradigm used
-
maflcko renamed this:
Return empty vector on invalid hex encoding
Return nullopt on invalid hex encoding
on May 30, 2022 -
maflcko renamed this:
Return nullopt on invalid hex encoding
Handle invalid hex encoding in ParseHex
on May 30, 2022 -
maflcko force-pushed on May 30, 2022
-
maflcko marked this as a draft on May 30, 2022
-
DrahtBot added the label Needs rebase on Jul 7, 2022
-
maflcko force-pushed on Dec 13, 2022
-
DrahtBot removed the label Needs rebase on Dec 13, 2022
-
maflcko commented at 6:45 pm on December 13, 2022: member
Ok, rebased and added a new function for
optional
. The old function remains an alias, with the fallback to an empty vector. This avoid having to change all existing code and making it needlessly verbose, because it is already properly handling empty vectors.0#include <array> 1#include <cstddef> 2#include <string_view> 3 4#define FromHex(chars_lit) \ 5 ([&]() { \ 6 using namespace std::literals; \ 7 constexpr std::string_view hex_str{chars_lit##sv}; \ 8 static_assert(IsHex(hex_str)); \ 9 std::array<std::byte, hex_str.size() / 2> b{}; \ 10 auto it = hex_str.begin(); \ 11 for (auto& i : b) { \ 12 auto c1 = HexDigit(*(it++)); \ 13 auto c2 = HexDigit(*(it++)); \ 14 i = std::byte(c1 << 4) | std::byte(c2); \ 15 } \ 16 \ 17 return b; \ 18 }())
Use:
0constexpr auto a{FromHex("ffaa")}; // OK 1constexpr auto a{FromHex("xxYx")}; // compile failure
-
maflcko marked this as ready for review on Dec 13, 2022
-
maflcko force-pushed on Jan 3, 2023
-
maflcko force-pushed on Jan 3, 2023
-
in src/test/util_tests.cpp:143 in fa6ec61d0c outdated
139@@ -140,26 +140,39 @@ BOOST_AUTO_TEST_CASE(parse_hex) 140 // Basic test vector 141 result = ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"); 142 BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end()); 143+ result = TryParseHex<uint8_t>("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f").value();
kiminuo commented at 9:43 am on January 8, 2023:Not sure if it is relevant here but I tried to check what happens when
My first guess was that
std::nullopt
should be returned:0// Single space should be handled correctly 1BOOST_CHECK(!TryParseHex<uint8_t>(" ").has_value());
but actually what happens is:
0// Single space should be handled correctly 1result = TryParseHex<uint8_t>(" ").value(); 2BOOST_CHECK_EQUAL(result.size(), 0);
Maybe it’s worth adding this test to document expected behavior.
maflcko commented at 2:25 pm on January 9, 2023:Thanks, donemaflcko force-pushed on Jan 9, 2023fanquake requested review from stickies-v on Feb 17, 2023fanquake requested review from john-moffett on Feb 17, 2023stickies-v commented at 10:32 am on February 23, 2023: contributorConcept ACKfanquake requested review from pinheadmz on Feb 23, 2023pinheadmz approvedpinheadmz commented at 6:39 pm on February 24, 2023: memberACK fad0c892c34c30cf8f50e832425210e24d45837e
Built and ran all tests, reviewed code. I also inserted a few other test cases but nothing broke so, take em or leave em:
Ff aA
(mixed case / multiple spaces, succeeds)F F
(spaces between nibbles, fails as expected)0-----BEGIN PGP SIGNED MESSAGE----- 1Hash: SHA256 2 3ACK fad0c892c34c30cf8f50e832425210e24d45837e 4-----BEGIN PGP SIGNATURE----- 5 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmP5A/QACgkQ5+KYS2KJ 7yToaJw/+OHwTtj30d86GvVcGf4v7Hz0nVl+qVI0/BNE+xEcVEOkfK47yYoIIuJ40 8uWiW7ts9s8ERQ6o60nVRBm2V8yEjjbD1kJf4+qJiWIEVvIkgzXrshziqy8RGDeaj 9yMpdSEwRmysnWQ4/hk6vVJ3+bn3dQfKCv7QA/CEEwcWQh148jZxPzSZwAuaU7dWH 10uzmA+P3AYFYJ+jfOVlahWnNVlmFDedJixXV6qwEyBZbpC9oUwIgb2pMcbalojktS 11G1IafwHMl0dvkhxVRlwKQK1I1tjoqaAT7Dfv/Y3wJaW4rjewVVE/Vs5iAheUmpKI 12bSsGGCwmiv+leyBpWltwyYDklzOt2ITysVcVjpNVE4TmfcFK3z3b30rdQGvGYc2Q 13UPG6GjOOZ1Kvb07oy0DcICbNkLTg5r4ZEqCRZFn9+o/Eu90EjHBD9PzvNAzk4yw0 14y5GinmmwmBHXNCHBYbU+ZrjrYJSmAjOORoOrYin7Gt6BQ8nIlA8f8LurTmj9/RJp 15Fj6vrfk8EjOmyPFoxzVOC7woirK8PMF8EU9yJIebaIzX2PzjSEXiTOhOV/BJs1bL 165WhjCHuqYdHWKS609twwvpEdQXFquThcLlmUATI+0jNt5gsegxdkF39BYQpr95B2 17hMS/yJb18ywl4HOa/Bk4HyRFekI+EUbkeFxwlruerwxzzKtKUQE= 18=qi0Q 19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
in src/test/util_tests.cpp:178 in fad0c892c3 outdated
177 result = ParseHex("1234 invalid 1234"); 178- BOOST_CHECK(result.size() == 2 && result[0] == 0x12 && result[1] == 0x34); 179+ BOOST_CHECK_EQUAL(result.size(), 0); 180+ BOOST_CHECK(!TryParseHex("1234 invalid 1234").has_value()); 181+ 182+ // Truncated input
stickies-v commented at 10:23 am on February 27, 2023:nit: add description of expected behaviour (and in 0000509239d4e699f57b392531f242ad6933c982 it would be “Truncated input is ignored”)
0 // Truncated input makes the parsing fail entirely
maflcko commented at 12:43 pm on February 27, 2023:thanks, donein src/test/util_tests.cpp:170 in fad0c892c3 outdated
167 " \0 " 168 " 22 "s}; 169 BOOST_CHECK_EQUAL(with_embedded_null.size(), 11); 170 result = ParseHex(with_embedded_null); 171- BOOST_CHECK(result.size() == 1 && result[0] == 0x11); 172+ BOOST_CHECK_EQUAL(result.size(), 0);
stickies-v commented at 10:42 am on February 27, 2023:nit: for all these, would prefer having consistency between using a temporary var for the
ParseHex
andTryParseHex
tests0 BOOST_CHECK_EQUAL(ParseHex(with_embedded_null).size(), 0);
(edit: updated
ParseHex(with_embedded_null)
->ParseHex(with_embedded_null).size()
)
maflcko commented at 12:05 pm on February 27, 2023:Can you explain this?
Currently it is consistent in that a all cases use
result = ParseHex(...)
. This is the case before this pull request and not changed.
maflcko commented at 12:06 pm on February 27, 2023:(Also, your suggestion wouldn’t compile as is, I am pretty sure)
stickies-v commented at 12:13 pm on February 27, 2023:The below diff captures my entire suggestion (and compiles/tests). In these locations, we use a temp var for
ParseHex
but not forTryParseHex
. It’s just a style/minor thing (hence the nit), but I think having consistency between both makes it easier for the reviewer to see the similarities between both tests.0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp 1index 65468a113..74e151b99 100644 2--- a/src/test/util_tests.cpp 3+++ b/src/test/util_tests.cpp 4@@ -166,18 +166,15 @@ BOOST_AUTO_TEST_CASE(parse_hex) 5 " \0 " 6 " 22 "s}; 7 BOOST_CHECK_EQUAL(with_embedded_null.size(), 11); 8- result = ParseHex(with_embedded_null); 9- BOOST_CHECK_EQUAL(result.size(), 0); 10+ BOOST_CHECK_EQUAL(ParseHex(with_embedded_null).size(), 0); 11 BOOST_CHECK(!TryParseHex(with_embedded_null).has_value()); 12 13 // Reject parsing if invalid value 14- result = ParseHex("1234 invalid 1234"); 15- BOOST_CHECK_EQUAL(result.size(), 0); 16+ BOOST_CHECK_EQUAL(ParseHex("1234 invalid 1234").size(), 0); 17 BOOST_CHECK(!TryParseHex("1234 invalid 1234").has_value()); 18 19 // Truncated input 20- result = ParseHex("12 3"); 21- BOOST_CHECK_EQUAL(result.size(), 0); 22+ BOOST_CHECK_EQUAL(ParseHex("12 3").size(), 0); 23 BOOST_CHECK(!TryParseHex("12 3").has_value()); 24 }
maflcko commented at 12:43 pm on February 27, 2023:Thanks, done.stickies-v approvedstickies-v commented at 12:01 pm on February 27, 2023: contributorACK fad0c892c34c30cf8f50e832425210e24d45837e
An API that fails on invalid input instead of accepting the partial valid part is more robust imo. I don’t see any places where this behaviour change will cause issues, but there are a lot of callsites.
Looking at the callsites of
ParseHex()
, it looks like there’s a significant number of places where we first checkIsHex()
and thenParseHex()
, iterating over the string twice when I think this can just be replaced with a singleTryParseHex()
. Besides being (I’d assume) more efficient, the main behaviour difference I can see is thatIsHex()
also returns false on an empty string, but in the proposed diff I don’t think that makes a difference.Can just as well be done in a follow-up, though, it’s a much bigger diff than the current PR. (Also: didn’t yet super thoroughly check all the template specifiers but happy to revise if you think this approach makes sense).
0diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp 1index dc3038316..7b57b0637 100644 2--- a/src/bitcoin-tx.cpp 3+++ b/src/bitcoin-tx.cpp 4@@ -442,12 +442,10 @@ static void MutateTxAddOutData(CMutableTransaction& tx, const std::string& strIn 5 // extract and validate DATA 6 const std::string strData{strInput.substr(pos, std::string::npos)}; 7 8- if (!IsHex(strData)) 9- throw std::runtime_error("invalid TX output data"); 10+ auto data{TryParseHex<unsigned char>(strData)}; 11+ if (!data.has_value()) throw std::runtime_error("invalid TX output data"); 12 13- std::vector<unsigned char> data = ParseHex(strData); 14- 15- CTxOut txout(value, CScript() << OP_RETURN << data); 16+ CTxOut txout(value, CScript() << OP_RETURN << data.value()); 17 tx.vout.push_back(txout); 18 } 19 20diff --git a/src/core_read.cpp b/src/core_read.cpp 21index 7bab171c8..b580e7867 100644 22--- a/src/core_read.cpp 23+++ b/src/core_read.cpp 24@@ -194,20 +194,16 @@ static bool DecodeTx(CMutableTransaction& tx, const std::vector<unsigned char>& 25 26 bool DecodeHexTx(CMutableTransaction& tx, const std::string& hex_tx, bool try_no_witness, bool try_witness) 27 { 28- if (!IsHex(hex_tx)) { 29- return false; 30- } 31- 32- std::vector<unsigned char> txData(ParseHex(hex_tx)); 33- return DecodeTx(tx, txData, try_no_witness, try_witness); 34+ auto tx_data{TryParseHex<unsigned char>(hex_tx)}; 35+ if (!tx_data) return false; 36+ return DecodeTx(tx, tx_data.value(), try_no_witness, try_witness); 37 } 38 39 bool DecodeHexBlockHeader(CBlockHeader& header, const std::string& hex_header) 40 { 41- if (!IsHex(hex_header)) return false; 42- 43- const std::vector<unsigned char> header_data{ParseHex(hex_header)}; 44- CDataStream ser_header(header_data, SER_NETWORK, PROTOCOL_VERSION); 45+ auto header_data{TryParseHex<unsigned char>(hex_header)}; 46+ if (!header_data) return false; 47+ CDataStream ser_header(header_data.value(), SER_NETWORK, PROTOCOL_VERSION); 48 try { 49 ser_header >> header; 50 } catch (const std::exception&) { 51@@ -218,11 +214,9 @@ bool DecodeHexBlockHeader(CBlockHeader& header, const std::string& hex_header) 52 53 bool DecodeHexBlk(CBlock& block, const std::string& strHexBlk) 54 { 55- if (!IsHex(strHexBlk)) 56- return false; 57- 58- std::vector<unsigned char> blockData(ParseHex(strHexBlk)); 59- CDataStream ssBlock(blockData, SER_NETWORK, PROTOCOL_VERSION); 60+ auto block_data{TryParseHex<unsigned char>(strHexBlk)}; 61+ if (!block_data) return false; 62+ CDataStream ssBlock(block_data.value(), SER_NETWORK, PROTOCOL_VERSION); 63 try { 64 ssBlock >> block; 65 } 66@@ -247,9 +241,9 @@ std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strN 67 std::string strHex; 68 if (v.isStr()) 69 strHex = v.getValStr(); 70- if (!IsHex(strHex)) 71- throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')"); 72- return ParseHex(strHex); 73+ auto hex{TryParseHex<unsigned char>(strHex)}; 74+ if (!hex) throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')"); 75+ return hex.value(); 76 } 77 78 int ParseSighashString(const UniValue& sighash) 79diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp 80index 85158c99c..3e68182d5 100644 81--- a/src/rpc/util.cpp 82+++ b/src/rpc/util.cpp 83@@ -112,9 +112,10 @@ std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName) 84 std::string strHex; 85 if (v.isStr()) 86 strHex = v.get_str(); 87- if (!IsHex(strHex)) 88- throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); 89- return ParseHex(strHex); 90+ auto hex{TryParseHex<unsigned char>(strHex)}; 91+ if (!hex) throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); 92+ 93+ return hex.value(); 94 } 95 std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey) 96 { 97@@ -198,10 +199,9 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList& 98 // Converts a hex string to a public key if possible 99 CPubKey HexToPubKey(const std::string& hex_in) 100 { 101- if (!IsHex(hex_in)) { 102- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in); 103- } 104- CPubKey vchPubKey(ParseHex(hex_in)); 105+ auto hex{TryParseHex<uint8_t>(hex_in)}; 106+ if (!hex) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in); 107+ CPubKey vchPubKey(hex.value()); 108 if (!vchPubKey.IsFullyValid()) { 109 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in); 110 } 111diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp 112index 864eb8864..83d465039 100644 113--- a/src/script/descriptor.cpp 114+++ b/src/script/descriptor.cpp 115@@ -1082,9 +1082,9 @@ std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const S 116 return nullptr; 117 } 118 if (split.size() == 1) { 119- if (IsHex(str)) { 120- std::vector<unsigned char> data = ParseHex(str); 121- CPubKey pubkey(data); 122+ auto data{TryParseHex<unsigned char>(str)}; 123+ if (data) { 124+ CPubKey pubkey(data.value()); 125 if (pubkey.IsFullyValid()) { 126 if (permit_uncompressed || pubkey.IsCompressed()) { 127 return std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, false); 128@@ -1092,9 +1092,9 @@ std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const S 129 error = "Uncompressed keys are not allowed"; 130 return nullptr; 131 } 132- } else if (data.size() == 32 && ctx == ParseScriptContext::P2TR) { 133+ } else if (data.value().size() == 32 && ctx == ParseScriptContext::P2TR) { 134 unsigned char fullkey[33] = {0x02}; 135- std::copy(data.begin(), data.end(), fullkey + 1); 136+ std::copy(data.value().begin(), data.value().end(), fullkey + 1); 137 pubkey.Set(std::begin(fullkey), std::end(fullkey)); 138 if (pubkey.IsFullyValid()) { 139 return std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, true); 140@@ -1160,15 +1160,15 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c 141 return nullptr; 142 } 143 std::string fpr_hex = std::string(slash_split[0].begin(), slash_split[0].end()); 144- if (!IsHex(fpr_hex)) { 145+ auto fpr_bytes{TryParseHex<unsigned char>(fpr_hex)}; 146+ if (!fpr_bytes) { 147 error = strprintf("Fingerprint '%s' is not hex", fpr_hex); 148 return nullptr; 149 } 150- auto fpr_bytes = ParseHex(fpr_hex); 151 KeyOriginInfo info; 152 static_assert(sizeof(info.fingerprint) == 4, "Fingerprint must be 4 bytes"); 153- assert(fpr_bytes.size() == 4); 154- std::copy(fpr_bytes.begin(), fpr_bytes.end(), info.fingerprint); 155+ assert(fpr_bytes.value().size() == 4); 156+ std::copy(fpr_bytes.value().begin(), fpr_bytes.value().end(), info.fingerprint); 157 if (!ParseKeyPath(slash_split, info.path, error)) return nullptr; 158 auto provider = ParsePubkeyInner(key_exp_index, origin_split[1], ctx, out, error); 159 if (!provider) return nullptr; 160@@ -1488,12 +1488,12 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const 161 } 162 if (ctx == ParseScriptContext::TOP && Func("raw", expr)) { 163 std::string str(expr.begin(), expr.end()); 164- if (!IsHex(str)) { 165+ auto bytes{TryParseHex<unsigned char>(str)}; 166+ if (!bytes) { 167 error = "Raw script is not hex"; 168 return nullptr; 169 } 170- auto bytes = ParseHex(str); 171- return std::make_unique<RawDescriptor>(CScript(bytes.begin(), bytes.end())); 172+ return std::make_unique<RawDescriptor>(CScript(bytes.value().begin(), bytes.value().end())); 173 } else if (Func("raw", expr)) { 174 error = "Can only have raw() at top level"; 175 return nullptr; 176diff --git a/src/script/miniscript.h b/src/script/miniscript.h 177index fa3b0350e..f400aac44 100644 178--- a/src/script/miniscript.h 179+++ b/src/script/miniscript.h 180@@ -1001,10 +1001,10 @@ std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(Span<co 181 int hash_size = FindNextChar(in, ')'); 182 if (hash_size < 1) return {}; 183 std::string val = std::string(in.begin(), in.begin() + hash_size); 184- if (!IsHex(val)) return {}; 185- auto hash = ParseHex(val); 186- if (hash.size() != expected_size) return {}; 187- return {{std::move(hash), hash_size}}; 188+ auto hash{TryParseHex<unsigned char>(val)}; 189+ if (!hash) return {}; 190+ if (hash.value().size() != expected_size) return {}; 191+ return {{std::move(hash.value()), hash_size}}; 192 } 193 194 /** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */ 195diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp 196index efa548ad9..55fa278e0 100644 197--- a/src/wallet/dump.cpp 198+++ b/src/wallet/dump.cpp 199@@ -240,22 +240,21 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: 200 continue; 201 } 202 203- if (!IsHex(key)) { 204+ auto k{TryParseHex<unsigned char>(key)}; 205+ if (!k) { 206 error = strprintf(_("Error: Got key that was not hex: %s"), key); 207 ret = false; 208 break; 209 } 210- if (!IsHex(value)) { 211+ auto v{TryParseHex<unsigned char>(value)}; 212+ if (!v) { 213 error = strprintf(_("Error: Got value that was not hex: %s"), value); 214 ret = false; 215 break; 216 } 217 218- std::vector<unsigned char> k = ParseHex(key); 219- std::vector<unsigned char> v = ParseHex(value); 220- 221- CDataStream ss_key(k, SER_DISK, CLIENT_VERSION); 222- CDataStream ss_value(v, SER_DISK, CLIENT_VERSION); 223+ CDataStream ss_key(k.value(), SER_DISK, CLIENT_VERSION); 224+ CDataStream ss_value(v.value(), SER_DISK, CLIENT_VERSION); 225 226 if (!batch->Write(ss_key, ss_value)) { 227 error = strprintf(_("Error: Unable to write record to new wallet")); 228diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp 229index 95117b6c1..382b731d4 100644 230--- a/src/wallet/rpc/backup.cpp 231+++ b/src/wallet/rpc/backup.cpp 232@@ -290,9 +290,8 @@ RPCHelpMan importaddress() 233 pwallet->MarkDirty(); 234 235 pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); 236- } else if (IsHex(request.params[0].get_str())) { 237- std::vector<unsigned char> data(ParseHex(request.params[0].get_str())); 238- CScript redeem_script(data.begin(), data.end()); 239+ } else if (auto data{TryParseHex<unsigned char>(request.params[0].get_str())}) { 240+ CScript redeem_script(data.value().begin(), data.value().end()); 241 242 std::set<CScript> scripts = {redeem_script}; 243 pwallet->ImportScripts(scripts, /*timestamp=*/0); 244@@ -463,10 +462,10 @@ RPCHelpMan importpubkey() 245 throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); 246 } 247 248- if (!IsHex(request.params[0].get_str())) 249- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string"); 250- std::vector<unsigned char> data(ParseHex(request.params[0].get_str())); 251- CPubKey pubKey(data); 252+ auto data{TryParseHex<unsigned char>(request.params[0].get_str())}; 253+ if (!data) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string"); 254+ 255+ CPubKey pubKey(data.value()); 256 if (!pubKey.IsFullyValid()) 257 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key"); 258 259@@ -576,9 +575,8 @@ RPCHelpMan importwallet() 260 } 261 nTimeBegin = std::min(nTimeBegin, nTime); 262 keys.push_back(std::make_tuple(key, nTime, fLabel, strLabel)); 263- } else if(IsHex(vstr[0])) { 264- std::vector<unsigned char> vData(ParseHex(vstr[0])); 265- CScript script = CScript(vData.begin(), vData.end()); 266+ } else if(auto data{TryParseHex<unsigned char>(vstr[0])}) { 267+ CScript script = CScript(data.value().begin(), data.value().end()); 268 int64_t birth_time = ParseISO8601DateTime(vstr[1]); 269 if (birth_time > 0) nTimeBegin = std::min(nTimeBegin, birth_time); 270 scripts.push_back(std::pair<CScript, int64_t>(script, birth_time)); 271@@ -959,11 +957,10 @@ static UniValue ProcessImportLegacy(ImportData& import_data, std::map<CKeyID, CP 272 } 273 script = GetScriptForDestination(dest); 274 } else { 275- if (!IsHex(output)) { 276- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid scriptPubKey \"" + output + "\""); 277- } 278- std::vector<unsigned char> vData(ParseHex(output)); 279- script = CScript(vData.begin(), vData.end()); 280+ auto data{TryParseHex<unsigned char>(output)}; 281+ if (!IsHex(output)) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid scriptPubKey \"" + output + "\""); 282+ 283+ script = CScript(data.value().begin(), data.value().end()); 284 CTxDestination dest; 285 if (!ExtractDestination(script, dest) && !internal) { 286 throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal must be set to true for nonstandard scriptPubKey imports."); 287@@ -973,26 +970,24 @@ static UniValue ProcessImportLegacy(ImportData& import_data, std::map<CKeyID, CP 288 289 // Parse all arguments 290 if (strRedeemScript.size()) { 291- if (!IsHex(strRedeemScript)) { 292+ auto parsed_redeemscript{TryParseHex<uint8_t>(strRedeemScript)}; 293+ if (!parsed_redeemscript) { 294 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid redeem script \"" + strRedeemScript + "\": must be hex string"); 295 } 296- auto parsed_redeemscript = ParseHex(strRedeemScript); 297- import_data.redeemscript = std::make_unique<CScript>(parsed_redeemscript.begin(), parsed_redeemscript.end()); 298+ import_data.redeemscript = std::make_unique<CScript>(parsed_redeemscript.value().begin(), parsed_redeemscript.value().end()); 299 } 300 if (witness_script_hex.size()) { 301- if (!IsHex(witness_script_hex)) { 302+ auto parsed_witnessscript{TryParseHex<uint8_t>(witness_script_hex)}; 303+ if (!parsed_witnessscript) { 304 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid witness script \"" + witness_script_hex + "\": must be hex string"); 305 } 306- auto parsed_witnessscript = ParseHex(witness_script_hex); 307- import_data.witnessscript = std::make_unique<CScript>(parsed_witnessscript.begin(), parsed_witnessscript.end()); 308+ import_data.witnessscript = std::make_unique<CScript>(parsed_witnessscript.value().begin(), parsed_witnessscript.value().end()); 309 } 310 for (size_t i = 0; i < pubKeys.size(); ++i) { 311 const auto& str = pubKeys[i].get_str(); 312- if (!IsHex(str)) { 313- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" must be a hex string"); 314- } 315- auto parsed_pubkey = ParseHex(str); 316- CPubKey pubkey(parsed_pubkey); 317+ auto parsed_pubkey{TryParseHex<uint8_t>(str)}; 318+ if (!parsed_pubkey) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" must be a hex string"); 319+ CPubKey pubkey(parsed_pubkey.value()); 320 if (!pubkey.IsFullyValid()) { 321 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" is not a valid public key"); 322 } 323diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp 324index 5f31c1d72..631fa7c1e 100644 325--- a/src/wallet/rpc/spend.cpp 326+++ b/src/wallet/rpc/spend.cpp 327@@ -605,11 +605,9 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, 328 if (solving_data.exists("pubkeys")) { 329 for (const UniValue& pk_univ : solving_data["pubkeys"].get_array().getValues()) { 330 const std::string& pk_str = pk_univ.get_str(); 331- if (!IsHex(pk_str)) { 332- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", pk_str)); 333- } 334- const std::vector<unsigned char> data(ParseHex(pk_str)); 335- const CPubKey pubkey(data.begin(), data.end()); 336+ auto data(TryParseHex<unsigned char>(pk_str)); 337+ if (!data) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", pk_str)); 338+ const CPubKey pubkey(data.value().begin(), data.value().end()); 339 if (!pubkey.IsFullyValid()) { 340 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not a valid public key", pk_str)); 341 } 342@@ -623,11 +621,9 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, 343 if (solving_data.exists("scripts")) { 344 for (const UniValue& script_univ : solving_data["scripts"].get_array().getValues()) { 345 const std::string& script_str = script_univ.get_str(); 346- if (!IsHex(script_str)) { 347- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", script_str)); 348- } 349- std::vector<unsigned char> script_data(ParseHex(script_str)); 350- const CScript script(script_data.begin(), script_data.end()); 351+ auto script_data{TryParseHex<unsigned char>(script_str)}; 352+ if (!script_data) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", script_str)); 353+ const CScript script(script_data.value().begin(), script_data.value().end()); 354 coinControl.m_external_provider.scripts.emplace(CScriptID(script), script); 355 } 356 } 357</details>
test: Add hex parse unit tests fa3549a77butil: Return empty vector on invalid hex encoding faab273e06maflcko force-pushed on Feb 27, 2023maflcko commented at 12:42 pm on February 27, 2023: memberforce pushed to change tests. can be re-reviewed with range-difffanquake requested review from pinheadmz on Feb 27, 2023fanquake requested review from stickies-v on Feb 27, 2023stickies-v approvedstickies-v commented at 1:42 pm on February 27, 2023: contributorre-ACK faab273e060d27e166b5fb7fe7692614ec9e5c76fanquake merged this on Feb 27, 2023fanquake closed this on Feb 27, 2023
sidhujag referenced this in commit 2b4a2bc118 on Feb 27, 2023maflcko deleted the branch on Feb 27, 2023hebasto commented at 12:03 pm on March 6, 2023: memberThis PR causes cross compiling errors for some hosts using default toolchains on Ubuntu 22.04.
For example, for
riscv64-linux-gnu
0$ make -C src bitcoind 1... 2 CXXLD bitcoind 3/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `.L11743': 4net_processing.cpp:(.text+0x1cc82): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)' 5/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-rest.o): in function `.L2528': 6rest.cpp:(.text+0x4fe4): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)' 7/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-torcontrol.o): in function `.L0 ': 8torcontrol.cpp:(.text._Z8ParseHexIhESt6vectorIT_SaIS1_EESt17basic_string_viewIcSt11char_traitsIcEE[_Z8ParseHexIhESt6vectorIT_SaIS1_EESt17basic_string_viewIcSt11char_traitsIcEE]+0x1a): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)' 9/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_common.a(libbitcoin_common_a-external_signer.o): in function `.L3933': 10external_signer.cpp:(.text+0x5378): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)' 11collect2: error: ld returned 1 exit status 12make: *** [Makefile:7178: bitcoind] Error 1 13make: Leaving directory '/home/hebasto/git/bitcoin/src'
OTOH, Guix build for
riscv64-linux-gnu
is successful.A compiler bug about
std::byte
type implementation?maflcko commented at 2:15 pm on March 6, 2023: memberYeah, the same happens on Lunar (gcc 12.2). Steps to reproduce on a fresh install:
0export HOST=powerpc64le-linux-gnu && export MAKEJOBS="$(nproc)" && apt update && apt install git vim htop -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && apt install -y bzip2 make automake cmake curl g++-multilib libtool binutils bsdmainutils pkg-config python3 patch bison && apt install -y g++-arm-linux-gnueabihf binutils-arm-linux-gnueabihf g++-aarch64-linux-gnu binutils-aarch64-linux-gnu g++-powerpc64-linux-gnu binutils-powerpc64-linux-gnu g++-powerpc64le-linux-gnu binutils-powerpc64le-linux-gnu g++-riscv64-linux-gnu binutils-riscv64-linux-gnu g++-s390x-linux-gnu binutils-s390x-linux-gnu && ( cd depends && make NO_QT=1 "-j${MAKEJOBS}" ) && ./autogen.sh && CONFIG_SITE=$PWD/depends/$HOST/share/config.site ./configure && make "-j${MAKEJOBS}" src/bitcoind
maflcko commented at 2:33 pm on March 6, 2023: memberIt works on gcc-10 (debian bullseye), same steps to reproduce as above.gruve-p commented at 3:01 pm on March 6, 2023: contributorSame issue on Ubuntu Jammy (22.04) when cross compiling with depends with gcc 11 on these hosts:i686-pc-linux-gnu
arm-linux-gnueabihf
powerpc64-linux-gnu
powerpc64le-linux-gnu
riscv64-linux-gnu
s390x-linux-gnu
john-moffett commented at 5:32 pm on March 6, 2023: contributorI guess it’s having issues with the template for the
std::optional<std::vector<uint8_t>>
instantiation, since it’s not available to the linker?Adding explicit instantiations should fix it, I think:
0diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp 1index 03459dc..d8b6318 100644 2--- a/src/util/strencodings.cpp 3+++ b/src/util/strencodings.cpp 4@@ -97,6 +97,8 @@ std::optional<std::vector<Byte>> TryParseHex(std::string_view str) 5 } 6 template std::vector<std::byte> ParseHex(std::string_view); 7 template std::vector<uint8_t> ParseHex(std::string_view); 8+template std::optional<std::vector<std::byte>> TryParseHex(std::string_view); 9+template std::optional<std::vector<uint8_t>> TryParseHex(std::string_view); 10 11 bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut) 12 {
Alternatively, I suppose you could define the template in the .h file:
0diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp 1index 03459dc..4fdaafd 100644 2--- a/src/util/strencodings.cpp 3+++ b/src/util/strencodings.cpp 4@@ -80,21 +79,0 @@ bool IsHexNumber(std::string_view str) 5-template <typename Byte> 6-std::optional<std::vector<Byte>> TryParseHex(std::string_view str) 7-{ 8- std::vector<Byte> vch; 9- auto it = str.begin(); 10- while (it != str.end()) { 11- if (IsSpace(*it)) { 12- ++it; 13- continue; 14- } 15- auto c1 = HexDigit(*(it++)); 16- if (it == str.end()) return std::nullopt; 17- auto c2 = HexDigit(*(it++)); 18- if (c1 < 0 || c2 < 0) return std::nullopt; 19- vch.push_back(Byte(c1 << 4) | Byte(c2)); 20- } 21- return vch; 22-} 23-template std::vector<std::byte> ParseHex(std::string_view); 24-template std::vector<uint8_t> ParseHex(std::string_view); 25- 26diff --git a/src/util/strencodings.h b/src/util/strencodings.h 27index 05e7b95..4efd25d 100644 28--- a/src/util/strencodings.h 29+++ b/src/util/strencodings.h 30@@ -68,0 +69 @@ std::vector<Byte> ParseHex(std::string_view hex_str) 31+ 32@@ -82,0 +84 @@ std::optional<std::vector<unsigned char>> DecodeBase32(std::string_view str); 33+ 34@@ -171,0 +174,19 @@ constexpr inline bool IsSpace(char c) noexcept { 35+template <typename Byte> 36+std::optional<std::vector<Byte>> TryParseHex(std::string_view str) 37+{ 38+ std::vector<Byte> vch; 39+ auto it = str.begin(); 40+ while (it != str.end()) { 41+ if (IsSpace(*it)) { 42+ ++it; 43+ continue; 44+ } 45+ auto c1 = HexDigit(*(it++)); 46+ if (it == str.end()) return std::nullopt; 47+ auto c2 = HexDigit(*(it++)); 48+ if (c1 < 0 || c2 < 0) return std::nullopt; 49+ vch.push_back(Byte(c1 << 4) | Byte(c2)); 50+ } 51+ return vch; 52+} 53+
I’m not sure why it’s working on most builds. Maybe the existing
ParseHex
explicit instantiations force explicit instantiations forTryParseHex
but only on some compilers?maflcko commented at 10:48 am on March 7, 2023: memberI guess it’s having issues with the template for the std::optional<std::vector<uint8_t» instantiation, since it’s not available to the linker?
It might be a compiler bug where the compiler skips the explicit instantiation of a template that has a function body (and thus is already required to be implicitly instantiated)?
Adding explicit instantiations should fix it, I think:
Thanks. Done something like that in #https://github.com/bitcoin/bitcoin/pull/27218
Alternatively, I suppose you could define the template in the .h file:
Seems fine, but maybe the code should be modified further with compile time assertions on
Byte
to catch programming mistakes, because moving it to the header exposed theByte
choice to the developer? We could also think about making a full compile-time version (https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1349509341). Can be done in a follow-up, I guess :man_shrugging:fanquake referenced this in commit d4ebdceaef on Mar 7, 2023sidhujag referenced this in commit 9cbc91d341 on Mar 7, 2023stickies-v referenced this in commit a83e0a8e55 on Mar 15, 2023bitcoin locked this on Mar 6, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me