- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 448
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
, with less unsafe
code
#1160
Conversation
Many thanks to @joshlf for reporting this issue. Fixes rust-random#1158.
<BlockRng64 as RngCore>::next_u32
<BlockRng64 as RngCore>::next_u32
, with less unsafe
code
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.
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.
Corrections needed, and I would like to see before/after benchmarks, though I expect perf is fine.
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 the code is fine, but the test is not.
It would still be nice to see before/after benches, though considering we only have one obscure RNG affecting by this, maybe nobody cares.
In fact, is there a good rationale for keeping this code?
I did not measure a significant performance difference. Before:
After:
|
Both libraries look obscure and haven't been touched in a couple of years.
Anyone else care to comment (@newpavlov)? Perhaps we should move |
I think we should simply create dummy 32 and 64 bit core RNGs (e.g. by simply incrementing an inner field) inside a module in the |
@newpavlov I migrated the new test from @dhardy I believe all your concerns are addressed now? |
The nightly failures are unrelated ( |
@vks |
We no longer implement the non-block step RNG.
@newpavlov We currently don't have a I agree that implementing |
I fixed the MIPS test by removing the special big-endian logic. I'm not sure this is correct. @dhardy Could you please have another look whether your concerns are addressed now? Thanks! |
I generally prefer to keep integration tests in the |
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.
Looks okay now! (For a real RNG the increment should be odd, otherwise the lowest bit is never flipped, but it really doesn't matter in this test.)
Many thanks to @joshlf for reporting this issue. This replaces #1159 and removes all unsafe code in the
next_u32
implementation.Fixes #1158.