-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
WyRand::generate_range
is off by one.
#45
Comments
I am not able to reproduce the off by one error you mention. What version are you on? Do any tests failed when running I tried to recreate using:
But a |
I encounter the same problem. It occurs only with unsigned integer types though. use nanorand::{tls_rng, Rng};
assert_eq!(tls_rng().generate_range(1..2 as u32), 1); // -> succeeds
assert_eq!(tls_rng().generate_range(1..2 as i32), 1); // -> panicks
I confirmed it happening on platforms I enabled these features in Cargo.toml: nanorand = { version = "0.7", default-features = false, features = ["tls", "getrandom"] }
getrandom = { version = "*", features = ["js"] } |
I also observed this and similar snippets (where let value = match self.rng.generate_range(0..4) {
0 => 0,
1 => 1,
2 => self.rng.generate_range(0..10),
3 => self.rng.generate(),
_ => unreachable!(),
}; |
Nanorand contains an off-by-one in its WyRand implementation which made the mutator crash on hitting a code path that should be unreachable. [1] There is an unmerged patch for it on GitHub, but this is the second time I encounter an issue with this library, it also suffered from a biased shuffle when I was looking for a way to shuffle playlists in Musium [2]. To be on the safe side, instead of trying to make Cargo use the patched fork, just swap out the library for one that works. [1]: https://github dot com/Absolucy/nanorand-rs/issues/45 [2]: https://github dot com/Absolucy/nanorand-rs/issues/37 (URLs defused to prevent GitHub from auto-linking them, it's not my goal to post passive-aggressive messages in somebody else's issue tracker.)
Nanorand contains an off-by-one in its WyRand implementation which made the mutator crash on hitting a code path that should be unreachable. [1] There is an unmerged patch for it on GitHub, but this is the second time I encounter an issue with this library, it also suffered from a biased shuffle when I was looking for a way to shuffle playlists in Musium [2]. To be on the safe side, instead of trying to make Cargo use the patched fork, just swap out the library for one that works. [1]: https://github dot com/Absolucy/nanorand-rs/issues/45 [2]: https://github dot com/Absolucy/nanorand-rs/issues/37 (URLs defused to prevent GitHub from auto-linking them, it's not my goal to post passive-aggressive messages in somebody else's issue tracker.)
Nanorand contains an off-by-one in its WyRand implementation which made the mutator crash on hitting a code path that should be unreachable. [1] There is an unmerged patch for it on GitHub, but this is the second time I encounter an issue with this library, it also suffered from a biased shuffle when I was looking for a way to shuffle playlists in Musium [2]. To be on the safe side, instead of trying to make Cargo use the patched fork, just swap out the library for one that works. [1]: https://github dot com/Absolucy/nanorand-rs/issues/45 [2]: https://github dot com/Absolucy/nanorand-rs/issues/37 (URLs defused to prevent GitHub from auto-linking them, it's not my goal to post passive-aggressive messages in somebody else's issue tracker.)
I have also noticed this issue on 0.7.0 but not 0.6.x. However, I experience it with signed types only. My PoC. use nanorand::{Rng, WyRand};
fn main() {
let mut test_seed = 0;
let mi = 50;
let ma = 100;
'outer: loop {
let mut rng = WyRand::new_seed(test_seed);
for j in 0..128 {
let v = rng.generate_range(mi..ma);
if v < mi || v >= ma {
println!("{} {} {}", test_seed, j, v);
// break 'outer;
}
}
test_seed += 1;
}
} |
Nanorand contains an off-by-one in its WyRand implementation which made the mutator crash on hitting a code path that should be unreachable. [1] There is an unmerged patch for it on GitHub, but this is the second time I encounter an issue with this library, it also suffered from a biased shuffle when I was looking for a way to shuffle playlists in Musium [2]. To be on the safe side, instead of trying to make Cargo use the patched fork, just swap out the library for one that works. [1]: https://github dot com/Absolucy/nanorand-rs/issues/45 [2]: https://github dot com/Absolucy/nanorand-rs/issues/37 (URLs defused to prevent GitHub from auto-linking them, it's not my goal to post passive-aggressive messages in somebody else's issue tracker.)
prints:
The text was updated successfully, but these errors were encountered: