Skip to content
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

Open
StyMaar opened this issue Sep 2, 2023 · 4 comments
Open

WyRand::generate_range is off by one. #45

StyMaar opened this issue Sep 2, 2023 · 4 comments

Comments

@StyMaar
Copy link

StyMaar commented Sep 2, 2023

fn example(){
    use nanorand::{Rng, WyRand};

    let mut rng = WyRand::new();
    println!("Random number: {}", rng.generate_range(1..=3));
    println!("Random number: {}", rng.generate_range(1..=3));
    println!("Random number: {}", rng.generate_range(1..=3));
    println!("Random number: {}", rng.generate_range(1..=3));
    println!("Random number: {}", rng.generate_range(1..=3));
    println!("Random number: {}", rng.generate_range(1..=3));

}

prints:

Random number: 1
Random number: 2
Random number: 1
Random number: 1
Random number: 0
Random number: 2
@jklott
Copy link
Contributor

jklott commented Oct 27, 2023

I am not able to reproduce the off by one error you mention. What version are you on? Do any tests failed when running cargo test? Some of the tests in gen.rs should fail if a number goes outside of the range specified.

I tried to recreate using:

for _ in 0..1_000_000 {
        let x  = rng.generate_range(1..=3);
        if x == 0 {
            break
        }
    }

But a 0 never occured.

@StT191
Copy link

StT191 commented Nov 26, 2023

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
thread 'main' panicked at src/main.rs:4:1:
assertion `left == right` failed
  left: 0
 right: 1

I confirmed it happening on platforms x86_64-unknown-linux-gnu and wasm32-unknown-unknown.

I enabled these features in Cargo.toml:

nanorand = { version = "0.7", default-features = false, features = ["tls", "getrandom"] }
getrandom = { version = "*", features = ["js"] }

@ruuda
Copy link

ruuda commented Apr 21, 2024

I also observed this and similar snippets (where rng is a WyRand) hitting the unreachable case:

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!(),
};

ruuda added a commit to ruuda/rcl that referenced this issue Apr 21, 2024
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.)
ruuda added a commit to ruuda/rcl that referenced this issue Apr 21, 2024
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.)
ruuda added a commit to ruuda/rcl that referenced this issue Apr 22, 2024
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.)
@cyeganeh01248
Copy link

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;
    }
}

ruuda added a commit to ruuda/rcl that referenced this issue Apr 25, 2024
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.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants