Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 24, 2018

Conversation

Projects
None yet
6 participants
@Empact
Copy link
Member

commented Jun 8, 2018

ParseHashV validates the length and encoding of the string and throws
an informative RPC error on failure, which is as good or better than
these alternative calls.

Note I switched ParseHashV to check string length first, because
IsHex tests that the length is even, and an error like:
"must be of length 64 (not 63, for X)" is much more informative than
"must be hexadecimal string (not X)" in that case.

Split from #13420

src/rpc/mining.cpp Outdated
@@ -247,7 +247,7 @@ static UniValue prioritisetransaction(const JSONRPCRequest& request)

LOCK(cs_main);

uint256 hash = ParseHashStr(request.params[0].get_str(), "txid");
uint256 hash = ParseHashV(request.params[0], "txid");

This comment has been minimized.

Copy link
@Empact

Empact Jun 8, 2018

Author Member

Note this trades ParseHashStr's std::runtime_error-based encoding-only test for ParseHashV's JSONRPCError-based length and encoding tests.

@Empact Empact force-pushed the Empact:parse-hash-v branch 2 times, most recently Jun 8, 2018

@fanquake fanquake added the Refactoring label Jun 9, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Jun 10, 2018

How about getchaintxstats?

uint256 hash = uint256S(request.params[1].get_str());

And there may be other examples.

Concept ACK.

@Empact Empact force-pushed the Empact:parse-hash-v branch 4 times, most recently Jun 10, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

Thanks @promag, I audited uint256S use and found 4 more cases. Also added more testing wherever the test outcomes changed.

@Empact Empact force-pushed the Empact:parse-hash-v branch Jun 11, 2018

@Empact Empact changed the title Consistently use ParseHashV to validate hash inputs in rpc Validate txid / blockhash length and encoding separately from validity/presence in rpc calls Jun 11, 2018

@Empact Empact changed the title Validate txid / blockhash length and encoding separately from validity/presence in rpc calls Validate txid / blockhash length and encoding separately from validity / presence in rpc calls Jun 11, 2018

@Empact Empact changed the title Validate txid / blockhash length and encoding separately from validity / presence in rpc calls Validate txid / blockhash length and encoding separate from validity / presence in rpc calls Jun 11, 2018

@Empact Empact force-pushed the Empact:parse-hash-v branch Jun 11, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

Made the get_str call guaranteed, which means non-string values fail with -1, "JSON value is not a string as expected", rather than a length failure.

@Empact Empact changed the title Validate txid / blockhash length and encoding separate from validity / presence in rpc calls Consistently validate txid / blockhash length and encoding in rpc calls Jun 11, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14307 (Consolidate redundant implementations of ParseHashStr by Empact)
  • #12153 (Avoid permanent cs_main lock in getblockheader by promag)
  • #10973 (Refactor: separate wallet from node by ryanofsky)

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

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

The last travis run for this pull request was 52 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot closed this Aug 2, 2018

@DrahtBot DrahtBot reopened this Aug 2, 2018

@kallewoof
Copy link
Member

left a comment

utACK

Small nit: you're mixing uint256 hash = ParseHashV(...) and uint256 hash(ParseHashV(...)). Would be cool to do only one (latter one looks better IMO but either works).

Consistently use ParseHashV to validate hash inputs in rpc
ParseHashV validates the length and encoding of the string and throws
an informative RPC error on failure, which is as good or better than
these alternative calls.

Note I switched ParseHashV to check string length first, because
IsHex tests that the length is even, and an error like:
"must be of length 64 (not 63, for X)" is much more informative than
"must be hexadecimal string (not X)"

@Empact Empact force-pushed the Empact:parse-hash-v branch to 5eb20f8 Aug 7, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

@kallewoof fair enough since I'm touching those lines anyway. Updated.

@kallewoof
Copy link
Member

left a comment

re-utACK 5eb20f8

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

utACK 5eb20f8

@MarcoFalke MarcoFalke merged commit 5eb20f8 into bitcoin:master Sep 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 24, 2018

Merge #13424: Consistently validate txid / blockhash length and encod…
…ing in rpc calls

5eb20f8 Consistently use ParseHashV to validate hash inputs in rpc (Ben Woosley)

Pull request description:

  ParseHashV validates the length and encoding of the string and throws
  an informative RPC error on failure, which is as good or better than
  these alternative calls.

  Note I switched ParseHashV to check string length first, because
  IsHex tests that the length is even, and an error like:
  "must be of length 64 (not 63, for X)" is much more informative than
  "must be hexadecimal string (not X)" in that case.

  Split from #13420

Tree-SHA512: f0786b41c0d7793ff76e4b2bb35547873070bbf7561d510029e8edb93f59176277efcd4d183b3185532ea69fc0bbbf3dbe9e19362e8017007ae9d51266cd78ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.