Consolidate redundant implementations of ParseHashStr #14307

pull Empact wants to merge 1 commits into bitcoin:master from Empact:parse-hash-str changing 6 files +87 −22
  1. 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

  2. MarcoFalke added the label Refactoring on Sep 24, 2018
  3. 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?

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

  5. 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 :-)

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

  7. Empact force-pushed on Sep 24, 2018
  8. 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?

  9. practicalswift commented at 3:46 PM on September 24, 2018: contributor

    Concept ACK

    Thanks for taking on this issue. Closing #14288.

  10. Empact force-pushed on Sep 24, 2018
  11. Empact force-pushed on Sep 24, 2018
  12. DrahtBot commented at 4:36 PM on September 24, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Reviewers, this pull request conflicts with the following ones:

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

  13. DrahtBot added the label Needs rebase on Sep 24, 2018
  14. Empact force-pushed on Sep 24, 2018
  15. Empact commented at 9:25 PM on September 24, 2018: member

    Rebased for #13424

  16. Empact force-pushed on Sep 24, 2018
  17. 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.

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

    if ((strHex.size() != 64) || !IsHex(strHex)) return false;
  19. 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.

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

  21. DrahtBot removed the label Needs rebase on Sep 24, 2018
  22. Empact force-pushed on Sep 25, 2018
  23. Empact force-pushed on Sep 25, 2018
  24. 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.

  25. Empact force-pushed on Sep 25, 2018
  26. 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
  27. 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.

  28. l2a5b1 commented at 11:49 AM on September 25, 2018: contributor

    utACK 909a8f6

    Nice improvement!

  29. 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
  30. Empact force-pushed on Sep 25, 2018
  31. Empact commented at 4:15 PM on September 25, 2018: member

    Switched the condition and added docs.

  32. l2a5b1 commented at 4:57 PM on September 25, 2018: contributor

    re-utACK 9c5af58

    Thanks for addressing the review comments @Empact!

  33. MarcoFalke commented at 11:40 PM on September 25, 2018: member

    utACK 9c5af58d51

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

  35. promag commented at 10:45 PM on September 26, 2018: member

    utACK 9c5af58.

  36. practicalswift commented at 3:46 AM on September 27, 2018: contributor

    utACK 9c5af58d51cea7d0cf2a598a9979eeba103b23ff

  37. MarcoFalke merged this on Sep 27, 2018
  38. MarcoFalke closed this on Sep 27, 2018
  39. MarcoFalke referenced this in commit 01211cea71 on Sep 27, 2018
  40. Empact deleted the branch on Dec 10, 2018
  41. deadalnix referenced this in commit e3f2849c16 on Feb 18, 2020
  42. ftrader referenced this in commit 569848a3af on May 19, 2020
  43. pravblockc referenced this in commit a8f037c1a6 on Jul 30, 2021
  44. pravblockc referenced this in commit ba2ab9029a on Aug 3, 2021
  45. pravblockc referenced this in commit 67108d97c0 on Aug 4, 2021
  46. pravblockc referenced this in commit 6cfb76a0c4 on Aug 4, 2021
  47. pravblockc referenced this in commit 35e19528db on Aug 4, 2021
  48. pravblockc referenced this in commit 59ae41853b on Aug 4, 2021
  49. DrahtBot locked this on Sep 8, 2021
Contributors
EmpactMarcoFalkepracticalswiftDrahtBotl2a5b1promag
Labels
Refactoring
Linked (view graph)
#14288 rest: Do not re-use the function name ParseHashStr (core_io.h) for a function with different behaviour in rest.cpp
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: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me