Skip to content

Add overflow checks for int to bytes conversions #1860

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

Merged
merged 8 commits into from
May 10, 2019

Conversation

godlygeek
Copy link

@godlygeek godlygeek commented May 7, 2019

For both small and long integers, raise an exception if calling struct.pack, adding an element to an array.array, or formatting an int with int.to_bytes would overflow the requested size.

@godlygeek godlygeek force-pushed the int_to_bytes_overflow_checks branch from a67c61b to f9ca13a Compare May 8, 2019 06:12
@tannewt
Copy link
Member

tannewt commented May 8, 2019

Thanks! Please ping me when you are ready for a review.

@godlygeek
Copy link
Author

One of the Travis failures is for a board running out of space... I'll try implementing this without the intermediate bit_length calculation for big ints and see whether that gets it back in range or not...

@godlygeek godlygeek force-pushed the int_to_bytes_overflow_checks branch 3 times, most recently from f3b4d37 to 4c253af Compare May 9, 2019 05:22
Matt Wozniski added 4 commits May 9, 2019 03:22
For both small and long integers, raise an exception if calling
struct.pack, adding an element to an array.array, or formatting an int
with int.to_bytes would overflow the requested size.
Initially I used one of the existing exception raise functions, but it's
more correct and not much more code to raise OverflowError instead.
@godlygeek godlygeek force-pushed the int_to_bytes_overflow_checks branch from 4c253af to fb7bb40 Compare May 9, 2019 07:23
@godlygeek godlygeek changed the title WIP: Add overflow checks for int to bytes conversions Add overflow checks for int to bytes conversions May 9, 2019
@godlygeek
Copy link
Author

@tannewt I think this is ready for a review.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for this! One request to remove gotos, looks great otherwise!

Rather than jumping to a label when an overflow is known to have
occurred, return early when one is known not to have.
@godlygeek
Copy link
Author

@tannewt - goto's removed, please take another look.

tannewt
tannewt previously approved these changes May 10, 2019
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you so much!

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants