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

Ergonomics with handling dropping RegionBuffer early. #1

Closed
Aaronepower opened this Issue Dec 9, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@Aaronepower
Owner

Aaronepower commented Dec 9, 2018

The following code should panic, right now it either causes a coredump or if the program is short enough nothing at all.

EDIT: I've published a workaround that will now cause this code to panic by adding a Drop implementation to the RegionBuffer type. I'm going to keep this open to instead track alternatives to solving this issue.

fn main() {
    let strings = region_buffer::region_buffer!["Hello".to_owned()];
    let mut el = strings.get_mut(0);
    drop(strings);
    el.push('x');
    println!("{}", el);
}

@Aaronepower Aaronepower changed the title Unsoundness with dropping RegionBuffer early. Ergonomics with handling dropping RegionBuffer early. Dec 9, 2018

@dtolnay

This comment has been minimized.

dtolnay commented Dec 9, 2018

614de6b does not fix the bug because it only kills the thread holding ownership of the RegionBuffer, not any other threads that may be holding elements at the time that the RegionBuffer is dropped.

The following still triggers a core dump or segfault as of region_buffer 0.1.4.

use region_buffer::region_buffer;
use std::sync::{Arc, Mutex};
use std::thread;

fn main() {
    let el = Arc::new(Mutex::new(None));
    let el2 = el.clone();
    let thread = thread::spawn(move || {
        let rb = region_buffer!["Hello".to_owned()];
        *el2.lock().unwrap() = Some(rb.get_mut(0));
    });
    thread.join().unwrap_err();
    let mut s = el.lock().unwrap();
    s.as_mut().unwrap().push('x');
    println!("{:?}", s);
}
@dtolnay

This comment has been minimized.

dtolnay commented Dec 9, 2018

Here is a simpler repro on a single thread.

use region_buffer::region_buffer;
use std::sync::Mutex;
use std::panic;

fn main() {
    let el = Mutex::new(None);
    let _ = panic::catch_unwind(|| {
        let rb = region_buffer!["Hello".to_owned()];
        *el.lock().unwrap() = Some(rb.get_mut(0));
    });
    let mut s = el.lock().unwrap();
    s.as_mut().unwrap().push('x');
    println!("{:?}", s);
}

@xfix xfix referenced this issue Dec 9, 2018

Merged

Resolve safety issues #5

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Dec 9, 2018

fixed by #5

@Aaronepower Aaronepower closed this Dec 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment