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

bitcoin-tx: Avoid treating integer overflow as OP_0 #23227

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 8, 2021

Seems odd to treat integer overflow as OP_0, so fix that.

@maflcko maflcko changed the title bitcoin-tx: Avoid treating overflow as OP_0 bitcoin-tx: Avoid treating integer overflow as OP_0 Oct 8, 2021
@practicalswift
Copy link
Contributor

Concept ACK

Nice to see LocaleIndependentAtoi be replaced by ToIntegral too.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 8, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22875 (Fix Racy ParseOpCode function initialization by JeremyRubin)

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.

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa0c2f9

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept and code-review ACK fa0c2f9

src/core_read.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK fa43e7c

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa43e7c

This PR’s primary focus is to avoid treating an overflow integer value as output 0.

This is done by using the ToIntegral function that returns the integer value of the number if it is in the int_64 range; otherwise, it produces a nullopt. Followed by this, a check is applied to a returned value which checks if it has a valid value. And return an error in case if its not.

This PR also deals with further minors changes, which are:

  • Appropriate additions to the comments in strencodings.h and lint-locale-dependence.sh files.
  • Using C++11 range based indicator in ParseScript function.
  • Fixing whitespaces in ParseScript function.

I agree with the above changes, and I think they are necessary and valuable.

I would like to suggest one addition, though. In case you have to update the PR for some major reason, how about also adding the test for negative -999….999 (i.e., huge number)

  { "exec": "./bitcoin-tx",
    "args": ["-create", "outscript=0:-999999999999999999999999999999"],
    "return_code": 1,
    "error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF",
    “description”: “Try to parse an output script with a decimal number below the allowed range.”
  },

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK fa43e7c

@meshcollider meshcollider merged commit fbbbc59 into bitcoin:master Oct 12, 2021
@maflcko maflcko deleted the 2110-ToIntegral branch October 12, 2021 07:03
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 12, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants