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
  1. 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.
  2. maflcko force-pushed on May 27, 2022
  3. 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?
  4. DrahtBot added the label Utils/log/libs on May 27, 2022
  5. 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?

  6. 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.

  7. 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.

  8. 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
  9. maflcko renamed this:
    Return empty vector on invalid hex encoding
    Return nullopt on invalid hex encoding
    on May 30, 2022
  10. maflcko renamed this:
    Return nullopt on invalid hex encoding
    Handle invalid hex encoding in ParseHex
    on May 30, 2022
  11. maflcko force-pushed on May 30, 2022
  12. maflcko marked this as a draft on May 30, 2022
  13. DrahtBot added the label Needs rebase on Jul 7, 2022
  14. maflcko force-pushed on Dec 13, 2022
  15. DrahtBot removed the label Needs rebase on Dec 13, 2022
  16. 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
    
  17. maflcko marked this as ready for review on Dec 13, 2022
  18. maflcko force-pushed on Jan 3, 2023
  19. maflcko force-pushed on Jan 3, 2023
  20. 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
  21. maflcko force-pushed on Jan 9, 2023
  22. fanquake requested review from stickies-v on Feb 17, 2023
  23. fanquake requested review from john-moffett on Feb 17, 2023
  24. stickies-v commented at 10:32 am on February 23, 2023: contributor
    Concept ACK
  25. fanquake requested review from pinheadmz on Feb 23, 2023
  26. pinheadmz approved
  27. 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

  28. 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
  29. 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.
  30. stickies-v approved
  31. 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>
    
  32. test: Add hex parse unit tests fa3549a77b
  33. util: Return empty vector on invalid hex encoding faab273e06
  34. maflcko force-pushed on Feb 27, 2023
  35. maflcko commented at 12:42 pm on February 27, 2023: member
    force pushed to change tests. can be re-reviewed with range-diff
  36. fanquake requested review from pinheadmz on Feb 27, 2023
  37. fanquake requested review from stickies-v on Feb 27, 2023
  38. stickies-v approved
  39. stickies-v commented at 1:42 pm on February 27, 2023: contributor
    re-ACK faab273e060d27e166b5fb7fe7692614ec9e5c76
  40. fanquake merged this on Feb 27, 2023
  41. fanquake closed this on Feb 27, 2023

  42. sidhujag referenced this in commit 2b4a2bc118 on Feb 27, 2023
  43. maflcko deleted the branch on Feb 27, 2023
  44. 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?

  45. 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
    
  46. maflcko commented at 2:33 pm on March 6, 2023: member
    It works on gcc-10 (debian bullseye), same steps to reproduce as above.
  47. 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
  48. 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?

  49. 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:

  50. fanquake referenced this in commit d4ebdceaef on Mar 7, 2023
  51. sidhujag referenced this in commit 9cbc91d341 on Mar 7, 2023
  52. stickies-v referenced this in commit a83e0a8e55 on Mar 15, 2023
  53. bitcoin 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