fix: script number overflow check for push_int by ChrisCho-H · Pull Request #3392 · rust-bitcoin/rust-bitcoin

ChrisCho-H · 2024-09-20T03:08:01Z

Fix the issue #1530. In the discussion of #1530, the suggested solution is to implement ScriptInt type to embrace the various type of integer(i32, u32, i64, u64, i128, u128, isize, usize...) to support both script number and locktime number.

However, as push_locktime and push_sequence implemented, there’s no need to support u32 of lock time for push_int anymore. Therefore, I’ve just changed the type of parameter to i32, and only check if it’s i32::MIN(which overflows 4 bytes sign-magnitude integer).

~~I also added push_uint method to use internally for push_locktime and push_sequence, which have a dependency on push_int method.~~

UPDATE: also add push_int_unchecked for those who want to push the integer out of range(and helper for push_locktime and push_sequence, which has the same functionality of former push_int.

coveralls · 2024-09-20T03:10:20Z

Pull Request Test Coverage Report for Build 11025652486

Details

19 of 26 (73.08%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.02%) to 82.848%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/blockdata/script/builder.rs	5	12	41.67%

Totals
Change from base Build 11023696523:	-0.02%
Covered Lines:	19017
Relevant Lines:	22954

💛 - Coveralls

tcharding · 2024-09-20T03:50:07Z

Patch 1 and 2 need to be combined mate, otherwise they won't build individually.

ChrisCho-H · 2024-09-20T04:43:43Z

Squashed commits!

tcharding

Thanks for your contribution, I had a play with it.

tcharding · 2024-09-20T05:43:54Z

bitcoin/src/blockdata/script/builder.rs

Perhaps this function is cleaner if written like this?

/// Adds instructions to push an integer onto the stack. /// /// Integers are encoded as little-endian signed-magnitude numbers, but there are dedicated /// opcodes to push some small integers. /// /// # Errors /// /// Only errors if `data == i32::MIN` (CScriptNum cannot have value -2^31). pub fn push_int(self, data: i32) -> Result<Builder, Error> { if data == i32::MIN { Err(Error::NumericOverflow) } else if data == -1 { Ok(sef.push_opcode(OP_PUSHNUM_NEG1)) } else if data >= 0 { Ok(self.push_uint(data as u32)) } else { Ok(self.push_int_non_minimal(data.into())) } }

Its up to you but I'd personally order the functions push_int, push_uint, and then push_int_non_minimal.

Also if we are touching these functions perhaps we should re-name the parameter, data does not convey much info. n is the name that jumps out at me but some folk don't like short names, is there any other conventional name we could use @apoelstra?

That's a great retouch! I will reflect it when the new name of param is ready.

n is fine for a number.

tcharding · 2024-09-20T17:50:21Z

Thanks for sticking with this man! Sorry to be a nuisance but can you change the patch series again. We use a system whereby after review, if changes need to be made, one should rebase and redo the patch series putting review suggestions in the original patch where it was suggested. The aim being that the PR (patch series) is always a series of discreet steps to create the final state. A patch later in the series should not change things done in an earlier patch. This system is designed to optimize for reviewer time at the cost of developer time based on the observation that in open source projects reviewer time is much more scarce.

And each patch should be a single discreet change, this makes it much easier to review, and more likely that reviewers will catch bugs.

So looking at all changes in this PR together, one strategy to break it up might be:

Do the push_int() to push_locktime() change
Introduce the new push_int function (and push_uint)

tcharding

And some more little suggestions.

tcharding · 2024-09-20T17:50:43Z

bitcoin/src/address/mod.rs

We could use push_uint here and not use unwrap

This file is out of blockdata so cannot use push_uint

tcharding · 2024-09-20T17:50:51Z

bitcoin/src/address/mod.rs

tcharding · 2024-09-20T17:51:09Z

bitcoin/src/blockdata/constants.rs

Suggested change

.push_int(486604799)

.unwrap()

.push_uint(486604799)

tcharding · 2024-09-20T17:52:23Z

bitcoin/src/blockdata/script/builder.rs

Maybe some sort of code comment here with a reference if you have one to spare, I don't have one, only what I read Sanket write in the issue. No stress if you don't find one though.

I've added a reference https://github.com/bitcoin/bitcoin/blob/cac846c2fbf6fc69bfc288fd387aa3f68d84d584/src/script/script.h#L230

ChrisCho-H · 2024-09-21T10:52:21Z

No problem! Reflect suggestions and rearrange the patches.

apoelstra · 2024-09-21T13:38:55Z

Reviewed 44105778e9a6c9530d3bb140c61d1e010e1f8c88. The code looks good but I'm confused as to what the intended semantics are:

In push_int you reject i32::MIN, presumably because this value wouldn't be accepted by numeric opcodes;
but in push_uint you accept numbers above i32::MAX even though those would not be accepted by numeric opcodes;
and there is no longer any way at all to push numbers at i32::MAX or below, making it impossible to do comparisons with the output of numeric opcodes

ChrisCho-H · 2024-09-21T16:00:08Z

push_uint is only for the special case of locktime number, which is allowed upto u32::MAX.

and there is no longer any way at all to push numbers at i32::MAX or below, making it impossible to do comparisons with the output of numeric opcodes

If "i32::MAX or below" means n <= i32::MAX, it can be pushed with push_int. I'm confused what this statement exactly means.

apoelstra · 2024-09-21T16:45:35Z

push_uint is only for the special case of locktime number, which is allowed upto u32::MAX.

We already have push_lock_time for this. Did you mean for push_uint to be private?

If "i32::MAX or below" means n <= i32::MAX, it can be pushed with push_int. I'm confused what this statement exactly means.

It cannot, because push_int now takes an i32.

ChrisCho-H · 2024-09-22T01:37:00Z

I originally meant to make it private(used as a helper method for push_lock_time andpush_sequence), but changed mind to make it public because someone might want to use u32 directly. However, if you think it's confusing I don't mind to make it private.

Do you mean n <= i32::MIN(not i32::MAX)? If so, yes. There's no way to push, but script operands must be in the range [-2^31 +1...2^31 -1]. I'm not sure about this but the overflown output should be also in the range [-2^31 +1...2^31 -1], so it's possible to compare the overflown output also within that range. Anyway it won't be valid to compare. In bitcoin core, it states "but results may overflow (and are valid as long as they are not used in a subsequent numeric operation)".

apoelstra · 2024-09-22T12:56:59Z

I'm not sure about this but the overflown output should be also in the range

They will not be. What if you ADD the maximum (or minimum) value of the range to itself.

Anyway it won't be valid to compare

What does this mean? The comparison is meaningful and can be implemented in Script.

ChrisCho-H · 2024-09-22T16:33:48Z

In Bitcoin core CScriptNum, it says

Numeric opcodes (OP_1ADD, etc) are restricted to operating on 4-byte integers.
The semantics are subtle, though: operands must be in the range [-2^31 +1...2^31 -1],
but results may overflow (and are valid as long as they are not used in a subsequent
numeric operation)

If the result overflows, it cannot be used in a subsequent numeric operation. Isn't the comparison(e.g. OP_NUMEQUAL) also numeric ops?

apoelstra · 2024-09-22T17:52:16Z

Yes, NUMEQUAL is a numeric op. EQUAL is not.

ChrisCho-H · 2024-09-22T18:00:02Z

Oh sorry I didn't know that case.

In that case, isn't push_slice an option to use as it's arbitrary data rather than number?

apoelstra · 2024-09-22T19:47:04Z

Yes, somebody could just use push_slice and manually construct the encoding for a scriptnum. But since we have existing code to do this (and we have released many versions that expose that functionality), I'd like a good reason to get rid of it. Removing it does not look like the conclusion of #1530 to me.

ChrisCho-H · 2024-09-23T03:02:37Z

I think it's just dangerous to allow push_int to push the integer out of range which is not valid for the numeric operation(as it's actually read as data, not integer), and the conclusion of #1530 seemed to be unnecessary as push_locktime and push_sequence introduced... However, as you said, for the compatibility it needs to support 5 bytes integer.

Personally I don't think the conclusion is safe enough to prevent user from pushing the wrong integer, because if i64 type could be used for ScriptInt, docs might warn user but still could lead to misunderstand 5 bytes integer is valid for numeric ops. While it might break the compatibility, I think it's worth getting rid of i64 option for push_int.

Or we may add new method like push_int_safe(naming is just temp) which strictly support only valid integer(i32 without i32::MIN) for numeric ops, to clearly notify user the safe integer(while maintains the compatibility for push_int as untouched).

apoelstra · 2024-09-23T16:27:12Z

This all seems reasonable to me. But then push_uint needs to also return a result since it currently allows pushing out-of-range valus.

I think we could call push_int_unsafe (we should always give the long name to the bad one) something wordy like push_script_encoded_int and maybe also give it some flags so the user can specify non-minimal encodings or other pathological things.

ChrisCho-H · 2024-09-24T01:04:43Z

I've updated with some changes, which has the followings for what we want to achieve.
commit 1100de47bb893407bd2380433460085d4388c282

push_int checks overflow so that only the integer in range can be pushed.
push_uint is private to user, just helper method for the others(push_locktime, push_sequence...) .

commit 8c7ab24f50ec667c0097d3e759f96f9baba7d336

add push_int_unchecked_overflow for those who want to push the integer out of range, which has the same functionality of former push_int.

apoelstra · 2024-09-24T12:29:35Z

bitcoin/src/blockdata/constants.rs

In 1100de47bb893407bd2380433460085d4388c282:

We should just use unwrap here and make push_uint pub(super). In general I am suspicious of pub(in module) because it suggests there is some abstraction failure. In this case it's just to save an unwrap call which is guaranteed to be optimized away.

Alternately we can use push_int_unchecked

I've removed push_uint and changed every push_int to push_int_unchecked!

apoelstra · 2024-09-24T12:31:42Z

bitcoin/src/blockdata/script/builder.rs

In 1100de47bb893407bd2380433460085d4388c282:

Why not call this method push_int_unchecked and have it take an i64? This will avoid a cast and avoid an entire method definition. Plus it could be pub and avoid any thinking about module visibilty.

I couldn't come up with this idea as constantly change the code. Thanks for good catch!

ChrisCho-H · 2024-09-24T14:30:35Z

c7067c88760f7d7a1d251e2ac5d6e04b56f687f5

rename push_int_unchecked_overflow -> push_int_unchecked
remove push_uint at all. It's redundant as push_int_unchecked can totally replace what push_uint can.

tcharding

ACK c7067c88760f7d7a1d251e2ac5d6e04b56f687f5

tcharding · 2024-09-24T23:23:54Z

CI fail is fixed on master now, just need to rebase this PR.

Also I added #3407 so as to not hold up your PR, the current state is already good and is a good improvement, thanks. Feel free to do #3407, to nack #3407, or to ignore it completely :)

ChrisCho-H · 2024-09-25T03:22:46Z

Rebased 33edaf9!

tcharding

ACK 33edaf9

apoelstra

ACK c7067c88760f7d7a1d251e2ac5d6e04b56f687f5 successfully ran local tests

…erflow a33bcd3 test: ensure push_int check i32::MIN of overflow error (Chris Hyunhum Cho) c9988ba refactor: use match for OP_N push in push_int_unchecked (Chris Hyunhum Cho) Pull request description: Follow up #3392 c9988ba - refactor `push_int_unchecked` with match expression for cleaner code(many thanks for tcharding #3407). a33bcd3 - ensure newly introduced safe `push_int` function as expected, testing if returns `Error::NumericOverflow` when `n` is `i32::MIN` ACKs for top commit: tcharding: ACK a33bcd3 apoelstra: ACK a33bcd3 successfully ran local tests Tree-SHA512: 14f19d37f35b47e148b40c5017f0270c534c136d86be0c061cb476e1693130c5fc1bfc45a6f7c75a473022490c5f4e061cbc02640b1a616619ae721116e3cd54

github-actions bot added C-bitcoin PRs modifying the bitcoin crate test labels Sep 20, 2024

ChrisCho-H force-pushed the fix/script-num-overflow branch from 5a5dc84 to 48fc158 Compare September 20, 2024 04:42

ChrisCho-H force-pushed the fix/script-num-overflow branch from 48fc158 to 4491e04 Compare September 20, 2024 05:09

tcharding reviewed Sep 20, 2024

View reviewed changes

ChrisCho-H force-pushed the fix/script-num-overflow branch from d6f28ca to 7f04560 Compare September 21, 2024 10:49

ChrisCho-H force-pushed the fix/script-num-overflow branch from 7f04560 to 4410577 Compare September 21, 2024 10:56

ChrisCho-H force-pushed the fix/script-num-overflow branch 4 times, most recently from 3f36913 to 034e99e Compare September 24, 2024 00:53

ChrisCho-H force-pushed the fix/script-num-overflow branch from 034e99e to 8c7ab24 Compare September 24, 2024 11:26

apoelstra reviewed Sep 24, 2024

View reviewed changes

ChrisCho-H force-pushed the fix/script-num-overflow branch 2 times, most recently from 25a71b7 to c7067c8 Compare September 24, 2024 14:23

tcharding mentioned this pull request Sep 24, 2024

Follow up #3392 (refactor) #3407

Closed

tcharding previously approved these changes Sep 24, 2024

View reviewed changes

ChrisCho-H dismissed tcharding’s stale review via c7067c8 September 25, 2024 02:40

ChrisCho-H force-pushed the fix/script-num-overflow branch from acd4716 to c7067c8 Compare September 25, 2024 02:40

ChrisCho-H added 2 commits September 25, 2024 03:07

refactor: use push_lock_time instead of push_int

8761461

fix: check overflow for push_int with push_int_unchecked

33edaf9

ChrisCho-H force-pushed the fix/script-num-overflow branch from c7067c8 to 33edaf9 Compare September 25, 2024 03:15

tcharding approved these changes Sep 25, 2024

View reviewed changes

apoelstra approved these changes Sep 25, 2024

View reviewed changes

apoelstra merged commit 3bbad77 into rust-bitcoin:master Sep 25, 2024

ChrisCho-H mentioned this pull request Sep 26, 2024

script: refactor push_int_unchecked and test push_int overflow #3411

Merged

liamaharon mentioned this pull request Feb 4, 2026

script::Builder::push_int panics #4499

Open

Conversation

ChrisCho-H commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 11025652486

Details

💛 - Coveralls

Uh oh!

tcharding commented Sep 20, 2024

Uh oh!

ChrisCho-H commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 20, 2024

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisCho-H Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisCho-H commented Sep 21, 2024

Uh oh!

apoelstra commented Sep 21, 2024

Uh oh!

ChrisCho-H commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 21, 2024

Uh oh!

ChrisCho-H commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 22, 2024

Uh oh!

ChrisCho-H commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 22, 2024

Uh oh!

ChrisCho-H commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 22, 2024

Uh oh!

ChrisCho-H commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisCho-H commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisCho-H Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ChrisCho-H commented Sep 20, 2024 •

edited

Loading

coveralls commented Sep 20, 2024 •

edited

Loading

ChrisCho-H commented Sep 20, 2024 •

edited

Loading

ChrisCho-H Sep 21, 2024 •

edited

Loading

ChrisCho-H commented Sep 21, 2024 •

edited

Loading

ChrisCho-H commented Sep 22, 2024 •

edited

Loading

ChrisCho-H commented Sep 22, 2024 •

edited

Loading

ChrisCho-H commented Sep 22, 2024 •

edited

Loading

ChrisCho-H commented Sep 23, 2024 •

edited

Loading

apoelstra commented Sep 23, 2024 •

edited

Loading

ChrisCho-H commented Sep 24, 2024 •

edited

Loading

ChrisCho-H Sep 24, 2024 •

edited

Loading

ChrisCho-H commented Sep 24, 2024 •

edited

Loading