Direct Rc/Weak cycle causes double free

[huonw]

use std::rc::{Rc, Weak};
use std::cell::RefCell;

struct Cycle {
    x: RefCell<Option<Weak<Cycle>>>
}

fn main() {
    let a = Rc::new(Cycle { x: RefCell::new(None) });
    let b = a.clone().downgrade();
    *a.borrow().x.borrow_mut().get() = Some(b);
}

crashes with a double free when run, with and without optimisations.

I'm investigating.

[Skrylar]

I just ran in to this problem while writing some (slightly more advanced code) https://gist.github.com/Skrylar/8779719

Currently breaks many designs of GUIs.

[alexcrichton]

I believe that the sequence of events looks like this:

1. The code above executes.
2. The destructor for Rc<Cycle> begins to run
3. The recount is decremented to 0, so the destructor of Cycle is run (note that the weak count hasn't been checked yet).
4. The destructor of Cycle runs, eventually running the destructor of Weak<Cycle>
5. The weak count is decremented to 0, so the Rc box allocation is freed
6. The destructor for Rc<Cycle> continues, sees the weak count is 0, and frees the inner box again.

I think that when the destructor for Rc is running we may want to do something like setting the box to immortal? I'm not entirely sure if that would work out.

[huonw]

Looks like you got it in one:

Rc: dropping 0x7fd4dc000de0
Rc: strong = 2, weak = 1
Weak: dropping 0x0
Rc: dropping 0x7fd4dc000de0
Rc: strong = 1, weak = 1
Rc: destroying
Weak: dropping 0x7fd4dc000de0
Weak: strong = 0, weak = 1
Weak: freeing
Rc: freeing

is the RUST_LOG=std::rc output of

diff --git a/src/libstd/rc.rs b/src/libstd/rc.rs
index 7d0ddb2..79d6d68 100644
--- a/src/libstd/rc.rs
+++ b/src/libstd/rc.rs
@@ -78,8 +78,12 @@ impl<T> Drop for Rc<T> {
     fn drop(&mut self) {
+        info!("Rc: dropping {}", self.ptr);
         unsafe {
             if self.ptr != 0 as *mut RcBox<T> {
+                info!("Rc: strong = {}, weak = {}", (*self.ptr).strong, (*self.ptr).weak);
                 (*self.ptr).strong -= 1;
                 if (*self.ptr).strong == 0 {
+                    info!("Rc: destroying");
                     read_ptr(self.borrow()); // destroy the contained object
                     if (*self.ptr).weak == 0 {
+                        info!("Rc: freeing");
                         exchange_free(self.ptr as *u8)
@@ -155,6 +159,9 @@ impl<T> Drop for Weak<T> {
     fn drop(&mut self) {
+        info!("Weak: dropping {}", self.ptr);
         unsafe {
             if self.ptr != 0 as *mut RcBox<T> {
+                info!("Weak: strong = {}, weak = {}", (*self.ptr).strong, (*self.ptr).weak);
                 (*self.ptr).weak -= 1;
                 if (*self.ptr).weak == 0 && (*self.ptr).strong == 0 {
+                    info!("Weak: freeing");
                     exchange_free(self.ptr as *u8)

[huonw]

I think we can just reorder the decrements.

[alexcrichton]

One side effect of re-ordering the decrements is that a weak pointer could be upgraded while the last reference to the box is being destroyed, which I think may cause problems because you'd have a strong reference to a thing that has had Drop run on it (I think?)