Skip to content

Commit

Permalink
Ensure an Rc isn't freed while running its own destructor.
Browse files Browse the repository at this point in the history
A weak pointer inside itself will have its destructor run when the last
strong pointer to that data disappears, so we need to make sure that the
Weak and Rc destructors don't duplicate work (i.e. freeing).

By making the Rcs effectively take a weak pointer, we ensure that no
Weak destructor will free the pointer while still ensuring that Weak
pointers can't be upgraded to strong ones as the destructors run.

This approach of starting weak at 1 is what libstdc++ does.

Fixes #12046.
  • Loading branch information
huonw committed Feb 5, 2014
1 parent 53864ce commit da45340
Showing 1 changed file with 27 additions and 2 deletions.
29 changes: 27 additions & 2 deletions src/libstd/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@ impl<T> Rc<T> {
pub fn new(value: T) -> Rc<T> {
unsafe {
Rc {
ptr: transmute(~RcBox { value: value, strong: 1, weak: 0 }),
// there is an implicit weak pointer owned by all the
// strong pointers, which ensures that the weak
// destructor never frees the allocation while the
// strong destructor is running, even if the weak
// pointer is stored inside the strong one.
ptr: transmute(~RcBox { value: value, strong: 1, weak: 1 }),
marker: marker::NoSend,
}
}
Expand Down Expand Up @@ -81,6 +86,11 @@ impl<T> Drop for Rc<T> {
(*self.ptr).strong -= 1;
if (*self.ptr).strong == 0 {
read_ptr(self.borrow()); // destroy the contained object

// remove the implicit "strong weak" pointer now
// that we've destroyed the contents.
(*self.ptr).weak -= 1;

if (*self.ptr).weak == 0 {
exchange_free(self.ptr as *u8)
}
Expand Down Expand Up @@ -156,7 +166,9 @@ impl<T> Drop for Weak<T> {
unsafe {
if self.ptr != 0 as *mut RcBox<T> {
(*self.ptr).weak -= 1;
if (*self.ptr).weak == 0 && (*self.ptr).strong == 0 {
// the weak count starts at 1, and will only go to
// zero if all the strong pointers have disappeared.
if (*self.ptr).weak == 0 {
exchange_free(self.ptr as *u8)
}
}
Expand Down Expand Up @@ -242,4 +254,17 @@ mod tests {
let a = Rc::new(RefCell::new(Gc::new(1)));
assert!(a.borrow().try_borrow_mut().is_some());
}

#[test]
fn weak_self_cyclic() {
struct Cycle {
x: RefCell<Option<Weak<Cycle>>>
}

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

// hopefully we don't double-free (or leak)...
}
}

5 comments on commit da45340

@bors
Copy link
Contributor

@bors bors commented on da45340 Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from thestinger
at huonw@da45340

@bors
Copy link
Contributor

@bors bors commented on da45340 Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging huonw/rust/cyclic-rc = da45340 into auto

@bors
Copy link
Contributor

@bors bors commented on da45340 Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huonw/rust/cyclic-rc = da45340 merged ok, testing candidate = 9a9a70b

@bors
Copy link
Contributor

@bors bors commented on da45340 Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bors
Copy link
Contributor

@bors bors commented on da45340 Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-forwarding master to auto = 9a9a70b

Please sign in to comment.