-
Notifications
You must be signed in to change notification settings - Fork 47
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
Avoid heap out-of-bounds read in Node::CalcOps (test case: OP_0 OP_2 OP_EQUAL) and assertion failure in ComputeType (test case: OP_0 OP_0 OP_EQUAL) #57
Conversation
bitcoin/script/miniscript.h
Outdated
@@ -519,6 +519,7 @@ struct Node { | |||
next_sats.push_back(sats[sats.size() - 1] + sub->ops.sat); | |||
sats = std::move(next_sats); | |||
} | |||
assert(k < sats.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In c303afd, all the assertions should be k <= sats.size()
instead of k < sats.size()
as it's now possible and that's what you are checking for in the previous commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I should have been more careful here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And i should have had tests to my PR updating the thresh bounds :) #59 fixes this.
c303afd
to
bc1f2c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK bc1f2c7 modulo the silent int64 -> unsigned cast.
Here is a test exercising the two fixes: darosior@24a9e0b
…unds read in case of k > sats.size() Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
bc1f2c7
to
a47dcc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a47dcc6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK a47dcc6
utACK a47dcc6 |
Closes #12.
Closes #13.
Supercedes #18