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

segfault in select_inplace from v0.12 #163

Open
jonathanstrong opened this issue Jan 28, 2022 · 1 comment
Open

segfault in select_inplace from v0.12 #163

jonathanstrong opened this issue Jan 28, 2022 · 1 comment

Comments

@jonathanstrong
Copy link

gdb output:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000055dd04d94b20 in statrs::statistics::slice_statistics::select_inplace ()
[Current thread is 1 (Thread 0x7f4ce53fe700 (LWP 2292366))]
(gdb) bt
#0  0x000055dd04d94b20 in statrs::statistics::slice_statistics::select_inplace ()
#1  0x000055dd04d94417 in statrs::statistics::slice_statistics::<impl statrs::statistics::order_statistics::OrderStatistics<f64> for [f64]>::quantile ()

I think my input slice to quantile had NaN values in it. from experimenting, I was able to trigger similar crashes (on v0.12 codebase) if I put a bunch of NaN values into the slice.

relevant convo about use of unsafe: #109

on a local codebase, I swapped in the safer replacement code that was made in the pull request linked above, and was able to get this panic output:

---- statistics::slice_statistics::test::check_quantile_on_256k_slice stdout ----
thread 'statistics::slice_statistics::test::check_quantile_on_256k_slice' panicked at 'index out of bounds: the len is 262144 but the index is 262144', src/statistics/slice_statistics.rs:340:20
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

which corresponds to

        // inside select_inplace
        loop {
            loop {
                begin += 1;
                if arr[begin] >= pivot {  // <- line 340 is this one
                    break;
                }
            }
            loop {
                end -= 1;
                if arr[end] <= pivot {
                    break;
                }
            }
            if end < begin {
                break;
            }
            arr.swap(begin, end);
        }

I am using v0.12 (in a bunch of places actually) because I have code that uses the old api. would you be open to a pull request to swap in the safer version of select_inplace and issue another v0.12 release?

(here is the test that triggered the above btw)

    #[test]
    fn check_quantile_on_256k_slice() {
        for _ in 0..(1024) {
            let mut xs: Vec<f64> = rand::distributions::Standard.sample_iter(&mut thread_rng()).take(1024*256).collect();
            for i in (0..xs.len()).step_by(42) {
                xs[i] = f64::NAN;
            }
            xs.quantile(0.01) ;
            xs.quantile(0.05) ;
            xs.quantile(0.1)  ;
            xs.quantile(0.2)  ;
            xs.quantile(0.25) ;
            xs.quantile(0.3)  ;
            xs.quantile(0.4)  ;
            xs.quantile(0.45) ;
            xs.quantile(0.5)  ;
            xs.quantile(0.55) ;
            xs.quantile(0.6)  ;
            xs.quantile(0.65) ;
            xs.quantile(0.7)  ;
            xs.quantile(0.75) ;
            xs.quantile(0.8)  ;
            xs.quantile(0.9)  ;
            xs.quantile(0.95) ;
            xs.quantile(0.99) ;
        }
    }

@YeungOnion
Copy link
Contributor

Didn't see that there was a PR for this, do you still want to include that? I'm sure I can figure out a fix release for 0.12 on crates.

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

2 participants