Fix: Use unsigned integer reading methods for PUSHDATA opcode lengths #19
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
This commit fixes a bug in the Script._build_chunks method where signed integer reading methods were being used for PUSHDATA opcode length values. Since these values should always be treated as unsigned, using signed integer methods could result in negative values when high bits are set, causing incorrect chunk parsing. Changed to use read_uint8, read_uint16_le, and read_uint32_le methods instead of read_int for PUSHDATA1, PUSHDATA2, and PUSHDATA4 opcodes respectively. Added tests to verify correct handling of large data lengths for all PUSHDATA opcodes.
ALiberalVoluntarist
requested review from
gitzhou and
voyager1708
and removed request for
gitzhou
voyager1708
approved these changes
Feb 28, 2025
Collaborator
voyager1708
left a comment
voyager1708
left a comment
There was a problem hiding this comment.
This pull request introduces several improvements and fixes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PUSHDATA opcode length parsing
Problem
The
Script._build_chunksmethod was usingread_intto parse the length bytes forPUSHDATA1, PUSHDATA2, and PUSHDATA4 opcodes. Since these values should always be treated
as unsigned integers, using a method that interprets them as signed could result in
negative values when the high bit is set. This would cause incorrect parsing of the
script chunks, potentially leading to runtime errors or incorrect script execution.
Solution
This PR modifies the
_build_chunksmethod to use the appropriate unsigned integerreading methods:
read_uint8()for PUSHDATA1read_uint16_le()for PUSHDATA2read_uint32_le()for PUSHDATA4This ensures that length values are always treated as unsigned, allowing for
the correct handling of large data chunks.
Testing
Added comprehensive tests that verify:
The tests ensure that scripts with PUSHDATA opcodes correctly maintain their integrity
through serialization and deserialization, and that chunk parsing works as expected.Retry
Checklist: