Check overflow when casting floats to integers. #28
Conversation
This change adds some new macro rules used when converting from floats to integers. There are two macro rule variants, one for signed ints, one for unsigned ints. Among other things, this change specifically addresses the overflow case documented in rust-num#12
|
Ah, I see that CI is spinning against an version of rust which doesn't provide |
`num` is tested against `rust 1.8.0`, which doesn't include `assert_ne!` -- so we use a plain ol' `assert` instead.
|
As a general note, I'm currently a Google employee, which means that Google owns the copyright for this patch by default. There is a process internal to Google by which employees can ask Google to liberate the copyright -- I intend to start that process first thing on Monday. But I wanted to leave a comment here to let y'all know. I will comment here on the status of that process. Furthermore, I will not merge this PR without an explicit green light from y'all with respect to this issue -- I just want to be as upfront as possible about what's going on behind the scenes ^_^ |
|
RE: Google, we don't ask for copyright assignment, so as long as your contribution is compatible with the project licenses (MIT and Apache-2.0), I think we're fine. IANAL, of course -- please do confirm with Google if you're not sure about this yourself. |
There was a problem hiding this comment.
It appears you just worked from the float_to_float code, right? Unfortunately, the concerns are really different here. The only interesting case there is f64 to f32, and note that it does all the comparisons using the larger f64.
Did you see the former PR, rust-num/num#135? This was on the right track, just needed more work around the boundary conditions. (Bonus points if you actually cherry-pick those commits, so credit/authorship is given, then complete it with your own commits.)
src/cast.rs
Outdated
| macro_rules! impl_to_primitive_float_to_signed_int { | ||
| ($SrcT:ident, $DstT:ident, $slf:expr) => ( | ||
| if size_of::<$SrcT>() <= size_of::<$DstT>() { | ||
| Some($slf as $DstT) |
There was a problem hiding this comment.
Checking size_of doesn't help -- e.g. 4-byte f32::MAX is over 3e38, which will overflow 8-byte i64, even 16-byte i128, and only barely fits in u128. (We don't support 128-bit types anyway.)
src/cast.rs
Outdated
| Some($slf as $DstT) | ||
| } else { | ||
| // Make sure the value is in range for the cast. | ||
| let n = $slf as $DstT; |
There was a problem hiding this comment.
Note in the issue that I mentioned UB (undefined behavior) -- once you've done an overflowing cast, all bets are off when using that value.
src/cast.rs
Outdated
| // Make sure the value is in range for the cast. | ||
| let n = $slf as $DstT; | ||
| let max_value: $DstT = ::std::$DstT::MAX; | ||
| if -max_value as $DstT <= n && n <= max_value as $DstT { |
There was a problem hiding this comment.
By definition we know that $DstT::MIN <= n && n <= $DstT::MAX, because at this point it is a $DstT. The only loophole is that MIN == -MAX - 1, so you're actually excluding MIN as a legal value.
src/cast.rs
Outdated
| // Make sure the value is in range for the cast. | ||
| let n = $slf as $DstT; | ||
| let max_value: $DstT = ::std::$DstT::MAX; | ||
| if n <= max_value as $DstT { |
There was a problem hiding this comment.
Same comment about invoking UB and n <= MAX always being true. Plus there's nothing here that would direct a negative float to None.
src/cast.rs
Outdated
| // Specifically make sure we didn't silently let the overflow through | ||
| // (This line is mostly for humans -- the robots should catch any problem | ||
| // on the line above). | ||
| assert!(source.to_i32() != Some(-2147483648)); |
There was a problem hiding this comment.
As a human, I don't understand the point of this line. If it was equal to None above, then it won't be equal to any Some. Trust the "robots".
Also note that -2147483648 is exactly i32::MIN, which would be a perfectly legal value! It just happens that LLVM is giving you this value for overflow too, but that's UB.
There was a problem hiding this comment.
I figured it might be nice to have some visual correspondence between this test and the PR, but I agree that it's "nonsensical" otherwise -- we shouldn't ever get to this line. I've removed it.
|
Overall: Understood! It seems that the direction of both of the PRs is pretty similar, though they obviously differ in the core macro definition which checks for overflow. I'd love to cherry-pick the commits from @apopiak's PR, but I doubt I'll be able to apply them cleanly -- they were written to |
|
Addendum: I could alternately stitch @apopiak's changes in manually, and then commit those changes as |
|
Yes, |
|
Indeed, authorship attribution is the main thing I'm going for. I'll do that then. |
|
BTW, you might want to keep an eye on rust-lang/rust#47857. The current implementation isn't sound, and they're debating whether |
|
Affirmative, thanks for the heads up! I'll comment there and see what I can contribute. I spent a little more time today hacking on this PR, nothing major. It's non-functional right now, so don't worry about reviewing anything new yet. Among other things, I added apopiak@'s changes in using |
|
I've continued this in #52 -- can you take a look? |
52: Refactor ToPrimitive range checks r=cuviper a=cuviper This is a rebase and continuation of PR #28. The primary benefit is that floats finally check for overflow before casting to integers, avoiding undefined behavior. Fixes #12. The inter-integer conversions and all of the macros for these have also been tweaked, hopefully improving readability. Exhaustive tests have been added for good and bad conversions around the target MIN and MAX values.
|
I merged #52, but further code reviews are welcome at any time! |
This change adds some new macro rules used when converting from floats
to integers. There are two macro rule variants, one for signed ints, one
for unsigned ints.
Among other things, this change specifically addresses the overflow case
documented in #12