Consolidate redundant implementations of ParseHashStr #14307

Empact · 2018-09-24T15:08:28Z

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

maflcko

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?

maflcko · 2018-09-24T15:20:24Z

src/core_read.cpp

@@ -193,14 +193,13 @@ bool DecodePSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx,
    return true;
 }

-uint256 ParseHashStr(const std::string& strHex, const std::string& strName)
+bool ParseHashStr(const std::string& strReq, uint256& v)


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

practicalswift · 2018-09-24T15:41:46Z

src/core_read.cpp

 {
-    if (!IsHex(strHex)) // Note: IsHex("") is false
-        throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
+    if (!IsHex(strReq) || (strReq.size() != 64))


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

practicalswift · 2018-09-24T15:43:39Z

src/bitcoin-tx.cpp

-            uint256 txid = ParseHashStr(prevOut["txid"].get_str(), "txid");
+            uint256 txid;
+            if (!ParseHashStr(prevOut["txid"].get_str(), txid)) {
+                throw std::runtime_error("txid must be hexadecimal string (not '" + prevOut["txid"].get_str() + "')");


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 · 2018-09-24T15:45:47Z

src/rpc/mining.cpp

-    uint256 hash = ParseHashStr(request.params[0].get_str(), "txid");
+    uint256 hash;
+    if (!ParseHashStr(request.params[0].get_str(), hash)) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "txid must be hexadecimal string (not '" + request.params[0].get_str() + "')");


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 · 2018-09-24T15:46:24Z

Concept ACK

Thanks for taking on this issue. Closing #14288.

DrahtBot · 2018-09-24T16:36:48Z

Reviewers, this pull request conflicts with the following ones:

util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #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.

Empact · 2018-09-24T21:25:07Z

Rebased for #13424

Empact · 2018-09-24T21:49:59Z

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.

practicalswift · 2018-09-24T21:49:59Z

src/core_read.cpp

 {
-    if (!IsHex(strHex)) // Note: IsHex("") is false
-        throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
+    if (!IsHex(strHex) || (strHex.size() != 64))


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

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

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)

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

practicalswift · 2018-09-24T21:50:41Z

src/bitcoin-tx.cpp

-    uint256 txid(uint256S(strTxid));
+    uint256 txid;
+    if (!ParseHashStr(vStrInputParts[0], txid)) {
+        throw std::runtime_error("txid must be hexadecimal string (not '" + vStrInputParts[0] + "')");


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?

This had been changed so I switched it back.

practicalswift · 2018-09-24T22:03:21Z

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

Empact · 2018-09-25T04:56:57Z

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 · 2018-09-25T05:26:55Z

@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

l2a5b1

utACK 909a8f6

Nice improvement!

l2a5b1 · 2018-09-25T11:41:37Z

src/core_io.h

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


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 · 2018-09-25T11:41:59Z

src/core_read.cpp

 {
-    if (!IsHex(strHex)) // Note: IsHex("") is false
-        throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
+    if (!IsHex(strHex) || (strHex.size() != 64))


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)

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

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

Empact · 2018-09-25T16:15:51Z

Switched the condition and added docs.

l2a5b1 · 2018-09-25T16:57:53Z

re-utACK 9c5af58

Thanks for addressing the review comments @Empact!

maflcko · 2018-09-25T23:40:52Z

utACK 9c5af58

promag · 2018-09-25T23:46:32Z

src/core_io.h

+ * @param[out] result the result of the parasing
+ * @returns true if successful, false if not
+ *
+ * @see ParseHashV for an RPC-oriented version of this


ParseHashV could use ParseHashStr?

Happy to do that as a follow-up.

promag · 2018-09-26T22:45:15Z

utACK 9c5af58.

practicalswift · 2018-09-27T03:46:25Z

utACK 9c5af58

9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley) Pull request description: 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 Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53

Summary: 9c5af58d51 Consolidate redundant implementations of ParseHashStr (Ben Woosley) Pull request description: 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 Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53 Backport of Core [[bitcoin/bitcoin#14307 | PR14307]] bitcoin/bitcoin#14307 Depends on D5294 Test Plan: ninja check ninja check-functional Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5295

…ashStr 9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley) Pull request description: 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 bitcoin#14288 Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53

…shStr 9c5af58 Consolidate redundant implementations of ParseHashStr (Ben Woosley) Pull request description: 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 bitcoin#14288 Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53

maflcko added the Refactoring label Sep 24, 2018

maflcko reviewed Sep 24, 2018

View reviewed changes

practicalswift mentioned this pull request Sep 24, 2018

rest: Do not re-use the function name ParseHashStr (core_io.h) for a function with different behaviour in rest.cpp #14288

Closed

practicalswift reviewed Sep 24, 2018

View reviewed changes

Empact force-pushed the parse-hash-str branch from c76c7ad to dc95535 Compare September 24, 2018 15:44

practicalswift reviewed Sep 24, 2018

View reviewed changes

Empact force-pushed the parse-hash-str branch 2 times, most recently from e063b15 to a85bbe9 Compare September 24, 2018 15:55

This was referenced Sep 24, 2018

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

Merged

Consistently validate txid / blockhash length and encoding in rpc calls #13424

Merged

DrahtBot added the Needs rebase label Sep 24, 2018

Empact force-pushed the parse-hash-str branch from a85bbe9 to c5f066e Compare September 24, 2018 21:23

Empact force-pushed the parse-hash-str branch from c5f066e to 0b5791e Compare September 24, 2018 21:44

practicalswift reviewed Sep 24, 2018

View reviewed changes

DrahtBot removed the Needs rebase label Sep 24, 2018

Empact force-pushed the parse-hash-str branch 2 times, most recently from c60cfb7 to f78c166 Compare September 25, 2018 04:20

Empact force-pushed the parse-hash-str branch from f78c166 to 909a8f6 Compare September 25, 2018 05:24

l2a5b1 reviewed Sep 25, 2018

View reviewed changes

Empact force-pushed the parse-hash-str branch from 909a8f6 to 9c5af58 Compare September 25, 2018 16:15

promag reviewed Sep 25, 2018

View reviewed changes

maflcko merged commit 9c5af58 into bitcoin:master Sep 27, 2018

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

Empact deleted the parse-hash-str branch December 10, 2018 05:39

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Consolidate redundant implementations of ParseHashStr #14307

Consolidate redundant implementations of ParseHashStr #14307

Uh oh!

Conversation

Empact commented Sep 24, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Sep 24, 2018

Uh oh!

DrahtBot commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Sep 24, 2018

Uh oh!

Empact commented Sep 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Sep 25, 2018

Uh oh!

Empact commented Sep 25, 2018

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Sep 25, 2018

Uh oh!

l2a5b1 commented Sep 25, 2018

Uh oh!

maflcko commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 26, 2018

Uh oh!

practicalswift commented Sep 27, 2018

Uh oh!

Uh oh!

practicalswift Sep 24, 2018 •

edited

Loading

practicalswift Sep 24, 2018 •

edited

Loading

DrahtBot commented Sep 24, 2018 •

edited

Loading

practicalswift commented Sep 24, 2018 •

edited

Loading