-
-
Notifications
You must be signed in to change notification settings - Fork 439
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
Fix unsoundness in <BlockRng64 as RngCore>:: next_u32 #1159
Conversation
Previously, certain implementations of `BlockRngCore` could cause `BlockRng64`'s implementation of `RngCore::next_u32` to exhibit undefined behavior (UB). This removes that UB by adding a bounds check, and adds documentation to `BlockRngCore::Results` clarifying requirements for that type. While we're here, run `rustfmt` on existing code. Fixes rust-random#1158
// return slices of the same length. Thus, it would be unsound to | ||
// assume that this index is in-bounds and to elide the bounds | ||
// check. | ||
results[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make all of the code safe at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can by getting u64
from the cached block, shifting it to 32 bits if one half was already used, and casting it to u32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the best approach. Maybe let's just convert index
to an index into the u64
slice and shift by 32 * self.half_used
bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #1160 for this.
I think we should also look into the performance impact. If it is big, we could introduce a breaking change (later, in another PR) and replace |
@vks |
@newpavlov I agree. I should have been more clear about this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, rand still doesn't use rustfmt
, but no worries.
This looks okay, but as said could probably be better still.
/// | ||
/// Code which uses this type is allowed to assume that `Results` is a type | ||
/// with at least one element. It may panic or exhibit incorrect behavior | ||
/// if this is not the case. It may *not* exhibit undefined behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes without saying, usually..
Fixed by #1160. |
Previously, certain implementations of
BlockRngCore
could causeBlockRng64
's implementation ofRngCore::next_u32
to exhibit undefined behavior (UB). This removes that UB by adding a bounds check, and adds documentation toBlockRngCore::Results
clarifying requirements for that type.While we're here, run
rustfmt
on existing code.Fixes #1158