Handle invalid hex encoding in ParseHex

maflcko commented at 1:49 pm on May 27, 2022: member

Seems 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: member

Looks 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: none

Concept 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

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 (a single space) is the input for the tested method.

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, done

maflcko force-pushed on Jan 9, 2023

fanquake requested review from stickies-v on Feb 17, 2023

fanquake requested review from john-moffett on Feb 17, 2023

stickies-v commented at 10:32 am on February 23, 2023: contributor

Concept ACK

fanquake requested review from pinheadmz on Feb 23, 2023

pinheadmz approved

pinheadmz commented at 6:39 pm on February 24, 2023: member

ACK 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, done

in 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 and TryParseHex tests

0    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 for TryParseHex. 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 approved

stickies-v commented at 12:01 pm on February 27, 2023: contributor

ACK 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 check IsHex() and then ParseHex(), iterating over the string twice when I think this can just be replaced with a single TryParseHex(). Besides being (I’d assume) more efficient, the main behaviour difference I can see is that IsHex() 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 fa3549a77b

util: Return empty vector on invalid hex encoding faab273e06

maflcko force-pushed on Feb 27, 2023

maflcko commented at 12:42 pm on February 27, 2023: member

force pushed to change tests. can be re-reviewed with range-diff

fanquake requested review from pinheadmz on Feb 27, 2023

fanquake requested review from stickies-v on Feb 27, 2023

stickies-v approved

stickies-v commented at 1:42 pm on February 27, 2023: contributor

re-ACK faab273e060d27e166b5fb7fe7692614ec9e5c76

fanquake merged this on Feb 27, 2023

fanquake closed this on Feb 27, 2023

sidhujag referenced this in commit 2b4a2bc118 on Feb 27, 2023

maflcko deleted the branch on Feb 27, 2023

hebasto commented at 12:03 pm on March 6, 2023: member

This 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: member

Yeah, 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: member

It works on gcc-10 (debian bullseye), same steps to reproduce as above.

gruve-p commented at 3:01 pm on March 6, 2023: contributor

Same 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: contributor

I 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 for TryParseHex but only on some compilers?

maflcko commented at 10:48 am on March 7, 2023: member

I 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 the Byte 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, 2023

sidhujag referenced this in commit 9cbc91d341 on Mar 7, 2023

stickies-v referenced this in commit a83e0a8e55 on Mar 15, 2023

bitcoin locked this on Mar 6, 2024

Handle invalid hex encoding in ParseHex #25227

Reviews

Conflicts