py/objint: Fix int.to_bytes() buffer size checks. #13087
Merged
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
7473451 to
8e7dcac
Compare
|
Code size report: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13087 +/- ##
=======================================
Coverage 98.42% 98.42%
=======================================
Files 161 161
Lines 21234 21248 +14
=======================================
+ Hits 20900 20914 +14
Misses 334 334 ☔ View full report in Codecov by Sentry. |
d509c0b to
c97d353
Compare
4 tasks
262a9ab to
fe533a8
Compare
dc8c049 to
ed3a113
Compare
projectgus
commented
May 1, 2024
| int slen = MP_INT_REPR_LEN(val, val < 0); | ||
| memset(data, val < 0 ? 0xFF : 0x00, dlen); | ||
| if (slen <= dlen) { | ||
| mp_binary_set_int(slen, big_endian, data + (big_endian ? (dlen - slen) : 0), val); |
There was a problem hiding this comment.
For later consideration (not this PR): can mp_binary_set_val and this function be combined to save size?
| } else { | ||
| return mp_clzll(x); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't really like this approach but it seems the least bad that works across all configurations and isn't too intrusive.
The other way I thought to do it was to only define mp_clz32() and mp_clz64() and then have each port define MP_SIZEOF_INT or a similar macro.
|
@dpgeorge Finally found a combination that works across all ports, I think! |
|
We need to go with the fixes here, even though they are relatively large in size. Otherwise there are subtle bugs in the integer/byte conversion. |
Signed-off-by: Angus Gratton <angus@redyak.com.au>
Fixes and improvements to `int.to_bytes()` are: - No longer overflows if byte size is 0 (closes micropython#13041). - Raises OverflowError in any case where number won't fit into byte length (now matches CPython, previously MicroPython would return a truncated bytes object). - Document that `micropython int.to_bytes()` doesn't implement the optional signed kwarg, but will behave as if `signed=True` when the integer is negative (this is the current behaviour). Add tests for this also. Requires changes for small ints, MPZ large ints, and "long long" large ints. Adds a new set of unit tests for ints between 32 and 64 bits to increase coverage of "long long" large ints, which are otherwise untested. Tested on unix port (64 bit small ints, MPZ long ints) and Zephyr STM32WB board (32 bit small ints, long long large ints). This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
ed3a113 to
908ab1c
Compare
2 tasks
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.
This "little fix" ended up growing and growing, so this might be too much code size impact. In which case this should be closed. There's a much smaller patch which only fixes the first point from the list below.
int.to_bytes()doesn't implement the optionalsignedkwarg, but will behave as ifsigned=Truewhen the integer is negative (this is the current behaviour). Add tests for this also.Changes are implemented for small ints, MPZ large ints, and "long long" large ints.
Adds a new set of unit tests for ints between 32 and 64 bits to increase coverage of "long long" large ints, which are otherwise untested.
Tested on unix port (64 bit small ints, MPZ long ints) and Zephyr STM32WB board (32 bit small ints, long long large ints).
Untested on a port whose native format is big endian (don't have one at hand).
This work was funded through GitHub Sponsors.