Skip to content

/rust

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

`Arc` and `Rc` are dropck-unsound #29106

Closed
apasel422 opened this Issue Oct 16, 2015 · 7 comments

Comments

Assignees
No one assigned
Labels
I-nominated T-libs
Projects
None yet
Milestone
No milestone
6 participants
@apasel422 @alexcrichton @Gankro @pnkfelix @sfackler @rust-highfive
@apasel422
Copy link
Member

apasel422 commented Oct 16, 2015 

use std::sync::Arc;

struct Foo<'a>(&'a String);

impl<'a> Drop for Foo<'a> {
    fn drop(&mut self) {
        println!("{:?}", self.0);
    }
}

fn main() {
    let (y, x);
    x = "alive".to_string();
    y = Arc::new(Foo(&x));
}

Output after compiling with rustc 1.5.0-nightly (6cdf31b12 2015-10-15):

thread '<main>' panicked at 'index 0 and/or 0 in `�25` do not lie on character boundary', ../src/libcore/str/mod.rs:1444

Compiling with alloc_system and running under Valgrind confirms the use-after-free. Note that replacing Arc::new with Rc::new results in the same invalid runtime behavior, while replacing it with Box::new reports the correct lifetime error at compile time.

I believe this is a result of Arc and Rc failing to include PhantomData<T> in their internals~~, while simultaneously specifying #[unsafe_destructor_blind_to_params] on their destructors~~. Assuming my analysis of the issue is correct, I have a patch for this incoming, which will also address #29037.

CC @pnkfelix @Gankro

@sfackler sfackler added the I-wrong label Oct 16, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Oct 16, 2015

triage: I-nominated

@rust-highfive rust-highfive added the I-nominated label Oct 16, 2015

@alexcrichton alexcrichton added the T-libs label Oct 16, 2015

@apasel422

This comment has been minimized.

Sign in to view
Copy link
Member Author

apasel422 commented Oct 16, 2015

Hmm. My proposed fix causes a number of errors in run-pass/dropck_legal_cycles.rs. Investigating now.
@Gankro

This comment has been minimized.

Sign in to view
Copy link
Contributor

Gankro commented Oct 16, 2015

Fixing this was originally blocked on #26905

Should be fixable now, may require #[unsafe_destructor_blind_to_params] to pass that run-pass test.
@apasel422

This comment has been minimized.

Sign in to view
Copy link
Member Author

apasel422 commented Oct 16, 2015

@Gankro {Arc, Rc} already use that attribute. My only change is essentially to add PhantomData<T> to each of them (via Shared). See https://github.com/rust-lang/rust/compare/master...apasel422:shared?expand=1.
@apasel422

This comment has been minimized.

Sign in to view
Copy link
Member Author

apasel422 commented Oct 16, 2015

Here are the exact compile errors with my patch:

/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:414:48: 414:52 error: `arc1` does not live long enough
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:414     arc0.0.write().unwrap().children.0 = Some(&arc1);
                                                                                                              ^~~~
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:400:32: 442:2 note: reference must be valid for the block suffix following statement 133 at 400:31...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:400     let mut c = c_orig.clone();
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:401     c.control_bits = 0b1;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:402     c.curr_mark = 110;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:403     assert!(!c.saw_prev_marked);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:404     arc0.descend_into_self(&mut c);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:405     assert!(c.saw_prev_marked);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:410:51: 442:2 note: ...but borrowed value is only valid for the block suffix following statement 140 at 410:50
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:410     let (arc0, arc1, arc2): (ARCRW, ARCRW, ARCRW);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:411     arc0 = ARCRW::new("arcrw0");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:412     arc1 = ARCRW::new("arcrw1");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:413     arc2 = ARCRW::new("arcrw2");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:414     arc0.0.write().unwrap().children.0 = Some(&arc1);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:415     arc0.0.write().unwrap().children.1 = Some(&arc2);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:415:48: 415:52 error: `arc2` does not live long enough
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:415     arc0.0.write().unwrap().children.1 = Some(&arc2);
                                                                                                              ^~~~
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:400:32: 442:2 note: reference must be valid for the block suffix following statement 133 at 400:31...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:400     let mut c = c_orig.clone();
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:401     c.control_bits = 0b1;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:402     c.curr_mark = 110;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:403     assert!(!c.saw_prev_marked);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:404     arc0.descend_into_self(&mut c);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:405     assert!(c.saw_prev_marked);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:410:51: 442:2 note: ...but borrowed value is only valid for the block suffix following statement 140 at 410:50
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:410     let (arc0, arc1, arc2): (ARCRW, ARCRW, ARCRW);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:411     arc0 = ARCRW::new("arcrw0");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:412     arc1 = ARCRW::new("arcrw1");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:413     arc2 = ARCRW::new("arcrw2");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:414     arc0.0.write().unwrap().children.0 = Some(&arc1);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:415     arc0.0.write().unwrap().children.1 = Some(&arc2);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:416:48: 416:52 error: `arc0` does not live long enough
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:416     arc2.0.write().unwrap().children.0 = Some(&arc0);
                                                                                                              ^~~~
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:400:32: 442:2 note: reference must be valid for the block suffix following statement 133 at 400:31...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:400     let mut c = c_orig.clone();
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:401     c.control_bits = 0b1;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:402     c.curr_mark = 110;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:403     assert!(!c.saw_prev_marked);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:404     arc0.descend_into_self(&mut c);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:405     assert!(c.saw_prev_marked);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:410:51: 442:2 note: ...but borrowed value is only valid for the block suffix following statement 140 at 410:50
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:410     let (arc0, arc1, arc2): (ARCRW, ARCRW, ARCRW);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:411     arc0 = ARCRW::new("arcrw0");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:412     arc1 = ARCRW::new("arcrw1");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:413     arc2 = ARCRW::new("arcrw2");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:414     arc0.0.write().unwrap().children.0 = Some(&arc1);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:415     arc0.0.write().unwrap().children.1 = Some(&arc2);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:432:47: 432:51 error: `arc1` does not live long enough
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:432     arc0.1.lock().unwrap().children.0 = Some(&arc1);
                                                                                                             ^~~~
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:418:32: 442:2 note: reference must be valid for the block suffix following statement 147 at 418:31...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:418     let mut c = c_orig.clone();
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:419     c.control_bits = 0b1;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:420     c.curr_mark = 110;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:421     assert!(!c.saw_prev_marked);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:422     arc0.descend_into_self(&mut c);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:423     assert!(c.saw_prev_marked);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:428:48: 442:2 note: ...but borrowed value is only valid for the block suffix following statement 154 at 428:47
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:428     let (arc0, arc1, arc2): (ARCM, ARCM, ARCM);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:429     arc0 = ARCM::new("arcm0");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:430     arc1 = ARCM::new("arcm1");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:431     arc2 = ARCM::new("arcm2");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:432     arc0.1.lock().unwrap().children.0 = Some(&arc1);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:433     arc0.1.lock().unwrap().children.1 = Some(&arc2);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:433:47: 433:51 error: `arc2` does not live long enough
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:433     arc0.1.lock().unwrap().children.1 = Some(&arc2);
                                                                                                             ^~~~
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:418:32: 442:2 note: reference must be valid for the block suffix following statement 147 at 418:31...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:418     let mut c = c_orig.clone();
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:419     c.control_bits = 0b1;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:420     c.curr_mark = 110;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:421     assert!(!c.saw_prev_marked);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:422     arc0.descend_into_self(&mut c);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:423     assert!(c.saw_prev_marked);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:428:48: 442:2 note: ...but borrowed value is only valid for the block suffix following statement 154 at 428:47
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:428     let (arc0, arc1, arc2): (ARCM, ARCM, ARCM);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:429     arc0 = ARCM::new("arcm0");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:430     arc1 = ARCM::new("arcm1");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:431     arc2 = ARCM::new("arcm2");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:432     arc0.1.lock().unwrap().children.0 = Some(&arc1);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:433     arc0.1.lock().unwrap().children.1 = Some(&arc2);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:434:47: 434:51 error: `arc0` does not live long enough
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:434     arc2.1.lock().unwrap().children.0 = Some(&arc0);
                                                                                                             ^~~~
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:418:32: 442:2 note: reference must be valid for the block suffix following statement 147 at 418:31...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:418     let mut c = c_orig.clone();
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:419     c.control_bits = 0b1;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:420     c.curr_mark = 110;
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:421     assert!(!c.saw_prev_marked);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:422     arc0.descend_into_self(&mut c);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:423     assert!(c.saw_prev_marked);
                                                               ...
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:428:48: 442:2 note: ...but borrowed value is only valid for the block suffix following statement 154 at 428:47
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:428     let (arc0, arc1, arc2): (ARCM, ARCM, ARCM);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:429     arc0 = ARCM::new("arcm0");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:430     arc1 = ARCM::new("arcm1");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:431     arc2 = ARCM::new("arcm2");
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:432     arc0.1.lock().unwrap().children.0 = Some(&arc1);
/home/andrew/rust/src/test/run-pass/dropck_legal_cycles.rs:433     arc0.1.lock().unwrap().children.1 = Some(&arc2);
                                                               ...

@jethrogb jethrogb referenced this issue Oct 16, 2015

Closed

Valgrind shows invalid read/write when using threads #26442

@apasel422

This comment has been minimized.

Sign in to view
Copy link
Member Author

apasel422 commented Oct 16, 2015

Since the test errors are only related to {ARCRW, ARCM}, I'm inclined to think that the only problem is that the {RwLock, Mutex} destructors aren't tagged with the attribute. Investigating.

@apasel422 apasel422 referenced this issue Oct 16, 2015

Merged

Add `Shared` pointer and have `{Arc, Rc}` use it #29110

@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix commented Oct 16, 2015

I can't believe I went through the exercise of writing up that dropck-legal-cycles test (and thus proved to myself the need for the attribute on Arc/Rc) but totally overlooked the missing PhantomData that should have been attached to Rc/Arc ...

(Part of the problem may be that we may not be documenting the right intuitions about what PhantomData denotes, with respect to notions like exclusive versus partial ownership.)

@pnkfelix pnkfelix referenced this issue Oct 16, 2015

Open

dropck doc oversights / revision #29111

bors added a commit that referenced this issue Oct 17, 2015

@bors
Auto merge of #29110 - apasel422:shared, r=alexcrichton 
Fixes #29037.
Fixes #29106.

r? @pnkfelix 
CC @Gankro
34d11d4

bors added a commit that referenced this issue Oct 17, 2015

@bors
Auto merge of #29110 - apasel422:shared, r=alexcrichton 
Fixes #29037.
Fixes #29106.

r? @pnkfelix 
CC @Gankro
Loading status checks…
e1944b6

@bors bors closed this in #29110 Oct 17, 2015

@diwic diwic referenced this issue Dec 30, 2015

Closed

"cargo test" fails on 1.5.0+ #27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.