From fafe4b80512a5a82712a3ee81b68cfeb21271dee Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 18 Jul 2024 22:17:09 +0200 Subject: [PATCH 1/7] test: refactor: Replace SetHex with uint256 constructor directly This avoids a hex-decoding and makes the next commit smaller. --- src/test/pow_tests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/pow_tests.cpp b/src/test/pow_tests.cpp index 3a44d1da499..efca5402408 100644 --- a/src/test/pow_tests.cpp +++ b/src/test/pow_tests.cpp @@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_negative_target) uint256 hash; unsigned int nBits; nBits = UintToArith256(consensus.powLimit).GetCompact(true); - hash.SetHex("0x1"); + hash = uint256{1}; BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus)); } @@ -95,7 +95,7 @@ BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_overflow_target) const auto consensus = CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus(); uint256 hash; unsigned int nBits{~0x00800000U}; - hash.SetHex("0x1"); + hash = uint256{1}; BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus)); } @@ -107,7 +107,7 @@ BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_too_easy_target) arith_uint256 nBits_arith = UintToArith256(consensus.powLimit); nBits_arith *= 2; nBits = nBits_arith.GetCompact(); - hash.SetHex("0x1"); + hash = uint256{1}; BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus)); } From fa103db2bb736bce4440f0bde564e6671e36311d Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 18 Jul 2024 22:24:38 +0200 Subject: [PATCH 2/7] scripted-diff: Rename SetHex to SetHexDeprecated SetHex is fragile, because it accepts any non-hex input or any length of input, without error feedback. This can lead to issues when the input is truncated or otherwise corrupted. Document the problem by renaming the method. In the future, the fragile method should be removed from the public interface. -BEGIN VERIFY SCRIPT- sed -i 's/SetHex/SetHexDeprecated/g' $( git grep -l SetHex ./src ) -END VERIFY SCRIPT- --- src/core_read.cpp | 2 +- src/qt/transactiontablemodel.cpp | 2 +- src/qt/transactionview.cpp | 6 +++--- src/test/uint256_tests.cpp | 18 +++++++++--------- src/uint256.cpp | 6 +++--- src/uint256.h | 4 ++-- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/core_read.cpp b/src/core_read.cpp index 0ba271a8d29..3e0485eb7bf 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -239,7 +239,7 @@ bool ParseHashStr(const std::string& strHex, uint256& result) if ((strHex.size() != 64) || !IsHex(strHex)) return false; - result.SetHex(strHex); + result.SetHexDeprecated(strHex); return true; } diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index d4267fcf61f..9214e7723d3 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -277,7 +277,7 @@ void TransactionTableModel::updateAmountColumnTitle() void TransactionTableModel::updateTransaction(const QString &hash, int status, bool showTransaction) { uint256 updated; - updated.SetHex(hash.toStdString()); + updated.SetHexDeprecated(hash.toStdString()); priv->updateWallet(walletModel->wallet(), updated, status, showTransaction); } diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 7e24dbd3ec9..2aaa65c6f73 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -396,7 +396,7 @@ void TransactionView::contextualMenu(const QPoint &point) // check if transaction can be abandoned, disable context menu action in case it doesn't uint256 hash; - hash.SetHex(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString()); + hash.SetHexDeprecated(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString()); abandonAction->setEnabled(model->wallet().transactionCanBeAbandoned(hash)); bumpFeeAction->setEnabled(model->wallet().transactionCanBeBumped(hash)); copyAddressAction->setEnabled(GUIUtil::hasEntryData(transactionView, 0, TransactionTableModel::AddressRole)); @@ -416,7 +416,7 @@ void TransactionView::abandonTx() // get the hash from the TxHashRole (QVariant / QString) uint256 hash; QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString(); - hash.SetHex(hashQStr.toStdString()); + hash.SetHexDeprecated(hashQStr.toStdString()); // Abandon the wallet transaction over the walletModel model->wallet().abandonTransaction(hash); @@ -431,7 +431,7 @@ void TransactionView::bumpFee([[maybe_unused]] bool checked) // get the hash from the TxHashRole (QVariant / QString) uint256 hash; QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString(); - hash.SetHex(hashQStr.toStdString()); + hash.SetHexDeprecated(hashQStr.toStdString()); // Bump tx fee over the walletModel uint256 newHash; diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp index 669983c0e95..59290692715 100644 --- a/src/test/uint256_tests.cpp +++ b/src/test/uint256_tests.cpp @@ -62,7 +62,7 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width) inline uint160 uint160S(std::string_view str) { uint160 rv; - rv.SetHex(str); + rv.SetHexDeprecated(str); return rv; } @@ -157,7 +157,7 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < > uint256S("1000000000000000000000000000000000000000000000000000000000000002")); } -BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize +BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHexDeprecated begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize { BOOST_CHECK_EQUAL(R1L.GetHex(), R1L.ToString()); BOOST_CHECK_EQUAL(R2L.GetHex(), R2L.ToString()); @@ -166,12 +166,12 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex begin() end() size() GetLow64 G uint256 TmpL(R1L); BOOST_CHECK_EQUAL(TmpL, R1L); // Verify previous values don't persist when setting to truncated string. - TmpL.SetHex("21"); + TmpL.SetHexDeprecated("21"); BOOST_CHECK_EQUAL(TmpL.ToString(), "0000000000000000000000000000000000000000000000000000000000000021"); - TmpL.SetHex(R2L.ToString()); BOOST_CHECK_EQUAL(TmpL, R2L); - TmpL.SetHex(ZeroL.ToString()); BOOST_CHECK_EQUAL(TmpL, uint256()); + TmpL.SetHexDeprecated(R2L.ToString()); BOOST_CHECK_EQUAL(TmpL, R2L); + TmpL.SetHexDeprecated(ZeroL.ToString()); BOOST_CHECK_EQUAL(TmpL, uint256()); - TmpL.SetHex(R1L.ToString()); + TmpL.SetHexDeprecated(R1L.ToString()); BOOST_CHECK_EQUAL_COLLECTIONS(R1L.begin(), R1L.end(), R1Array, R1Array + R1L.size()); BOOST_CHECK_EQUAL_COLLECTIONS(TmpL.begin(), TmpL.end(), R1Array, R1Array + TmpL.size()); BOOST_CHECK_EQUAL_COLLECTIONS(R2L.begin(), R2L.end(), R2Array, R2Array + R2L.size()); @@ -214,10 +214,10 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex begin() end() size() GetLow64 G BOOST_CHECK_EQUAL(MaxS.GetHex(), MaxS.ToString()); uint160 TmpS(R1S); BOOST_CHECK_EQUAL(TmpS, R1S); - TmpS.SetHex(R2S.ToString()); BOOST_CHECK_EQUAL(TmpS, R2S); - TmpS.SetHex(ZeroS.ToString()); BOOST_CHECK_EQUAL(TmpS, uint160()); + TmpS.SetHexDeprecated(R2S.ToString()); BOOST_CHECK_EQUAL(TmpS, R2S); + TmpS.SetHexDeprecated(ZeroS.ToString()); BOOST_CHECK_EQUAL(TmpS, uint160()); - TmpS.SetHex(R1S.ToString()); + TmpS.SetHexDeprecated(R1S.ToString()); BOOST_CHECK_EQUAL_COLLECTIONS(R1S.begin(), R1S.end(), R1Array, R1Array + R1S.size()); BOOST_CHECK_EQUAL_COLLECTIONS(TmpS.begin(), TmpS.end(), R1Array, R1Array + TmpS.size()); BOOST_CHECK_EQUAL_COLLECTIONS(R2S.begin(), R2S.end(), R2Array, R2Array + R2S.size()); diff --git a/src/uint256.cpp b/src/uint256.cpp index f15150ca956..2756a7f5cd8 100644 --- a/src/uint256.cpp +++ b/src/uint256.cpp @@ -18,7 +18,7 @@ std::string base_blob::GetHex() const } template -void base_blob::SetHex(const std::string_view str) +void base_blob::SetHexDeprecated(const std::string_view str) { std::fill(m_data.begin(), m_data.end(), 0); @@ -52,12 +52,12 @@ std::string base_blob::ToString() const // Explicit instantiations for base_blob<160> template std::string base_blob<160>::GetHex() const; template std::string base_blob<160>::ToString() const; -template void base_blob<160>::SetHex(std::string_view); +template void base_blob<160>::SetHexDeprecated(std::string_view); // Explicit instantiations for base_blob<256> template std::string base_blob<256>::GetHex() const; template std::string base_blob<256>::ToString() const; -template void base_blob<256>::SetHex(std::string_view); +template void base_blob<256>::SetHexDeprecated(std::string_view); const uint256 uint256::ZERO(0); const uint256 uint256::ONE(1); diff --git a/src/uint256.h b/src/uint256.h index 406a9c7203c..3f7dce69834 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -59,7 +59,7 @@ public: // Hex string representations are little-endian. std::string GetHex() const; - void SetHex(std::string_view str); + void SetHexDeprecated(std::string_view str); std::string ToString() const; constexpr const unsigned char* data() const { return m_data.data(); } @@ -119,7 +119,7 @@ public: inline uint256 uint256S(std::string_view str) { uint256 rv; - rv.SetHex(str); + rv.SetHexDeprecated(str); return rv; } From fad2991ba073de0bd1f12e42bf0fbaca4a265508 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 18 Jul 2024 22:38:14 +0200 Subject: [PATCH 3/7] refactor: Implement strict uint256::FromHex() This is a safe replacement of the previous SetHex, which now returns an optional to indicate success or failure. The code is similar to the ParseHashStr helper, which will be removed in a later commit. --- src/test/uint256_tests.cpp | 14 +++++++------- src/uint256.h | 29 +++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp index 59290692715..d280126cde4 100644 --- a/src/test/uint256_tests.cpp +++ b/src/test/uint256_tests.cpp @@ -157,7 +157,7 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < > uint256S("1000000000000000000000000000000000000000000000000000000000000002")); } -BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHexDeprecated begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize +BOOST_AUTO_TEST_CASE(methods) // GetHex SetHexDeprecated FromHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize { BOOST_CHECK_EQUAL(R1L.GetHex(), R1L.ToString()); BOOST_CHECK_EQUAL(R2L.GetHex(), R2L.ToString()); @@ -168,10 +168,10 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHexDeprecated begin() end() size() // Verify previous values don't persist when setting to truncated string. TmpL.SetHexDeprecated("21"); BOOST_CHECK_EQUAL(TmpL.ToString(), "0000000000000000000000000000000000000000000000000000000000000021"); - TmpL.SetHexDeprecated(R2L.ToString()); BOOST_CHECK_EQUAL(TmpL, R2L); - TmpL.SetHexDeprecated(ZeroL.ToString()); BOOST_CHECK_EQUAL(TmpL, uint256()); + BOOST_CHECK_EQUAL(uint256::FromHex(R2L.ToString()).value(), R2L); + BOOST_CHECK_EQUAL(uint256::FromHex(ZeroL.ToString()).value(), uint256()); - TmpL.SetHexDeprecated(R1L.ToString()); + TmpL = uint256::FromHex(R1L.ToString()).value(); BOOST_CHECK_EQUAL_COLLECTIONS(R1L.begin(), R1L.end(), R1Array, R1Array + R1L.size()); BOOST_CHECK_EQUAL_COLLECTIONS(TmpL.begin(), TmpL.end(), R1Array, R1Array + TmpL.size()); BOOST_CHECK_EQUAL_COLLECTIONS(R2L.begin(), R2L.end(), R2Array, R2Array + R2L.size()); @@ -214,10 +214,10 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHexDeprecated begin() end() size() BOOST_CHECK_EQUAL(MaxS.GetHex(), MaxS.ToString()); uint160 TmpS(R1S); BOOST_CHECK_EQUAL(TmpS, R1S); - TmpS.SetHexDeprecated(R2S.ToString()); BOOST_CHECK_EQUAL(TmpS, R2S); - TmpS.SetHexDeprecated(ZeroS.ToString()); BOOST_CHECK_EQUAL(TmpS, uint160()); + BOOST_CHECK_EQUAL(uint160::FromHex(R2S.ToString()).value(), R2S); + BOOST_CHECK_EQUAL(uint160::FromHex(ZeroS.ToString()).value(), uint160()); - TmpS.SetHexDeprecated(R1S.ToString()); + TmpS = uint160::FromHex(R1S.ToString()).value(); BOOST_CHECK_EQUAL_COLLECTIONS(R1S.begin(), R1S.end(), R1Array, R1Array + R1S.size()); BOOST_CHECK_EQUAL_COLLECTIONS(TmpS.begin(), TmpS.end(), R1Array, R1Array + TmpS.size()); BOOST_CHECK_EQUAL_COLLECTIONS(R2S.begin(), R2S.end(), R2Array, R2Array + R2S.size()); diff --git a/src/uint256.h b/src/uint256.h index 3f7dce69834..f8e78b820f3 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -8,12 +8,14 @@ #include #include +#include #include #include #include +#include #include -#include +#include #include /** Template base class for fixed-sized opaque blobs. */ @@ -59,6 +61,7 @@ public: // Hex string representations are little-endian. std::string GetHex() const; + /** Unlike FromHex this accepts any invalid input, thus it is fragile and deprecated */ void SetHexDeprecated(std::string_view str); std::string ToString() const; @@ -88,12 +91,30 @@ public: } }; +namespace detail { +/** + * Writes the hex string (treated as little-endian) into a new uintN_t object + * and only returns a value iff all of the checks pass: + * - Input length is uintN_t::size()*2 + * - All characters are hex + */ +template +std::optional FromHex(std::string_view str) +{ + if (uintN_t::size() * 2 != str.size() || !IsHex(str)) return std::nullopt; + uintN_t rv; + rv.SetHexDeprecated(str); + return rv; +} +} // namespace detail + /** 160-bit opaque blob. * @note This type is called uint160 for historical reasons only. It is an opaque * blob of 160 bits and has no integer operations. */ class uint160 : public base_blob<160> { public: + static std::optional FromHex(std::string_view str) { return detail::FromHex(str); } constexpr uint160() = default; constexpr explicit uint160(Span vch) : base_blob<160>(vch) {} }; @@ -105,6 +126,7 @@ public: */ class uint256 : public base_blob<256> { public: + static std::optional FromHex(std::string_view str) { return detail::FromHex(str); } constexpr uint256() = default; constexpr explicit uint256(uint8_t v) : base_blob<256>(v) {} constexpr explicit uint256(Span vch) : base_blob<256>(vch) {} @@ -113,8 +135,7 @@ public: }; /* uint256 from std::string_view, treated as little-endian. - * This is not a uint256 constructor because of historical fears of uint256(0) - * resolving to a NULL string and crashing. + * DEPRECATED. Unlike FromHex this accepts any invalid input, thus it is fragile and deprecated! */ inline uint256 uint256S(std::string_view str) { From fab6ddbee64e50d5e2f499aebca35b5911896ec4 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 19 Jul 2024 08:48:45 +0200 Subject: [PATCH 4/7] refactor: Expose FromHex in transaction_identifier This is needed for the next commit. --- src/util/transaction_identifier.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index 1ca48a04547..0e3251f4fce 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -42,6 +42,12 @@ public: /** Wrapped `uint256` methods. */ constexpr bool IsNull() const { return m_wrapped.IsNull(); } constexpr void SetNull() { m_wrapped.SetNull(); } + static std::optional FromHex(std::string_view hex) + { + auto u{uint256::FromHex(hex)}; + if (!u) return std::nullopt; + return FromUint256(*u); + } std::string GetHex() const { return m_wrapped.GetHex(); } std::string ToString() const { return m_wrapped.ToString(); } static constexpr auto size() { return decltype(m_wrapped)::size(); } @@ -66,6 +72,7 @@ using Txid = transaction_identifier; /** Wtxid commits to all transaction fields including the witness. */ using Wtxid = transaction_identifier; +/** DEPRECATED due to missing length-check and hex-check, please use the safer FromHex, or FromUint256 */ inline Txid TxidFromString(std::string_view str) { return Txid::FromUint256(uint256S(str)); From fa9077724507faad207f29509a8202fc6ac9d502 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 19 Jul 2024 08:52:21 +0200 Subject: [PATCH 5/7] rest: Reject truncated hex txid early in getutxos parsing --- src/rest.cpp | 5 +++-- test/functional/interface_rest.py | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/rest.cpp b/src/rest.cpp index 3cf6ad343c8..80e6b52938d 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -792,13 +792,14 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std:: if (txid_out.size() != 2) { return RESTERR(req, HTTP_BAD_REQUEST, "Parse error"); } + auto txid{Txid::FromHex(txid_out.at(0))}; auto output{ToIntegral(txid_out.at(1))}; - if (!output || !IsHex(txid_out.at(0))) { + if (!txid || !output) { return RESTERR(req, HTTP_BAD_REQUEST, "Parse error"); } - vOutPoints.emplace_back(TxidFromString(txid_out.at(0)), *output); + vOutPoints.emplace_back(*txid, *output); } if (vOutPoints.size() > 0) diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index d547da9cf24..ba6e960476b 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -208,6 +208,8 @@ class RESTTest (BitcoinTestFramework): self.test_rest_request(f"/getutxos/{spending[0]}_+1", ret_type=RetType.OBJ, status=400) self.test_rest_request(f"/getutxos/{spending[0]}-+1", ret_type=RetType.OBJ, status=400) self.test_rest_request(f"/getutxos/{spending[0]}--1", ret_type=RetType.OBJ, status=400) + self.test_rest_request(f"/getutxos/{spending[0]}aa-1234", ret_type=RetType.OBJ, status=400) + self.test_rest_request(f"/getutxos/aa-1234", ret_type=RetType.OBJ, status=400) # Test limits long_uri = '/'.join([f"{txid}-{n_}" for n_ in range(20)]) From fa7b57e5f5a6dafbbadc361ffd27b58afff1ed59 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 18 Jul 2024 22:54:38 +0200 Subject: [PATCH 6/7] refactor: Replace ParseHashStr with FromHex No need to have two functions with different names that achieve the exact same thing. --- src/bitcoin-tx.cpp | 12 +++++------ src/core_io.h | 9 -------- src/core_read.cpp | 9 -------- src/rest.cpp | 39 ++++++++++++++++++---------------- src/rpc/mining.cpp | 8 +++---- src/test/blockfilter_tests.cpp | 9 +++----- src/test/fuzz/hex.cpp | 3 +-- 7 files changed, 34 insertions(+), 55 deletions(-) diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 89faa0123a5..89c03c16472 100644 --- a/src/bitcoin-tx.cpp +++ b/src/bitcoin-tx.cpp @@ -264,8 +264,8 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu throw std::runtime_error("TX input missing separator"); // extract and validate TXID - uint256 txid; - if (!ParseHashStr(vStrInputParts[0], txid)) { + auto txid{Txid::FromHex(vStrInputParts[0])}; + if (!txid) { throw std::runtime_error("invalid TX input txid"); } @@ -285,7 +285,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu } // append to transaction input list - CTxIn txin(Txid::FromUint256(txid), vout, CScript(), nSequenceIn); + CTxIn txin(*txid, vout, CScript(), nSequenceIn); tx.vin.push_back(txin); } @@ -625,8 +625,8 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr) if (!prevOut.checkObject(types)) throw std::runtime_error("prevtxs internal object typecheck fail"); - uint256 txid; - if (!ParseHashStr(prevOut["txid"].get_str(), txid)) { + auto txid{Txid::FromHex(prevOut["txid"].get_str())}; + if (!txid) { throw std::runtime_error("txid must be hexadecimal string (not '" + prevOut["txid"].get_str() + "')"); } @@ -634,7 +634,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr) if (nOut < 0) throw std::runtime_error("vout cannot be negative"); - COutPoint out(Txid::FromUint256(txid), nOut); + COutPoint out(*txid, nOut); std::vector pkData(ParseHexUV(prevOut["scriptPubKey"], "scriptPubKey")); CScript scriptPubKey(pkData.begin(), pkData.end()); diff --git a/src/core_io.h b/src/core_io.h index 4405f5c8f83..9305bb72393 100644 --- a/src/core_io.h +++ b/src/core_io.h @@ -37,15 +37,6 @@ std::string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDeco [[nodiscard]] bool DecodeHexBlk(CBlock&, const std::string& strHexBlk); bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header); -/** - * Parse a hex string into 256 bits - * @param[in] strHex a hex-formatted, 64-character string - * @param[out] result the result of the parsing - * @returns true if successful, false if not - * - * @see ParseHashV for an RPC-oriented version of this - */ -bool ParseHashStr(const std::string& strHex, uint256& result); [[nodiscard]] util::Result SighashFromStr(const std::string& sighash); // core_write.cpp diff --git a/src/core_read.cpp b/src/core_read.cpp index 3e0485eb7bf..23f341c2304 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -234,15 +234,6 @@ bool DecodeHexBlk(CBlock& block, const std::string& strHexBlk) return true; } -bool ParseHashStr(const std::string& strHex, uint256& result) -{ - if ((strHex.size() != 64) || !IsHex(strHex)) - return false; - - result.SetHexDeprecated(strHex); - return true; -} - util::Result SighashFromStr(const std::string& sighash) { static const std::map map_sighash_values = { diff --git a/src/rest.cpp b/src/rest.cpp index 80e6b52938d..c42bc8e40c2 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -217,9 +217,10 @@ static bool rest_headers(const std::any& context, return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count is invalid or out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, raw_count)); } - uint256 hash; - if (!ParseHashStr(hashStr, hash)) + auto hash{uint256::FromHex(hashStr)}; + if (!hash) { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr); + } const CBlockIndex* tip = nullptr; std::vector headers; @@ -231,7 +232,7 @@ static bool rest_headers(const std::any& context, LOCK(cs_main); CChain& active_chain = chainman.ActiveChain(); tip = active_chain.Tip(); - const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash); + const CBlockIndex* pindex{chainman.m_blockman.LookupBlockIndex(*hash)}; while (pindex != nullptr && active_chain.Contains(pindex)) { headers.push_back(pindex); if (headers.size() == *parsed_count) { @@ -290,9 +291,10 @@ static bool rest_block(const std::any& context, std::string hashStr; const RESTResponseFormat rf = ParseDataFormat(hashStr, strURIPart); - uint256 hash; - if (!ParseHashStr(hashStr, hash)) + auto hash{uint256::FromHex(hashStr)}; + if (!hash) { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr); + } FlatFilePos pos{}; const CBlockIndex* pblockindex = nullptr; @@ -303,7 +305,7 @@ static bool rest_block(const std::any& context, { LOCK(cs_main); tip = chainman.ActiveChain().Tip(); - pblockindex = chainman.m_blockman.LookupBlockIndex(hash); + pblockindex = chainman.m_blockman.LookupBlockIndex(*hash); if (!pblockindex) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } @@ -390,8 +392,8 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count is invalid or out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, raw_count)); } - uint256 block_hash; - if (!ParseHashStr(raw_blockhash, block_hash)) { + auto block_hash{uint256::FromHex(raw_blockhash)}; + if (!block_hash) { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + raw_blockhash); } @@ -413,7 +415,7 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const ChainstateManager& chainman = *maybe_chainman; LOCK(cs_main); CChain& active_chain = chainman.ActiveChain(); - const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block_hash); + const CBlockIndex* pindex{chainman.m_blockman.LookupBlockIndex(*block_hash)}; while (pindex != nullptr && active_chain.Contains(pindex)) { headers.push_back(pindex); if (headers.size() == *parsed_count) @@ -494,8 +496,8 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilter//"); } - uint256 block_hash; - if (!ParseHashStr(uri_parts[1], block_hash)) { + auto block_hash{uint256::FromHex(uri_parts[1])}; + if (!block_hash) { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + uri_parts[1]); } @@ -516,7 +518,7 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s if (!maybe_chainman) return false; ChainstateManager& chainman = *maybe_chainman; LOCK(cs_main); - block_index = chainman.m_blockman.LookupBlockIndex(block_hash); + block_index = chainman.m_blockman.LookupBlockIndex(*block_hash); if (!block_index) { return RESTERR(req, HTTP_NOT_FOUND, uri_parts[1] + " not found"); } @@ -616,14 +618,14 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const jsonRequest.params = UniValue(UniValue::VARR); if (!hash_str.empty()) { - uint256 hash; - if (!ParseHashStr(hash_str, hash)) { + auto hash{uint256::FromHex(hash_str)}; + if (!hash) { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hash_str); } const ChainstateManager* chainman = GetChainman(context, req); if (!chainman) return false; - if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hash_str, "blockhash")))) { + if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(*hash))) { return RESTERR(req, HTTP_BAD_REQUEST, "Block not found"); } @@ -704,9 +706,10 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string std::string hashStr; const RESTResponseFormat rf = ParseDataFormat(hashStr, strURIPart); - uint256 hash; - if (!ParseHashStr(hashStr, hash)) + auto hash{uint256::FromHex(hashStr)}; + if (!hash) { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr); + } if (g_txindex) { g_txindex->BlockUntilSyncedToCurrentChain(); @@ -715,7 +718,7 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string const NodeContext* const node = GetNodeContext(context, req); if (!node) return false; uint256 hashBlock = uint256(); - const CTransactionRef tx = GetTransaction(/*block_index=*/nullptr, node->mempool.get(), hash, hashBlock, node->chainman->m_blockman); + const CTransactionRef tx{GetTransaction(/*block_index=*/nullptr, node->mempool.get(), *hash, hashBlock, node->chainman->m_blockman)}; if (!tx) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 8482ce6eb2e..d41ec5bdc62 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -345,13 +345,11 @@ static RPCHelpMan generateblock() std::vector txs; const auto raw_txs_or_txids = request.params[1].get_array(); for (size_t i = 0; i < raw_txs_or_txids.size(); i++) { - const auto str(raw_txs_or_txids[i].get_str()); + const auto& str{raw_txs_or_txids[i].get_str()}; - uint256 hash; CMutableTransaction mtx; - if (ParseHashStr(str, hash)) { - - const auto tx = mempool.get(hash); + if (auto hash{uint256::FromHex(str)}) { + const auto tx{mempool.get(*hash)}; if (!tx) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Transaction %s not in mempool.", str)); } diff --git a/src/test/blockfilter_tests.cpp b/src/test/blockfilter_tests.cpp index b372f25ea95..470fdde30a3 100644 --- a/src/test/blockfilter_tests.cpp +++ b/src/test/blockfilter_tests.cpp @@ -149,8 +149,7 @@ BOOST_AUTO_TEST_CASE(blockfilters_json_test) unsigned int pos = 0; /*int block_height =*/ test[pos++].getInt(); - uint256 block_hash; - BOOST_CHECK(ParseHashStr(test[pos++].get_str(), block_hash)); + BOOST_CHECK(uint256::FromHex(test[pos++].get_str())); CBlock block; BOOST_REQUIRE(DecodeHexBlk(block, test[pos++].get_str())); @@ -165,11 +164,9 @@ BOOST_AUTO_TEST_CASE(blockfilters_json_test) tx_undo.vprevout.emplace_back(txout, 0, false); } - uint256 prev_filter_header_basic; - BOOST_CHECK(ParseHashStr(test[pos++].get_str(), prev_filter_header_basic)); + uint256 prev_filter_header_basic{*Assert(uint256::FromHex(test[pos++].get_str()))}; std::vector filter_basic = ParseHex(test[pos++].get_str()); - uint256 filter_header_basic; - BOOST_CHECK(ParseHashStr(test[pos++].get_str(), filter_header_basic)); + uint256 filter_header_basic{*Assert(uint256::FromHex(test[pos++].get_str()))}; BlockFilter computed_filter_basic(BlockFilterType::BASIC, block, block_undo); BOOST_CHECK(computed_filter_basic.GetFilter().GetEncoded() == filter_basic); diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp index f67b820d11e..bc46863f1df 100644 --- a/src/test/fuzz/hex.cpp +++ b/src/test/fuzz/hex.cpp @@ -27,8 +27,7 @@ FUZZ_TARGET(hex) assert(ToLower(random_hex_string) == hex_data); } (void)IsHexNumber(random_hex_string); - uint256 result; - (void)ParseHashStr(random_hex_string, result); + (void)uint256::FromHex(random_hex_string); (void)uint256S(random_hex_string); try { (void)HexToPubKey(random_hex_string); From fac0c3d4bfc97b94f0594f7606650921feef2c8a Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 24 Jul 2024 17:38:59 +0200 Subject: [PATCH 7/7] doc: Add release notes for two pull requests --- doc/release-notes-30482.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 doc/release-notes-30482.md diff --git a/doc/release-notes-30482.md b/doc/release-notes-30482.md new file mode 100644 index 00000000000..fb625c2fa98 --- /dev/null +++ b/doc/release-notes-30482.md @@ -0,0 +1,6 @@ +Updated REST APIs +----------------- +- Parameter validation for `/rest/getutxos` has been improved by rejecting + truncated or overly large txids and malformed outpoint indices by raising an + HTTP_BAD_REQUEST "Parse error". Previously, these malformed requests would be + silently handled. (#30482, #30444)