Fix assert failure in miniscript string parsing #26149
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@darosior FWIW, my thinking with this code was incorrectly assuming that thresh's k couldn't be larger than 20, like multi(). That's obviously false. The 0x7fff and 0x7fffff conditions can't actually be hit (they'd immediately cause the script size.to exceed the limit) but I've included them here for consistency. |
|
utACK 648f695 I checked it's the only place we missed it. |
amovfx
reviewed
Sep 21, 2022
| @@ -1221,7 +1221,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx) | |||
| // n = 1 here because we read the first WRAPPED_EXPR before reaching THRESH | |||
| to_parse.emplace_back(ParseContext::THRESH, 1, k); | |||
| to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); | |||
| script_size += 2 + (k > 16); | |||
| script_size += 2 + (k > 16) + (k > 0x7f) + (k > 0x7fff) + (k > 0x7fffff); | |||
There was a problem hiding this comment.
Reviewed and built locally. Checks all pass.
This hex notation is new to me. I read that The 0x7FFF notation is much more clear about potential over/underflow than the decimal notation.
Is this why it is used? A more safe representation of an int?
Also, this might not be the right place to ask. Should I push these questions to stack exchange?
There was a problem hiding this comment.
I use the hex notation here because it's much more obvious. People might not recognize 32767 as the largest 16-bit signed integer, or 16777215 as the largest 24-bit signed integer; but in the hex notation this is immediately clear.
It's just more readable to reviewers.
|
ACK 648f695 |
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Sep 22, 2022
GitHub-Pull: bitcoin#26149 Rebased-From: 648f695
|
Added to #26133 for backporting to 24.x. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 23, 2022
648f695 Correct sanity-checking script_size calculation (Pieter Wuille) Pull request description: Fix a bug in the script_size sanity-check in the miniscript string parser, found by oss-fuzz in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51636, and introduced in e8cc2e4 (bitcoin#25540). This bug would cause an assertion failure when feeding a miniscript with a `thresh(k,...)` fragment, with k >= 128, to an RPC. ACKs for top commit: darosior: utACK 648f695 achow101: ACK 648f695 Tree-SHA512: d86a0721758cd1e42ef02050b542f0935efdc19447a1ca76a3ade96352a6ee8261eef3d4a5cbdec77bf0ad14dfed42e9eb6bd4246b816a9f6f06d786900da9e7
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Sep 26, 2022
GitHub-Pull: bitcoin#26149 Rebased-From: 648f695
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Sep 29, 2022
GitHub-Pull: bitcoin#26149 Rebased-From: 648f695
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Oct 11, 2022
GitHub-Pull: bitcoin#26149 Rebased-From: 648f695
achow101
added a commit
that referenced
this pull request
Oct 13, 2022
e2e4c29 tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow) 4d42c3a psbt: Only include m_tap_tree if it has scripts (Andrew Chow) d810fde psbt: Change m_tap_tree to store just the tuples (Andrew Chow) a9419ef tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow) 4abd2ab psbt: Fix merging of m_tap_tree (Andrew Chow) 1390c96 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin) 9b438f0 refactor: revert m_next_resend to not be std::atomic (stickies-v) 43ced0b wallet: only update m_next_resend when actually resending (stickies-v) fc8f2bf refactor: carve out tx resend timer logic into ShouldResend (stickies-v) a6fb674 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v) 5ad82a0 index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky) 997faf6 contrib: Fix capture_output in getcoins.py (willcl-ark) 7e0bcfb p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane) c97d924 Correct sanity-checking script_size calculation (Pieter Wuille) da6fba6 docs: Add 371 to bips.md (Andrew Chow) Pull request description: Will collect backports for rc2 as they become available. Currently: * #25858 * #26124 * #26149 * #26172 * #26205 * #26212 * #26215 ACKs for top commit: dergoegge: ACK e2e4c29 achow101: ACK e2e4c29 instagibbs: ACK e2e4c29 Tree-SHA512: b6374fe202561057dbe1430d4c40f06f721eb568f91e7275ae1ee7747edf780ce64620382d13ecc4b9571d931dc25d226af8284987cf35ff6a6182c5f64eb10c
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a bug in the script_size sanity-check in the miniscript string parser, found by oss-fuzz in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51636, and introduced in e8cc2e4 (#25540).
This bug would cause an assertion failure when feeding a miniscript with a
thresh(k,...)fragment, with k >= 128, to an RPC.