Fix wrap around issues on large numerics#543
Fix wrap around issues on large numerics#543jlusiardi wants to merge 4 commits intoultrajson:mainfrom jlusiardi:issue_440
Conversation
There seemed to be a mistake on LLONG_MAX vs ULLONG_MAX (the latter is the maximum value for an object of type `unsigned long long int` which JSUINT64 should be). I fixed that. I am unsure about how much performance is lost on the check for overflow on digit check...
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
+ Coverage 91.76% 91.77% +0.01%
==========================================
Files 6 6
Lines 1821 1824 +3
==========================================
+ Hits 1671 1674 +3
Misses 150 150 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bwoodsend
left a comment
bwoodsend
left a comment
There was a problem hiding this comment.
Bear with me here - this stuff makes my head hurt...
| { | ||
| return SetError(ds, -1, overflowLimit == LLONG_MAX ? "Value is too big!" : "Value is too small"); | ||
| return SetError(ds, -1, overflowLimit == LLONG_MAX ? "Value is too big" : "Value is too small"); | ||
| } |
There was a problem hiding this comment.
Is the overflowLimit == LLONG_MAX ? ... switch not redundant because of the if (intNeg == 1) and else if (intNeg == -1) conditions above it?
There was a problem hiding this comment.
(P.S. If we're ditching !s in error messages then there's that one still lurking on line 170.)
| case '9': | ||
| { | ||
| // detect overflow on digit shift | ||
| if ((intValue * 10ULL) % 10 != 0) |
There was a problem hiding this comment.
Would if (intValue > ULLONG_MAX / 10) not be equivalent? It's less headache inducing and probably fractionally more performant since the ULLONG_MAX / 10 will be evaluated at compile time leaving just the > operation at runtime.
There was a problem hiding this comment.
Yeah, I think that's functionally identical. In both cases, an intValue of ULLONG_MAX / 10 = 1844674407370955161 would pass the check, as it should since it does not overflow on the multiplication. It might still overflow on the addition if the next digit is 6 to 9, but that would be caught in the check below. And intValue = ULLONG_MAX / 10 + 1 would fail this added check in both forms.
JustAnotherArchivist
left a comment
JustAnotherArchivist
left a comment
There was a problem hiding this comment.
Likewise, integer arithmetic is bad enough as it is, but decoding from a string definitely induces headaches.
| { | ||
| hasError = 1; | ||
| } | ||
| else if (intNeg == -1 && intValue > overflowLimit) |
There was a problem hiding this comment.
Doesn't the removal of this check mean that there is no test against the smaller overflowLimit of LLONG_MAX for negative numbers anymore? Then we could get an overflow further down in the BREAK_INT_LOOP section.
| @pytest.mark.parametrize( | ||
| "test_input", | ||
| [ | ||
| ("33333333303333333333"), | ||
| ("18446744073709551616"), # 64 bit | ||
| ("-18446744073709551616"), # 64 bit | ||
| ("-80888888888888888888"), | ||
| ], | ||
| ) | ||
| def test_decode_big_numeric(test_input): | ||
| with pytest.raises(ujson.JSONDecodeError): | ||
| ujson.loads(test_input) |
There was a problem hiding this comment.
This should be merged with test_decode_raises, which already has some tests for big ints. I have no strong opinion on whether it should all be in one test or not, though I tend towards yes (since the test code is basically identical, just with different input).
|
I'll close this PR and recommend looking at #544 instead. |
5 participants
There seemed to be a mistake on LLONG_MAX vs ULLONG_MAX (the latter is
the maximum value for an object of type
unsigned long long intwhichJSUINT64 should be). I fixed that.
I am unsure about how much performance is lost on the check for overflow
on digit check...
Fixes #440
Changes proposed in this pull request: