Consolidate redundant implementations of ParseHashStr

Empact commented at 3:08 pm on September 24, 2018: member

This change:

adds a length check to all calls to ParseHashStr, appropriate given its use to populate a 256-bit number from a hex str
allows the caller to handle the failure, which allows for the more appropriate JSONRPCError on failure in prioritisetransaction rpc

Relative to #14288

MarcoFalke added the label Refactoring on Sep 24, 2018

in src/core_read.cpp:196 in c76c7ad473 outdated

192@@ -193,14 +193,13 @@ bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx,
193     return true;
194 }
195 
196-uint256 ParseHashStr(const std::string& strHex, const std::string& strName)
197+bool ParseHashStr(const std::string& strReq, uint256& v)

MarcoFalke commented at 3:20 pm on September 24, 2018:

nit: Could keep the name strHex and result to minimize the diff?

MarcoFalke commented at 3:23 pm on September 24, 2018: member

Concept ACK. Looks like the new parse helper will reject strings that are hex but not exactly 64 chars. Could add a test that fails with this change, but passes without?

in src/core_read.cpp:198 in c76c7ad473 outdated

192@@ -193,14 +193,13 @@ bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx,
193     return true;
194 }
195 
196-uint256 ParseHashStr(const std::string& strHex, const std::string& strName)
197+bool ParseHashStr(const std::string& strReq, uint256& v)
198 {
199-    if (!IsHex(strHex)) // Note: IsHex("") is false
200-        throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
201+    if (!IsHex(strReq) || (strReq.size() != 64))

practicalswift commented at 3:41 pm on September 24, 2018:

Nit: Redundant parentheses around strReq.size() != 64 :-)

in src/bitcoin-tx.cpp:595 in c76c7ad473 outdated

589@@ -590,7 +590,10 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
590             if (!prevOut.checkObject(types))
591                 throw std::runtime_error("prevtxs internal object typecheck fail");
592 
593-            uint256 txid = ParseHashStr(prevOut["txid"].get_str(), "txid");
594+            uint256 txid;
595+            if (!ParseHashStr(prevOut["txid"].get_str(), txid)) {
596+                throw std::runtime_error("txid must be hexadecimal string (not '" + prevOut["txid"].get_str() + "')");

practicalswift commented at 3:43 pm on September 24, 2018:

I know this is unchanged from the previous version, but could there be circumstances where this opens up for log file injection? Safer not to echo the input?

Empact force-pushed on Sep 24, 2018

in src/rpc/mining.cpp:252 in dc95535d39 outdated

246@@ -247,7 +247,10 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)
247 
248     LOCK(cs_main);
249 
250-    uint256 hash = ParseHashStr(request.params[0].get_str(), "txid");
251+    uint256 hash;
252+    if (!ParseHashStr(request.params[0].get_str(), hash)) {
253+        throw JSONRPCError(RPC_INVALID_PARAMETER, "txid must be hexadecimal string (not '" + request.params[0].get_str() + "')");

practicalswift commented at 3:45 pm on September 24, 2018:

I know this is unchanged from the previous version, but could there be circumstances where this opens up for log file injection? Safer not to echo the input?

practicalswift commented at 3:46 pm on September 24, 2018: contributor

Concept ACK

Thanks for taking on this issue. Closing #14288.

Empact force-pushed on Sep 24, 2018

DrahtBot commented at 4:36 pm on September 24, 2018: member

#13815 (util: Add [[nodiscard]] to all {Decode,Parse}… functions returning bool by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Sep 24, 2018

Empact force-pushed on Sep 24, 2018

Empact commented at 9:25 pm on September 24, 2018: member

Rebased for #13424

Empact force-pushed on Sep 24, 2018

Empact commented at 9:49 pm on September 24, 2018: member

Converted one parse from uint256S to ParseHashStr in bitcoin-tx. Was the only other place I could find with local validation of 64-char hex inputs.

in src/core_read.cpp:198 in 0b5791e3e1 outdated

192@@ -193,14 +193,13 @@ bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx,
193     return true;
194 }
195 
196-uint256 ParseHashStr(const std::string& strHex, const std::string& strName)
197+bool ParseHashStr(const std::string& strHex, uint256& result)
198 {
199-    if (!IsHex(strHex)) // Note: IsHex("") is false
200-        throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
201+    if (!IsHex(strHex) || (strHex.size() != 64))

practicalswift commented at 9:49 pm on September 24, 2018:

Nit: Redundant parentheses around strHex.size() != 64 :-)

Empact commented at 4:59 am on September 25, 2018:

Could go either way but I like explicit precedence to guide the eye.

l2a5b1 commented at 11:41 am on September 25, 2018:

Perhaps swap the two boolean conditions? If the condition strHex.size() is false then the function can return without having to evaluate the condition IsHex(strHex)

0if ((strHex.size() != 64) || !IsHex(strHex)) return false;

in src/bitcoin-tx.cpp:245 in 0b5791e3e1 outdated

244-    if ((strTxid.size() != 64) || !IsHex(strTxid))
245-        throw std::runtime_error("invalid TX input txid");
246-    uint256 txid(uint256S(strTxid));
247+    uint256 txid;
248+    if (!ParseHashStr(vStrInputParts[0], txid)) {
249+        throw std::runtime_error("txid must be hexadecimal string (not '" + vStrInputParts[0] + "')");

practicalswift commented at 9:50 pm on September 24, 2018:

I know this is unchanged from the previous version, but could there be circumstances where this opens up for log file injection? Safer not to echo the input?

Empact commented at 4:24 am on September 25, 2018:

This had been changed so I switched it back.

practicalswift commented at 10:03 pm on September 24, 2018: contributor

What about adding NODISCARD (cherry-pick ca130493faed271bc05a2ad24db98a9cf167d159) to ParseHashStr to make sure all future callers check the return value?

DrahtBot removed the label Needs rebase on Sep 24, 2018

Empact force-pushed on Sep 25, 2018

Empact commented at 4:56 am on September 25, 2018: member

I like NODISCARD but am inclined to keep the changes separate for easier review. h/t #13815 I reverted the change that added new user input to the exception message. Now the log impact should be net 0.

Empact force-pushed on Sep 25, 2018

Empact commented at 5:26 am on September 25, 2018: member

@MarcoFalke added tests, only two of them fail before this change because of existing checks:

Tests the check for invalid txid valid hex, but too short
Tests the check for invalid txid valid hex, but too long

in src/core_io.h:37 in 909a8f6875 outdated

24@@ -25,7 +25,7 @@ std::string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDeco
25 bool DecodeHexTx(CMutableTransaction& tx, const std::string& hex_tx, bool try_no_witness = false, bool try_witness = true);
26 bool DecodeHexBlk(CBlock&, const std::string& strHexBlk);
27 bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
28-uint256 ParseHashStr(const std::string&, const std::string& strName);
29+bool ParseHashStr(const std::string& strHex, uint256& result);

l2a5b1 commented at 11:41 am on September 25, 2018:

Perhaps we can document bool ParseHashStr(const std::string& strHex, uint256& result) and specify the pre-condition that the strHex argument must be a hexadecimal string with a length of 64 bytes.

l2a5b1 commented at 11:49 am on September 25, 2018: contributor

utACK 909a8f6

Nice improvement!

Consolidate redundant implementations of ParseHashStr

This change:
* adds a length check to ParseHashStr, appropriate given its use to populate
  a 256-bit number from a hex str.
* allows the caller to handle the failure, which allows for the more
  appropriate JSONRPCError on failure in prioritisetransaction rpc

9c5af58d51

Empact force-pushed on Sep 25, 2018

Empact commented at 4:15 pm on September 25, 2018: member

Switched the condition and added docs.

l2a5b1 commented at 4:57 pm on September 25, 2018: contributor

re-utACK 9c5af58

Thanks for addressing the review comments @Empact!

MarcoFalke commented at 11:40 pm on September 25, 2018: member

utACK 9c5af58d51

in src/core_io.h:35 in 9c5af58d51

31+ * Parse a hex string into 256 bits
32+ * @param[in] strHex a hex-formatted, 64-character string
33+ * @param[out] result the result of the parasing
34+ * @returns true if successful, false if not
35+ *
36+ * @see ParseHashV for an RPC-oriented version of this

promag commented at 11:46 pm on September 25, 2018:

ParseHashV could use ParseHashStr?

Empact commented at 10:39 pm on September 26, 2018:

Happy to do that as a follow-up.

promag commented at 10:45 pm on September 26, 2018: member

utACK 9c5af58.

practicalswift commented at 3:46 am on September 27, 2018: contributor

utACK 9c5af58d51cea7d0cf2a598a9979eeba103b23ff

MarcoFalke merged this on Sep 27, 2018

MarcoFalke closed this on Sep 27, 2018

MarcoFalke referenced this in commit 01211cea71 on Sep 27, 2018

Empact deleted the branch on Dec 10, 2018

deadalnix referenced this in commit e3f2849c16 on Feb 18, 2020

ftrader referenced this in commit 569848a3af on May 19, 2020

pravblockc referenced this in commit a8f037c1a6 on Jul 30, 2021

pravblockc referenced this in commit ba2ab9029a on Aug 3, 2021

pravblockc referenced this in commit 67108d97c0 on Aug 4, 2021

pravblockc referenced this in commit 6cfb76a0c4 on Aug 4, 2021

pravblockc referenced this in commit 35e19528db on Aug 4, 2021

pravblockc referenced this in commit 59ae41853b on Aug 4, 2021

DrahtBot locked this on Sep 8, 2021

Consolidate redundant implementations of ParseHashStr #14307