Skip to content

Commit eb2e420

Browse files
borsbors
committed
Auto merge of rust-lang#132231 - lukas-code:rc-plug-leaks, r=tgross35
Rc/Arc: don't leak the allocation if drop panics Currently, when the last `Rc<T>` or `Arc<T>` is dropped and the destructor of `T` panics, the allocation will be leaked. This leak is unnecessary since the data cannot be (safely) accessed again and `Box` already deallocates in this case, so let's do the same for `Rc` and `Arc`, too.
2 parents 6495896 + 9318ae3 commit eb2e420

File tree

5 files changed

+139
-21
lines changed

5 files changed

+139
-21
lines changed

‎alloc/src/rc.rs

+17-11
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,21 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
376376
unsafe fn from_ptr_in(ptr: *mut RcInner<T>, alloc: A) -> Self {
377377
unsafe { Self::from_inner_in(NonNull::new_unchecked(ptr), alloc) }
378378
}
379+
380+
// Non-inlined part of `drop`.
381+
#[inline(never)]
382+
unsafe fn drop_slow(&mut self) {
383+
// Reconstruct the "strong weak" pointer and drop it when this
384+
// variable goes out of scope. This ensures that the memory is
385+
// deallocated even if the destructor of `T` panics.
386+
let _weak = Weak { ptr: self.ptr, alloc: &self.alloc };
387+
388+
// Destroy the contained object.
389+
// We cannot use `get_mut_unchecked` here, because `self.alloc` is borrowed.
390+
unsafe {
391+
ptr::drop_in_place(&mut (*self.ptr.as_ptr()).value);
392+
}
393+
}
379394
}
380395

381396
impl<T> Rc<T> {
@@ -2252,21 +2267,12 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Rc<T, A> {
22522267
/// drop(foo); // Doesn't print anything
22532268
/// drop(foo2); // Prints "dropped!"
22542269
/// ```
2270+
#[inline]
22552271
fn drop(&mut self) {
22562272
unsafe {
22572273
self.inner().dec_strong();
22582274
if self.inner().strong() == 0 {
2259-
// destroy the contained object
2260-
ptr::drop_in_place(Self::get_mut_unchecked(self));
2261-
2262-
// remove the implicit "strong weak" pointer now that we've
2263-
// destroyed the contents.
2264-
self.inner().dec_weak();
2265-
2266-
if self.inner().weak() == 0 {
2267-
self.alloc
2268-
.deallocate(self.ptr.cast(), Layout::for_value_raw(self.ptr.as_ptr()));
2269-
}
2275+
self.drop_slow();
22702276
}
22712277
}
22722278
}

‎alloc/src/sync.rs

+9-7
Original file line numberDiff line numberDiff line change
@@ -1872,15 +1872,17 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
18721872
// Non-inlined part of `drop`.
18731873
#[inline(never)]
18741874
unsafe fn drop_slow(&mut self) {
1875+
// Drop the weak ref collectively held by all strong references when this
1876+
// variable goes out of scope. This ensures that the memory is deallocated
1877+
// even if the destructor of `T` panics.
1878+
// Take a reference to `self.alloc` instead of cloning because 1. it'll last long
1879+
// enough, and 2. you should be able to drop `Arc`s with unclonable allocators
1880+
let _weak = Weak { ptr: self.ptr, alloc: &self.alloc };
1881+
18751882
// Destroy the data at this time, even though we must not free the box
18761883
// allocation itself (there might still be weak pointers lying around).
1877-
unsafe { ptr::drop_in_place(Self::get_mut_unchecked(self)) };
1878-
1879-
// Drop the weak ref collectively held by all strong references
1880-
// Take a reference to `self.alloc` instead of cloning because 1. it'll
1881-
// last long enough, and 2. you should be able to drop `Arc`s with
1882-
// unclonable allocators
1883-
drop(Weak { ptr: self.ptr, alloc: &self.alloc });
1884+
// We cannot use `get_mut_unchecked` here, because `self.alloc` is borrowed.
1885+
unsafe { ptr::drop_in_place(&mut (*self.ptr.as_ptr()).data) };
18841886
}
18851887

18861888
/// Returns `true` if the two `Arc`s point to the same allocation in a vein similar to

‎alloc/tests/arc.rs

+38-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use std::any::Any;
2-
use std::cell::RefCell;
2+
use std::cell::{Cell, RefCell};
33
use std::iter::TrustedLen;
44
use std::mem;
55
use std::sync::{Arc, Weak};
@@ -89,7 +89,7 @@ fn eq() {
8989

9090
// The test code below is identical to that in `rc.rs`.
9191
// For better maintainability we therefore define this type alias.
92-
type Rc<T> = Arc<T>;
92+
type Rc<T, A = std::alloc::Global> = Arc<T, A>;
9393

9494
const SHARED_ITER_MAX: u16 = 100;
9595

@@ -210,6 +210,42 @@ fn weak_may_dangle() {
210210
// borrow might be used here, when `val` is dropped and runs the `Drop` code for type `std::sync::Weak`
211211
}
212212

213+
/// Test that a panic from a destructor does not leak the allocation.
214+
#[test]
215+
#[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
216+
fn panic_no_leak() {
217+
use std::alloc::{AllocError, Allocator, Global, Layout};
218+
use std::panic::{AssertUnwindSafe, catch_unwind};
219+
use std::ptr::NonNull;
220+
221+
struct AllocCount(Cell<i32>);
222+
unsafe impl Allocator for AllocCount {
223+
fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
224+
self.0.set(self.0.get() + 1);
225+
Global.allocate(layout)
226+
}
227+
unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
228+
self.0.set(self.0.get() - 1);
229+
unsafe { Global.deallocate(ptr, layout) }
230+
}
231+
}
232+
233+
struct PanicOnDrop;
234+
impl Drop for PanicOnDrop {
235+
fn drop(&mut self) {
236+
panic!("PanicOnDrop");
237+
}
238+
}
239+
240+
let alloc = AllocCount(Cell::new(0));
241+
let rc = Rc::new_in(PanicOnDrop, &alloc);
242+
assert_eq!(alloc.0.get(), 1);
243+
244+
let panic_message = catch_unwind(AssertUnwindSafe(|| drop(rc))).unwrap_err();
245+
assert_eq!(*panic_message.downcast_ref::<&'static str>().unwrap(), "PanicOnDrop");
246+
assert_eq!(alloc.0.get(), 0);
247+
}
248+
213249
/// This is similar to the doc-test for `Arc::make_mut()`, but on an unsized type (slice).
214250
#[test]
215251
fn make_mut_unsized() {

‎alloc/tests/boxed.rs

+38
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,44 @@ fn box_deref_lval() {
6060
assert_eq!(x.get(), 1000);
6161
}
6262

63+
/// Test that a panic from a destructor does not leak the allocation.
64+
#[test]
65+
#[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
66+
fn panic_no_leak() {
67+
use std::alloc::{AllocError, Allocator, Global, Layout};
68+
use std::panic::{AssertUnwindSafe, catch_unwind};
69+
use std::ptr::NonNull;
70+
71+
struct AllocCount(Cell<i32>);
72+
unsafe impl Allocator for AllocCount {
73+
fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
74+
self.0.set(self.0.get() + 1);
75+
Global.allocate(layout)
76+
}
77+
unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
78+
self.0.set(self.0.get() - 1);
79+
unsafe { Global.deallocate(ptr, layout) }
80+
}
81+
}
82+
83+
struct PanicOnDrop {
84+
_data: u8,
85+
}
86+
impl Drop for PanicOnDrop {
87+
fn drop(&mut self) {
88+
panic!("PanicOnDrop");
89+
}
90+
}
91+
92+
let alloc = AllocCount(Cell::new(0));
93+
let b = Box::new_in(PanicOnDrop { _data: 42 }, &alloc);
94+
assert_eq!(alloc.0.get(), 1);
95+
96+
let panic_message = catch_unwind(AssertUnwindSafe(|| drop(b))).unwrap_err();
97+
assert_eq!(*panic_message.downcast_ref::<&'static str>().unwrap(), "PanicOnDrop");
98+
assert_eq!(alloc.0.get(), 0);
99+
}
100+
63101
#[allow(unused)]
64102
pub struct ConstAllocator;
65103

‎alloc/tests/rc.rs

+37-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use std::any::Any;
2-
use std::cell::RefCell;
2+
use std::cell::{Cell, RefCell};
33
use std::iter::TrustedLen;
44
use std::mem;
55
use std::rc::{Rc, Weak};
@@ -206,6 +206,42 @@ fn weak_may_dangle() {
206206
// borrow might be used here, when `val` is dropped and runs the `Drop` code for type `std::rc::Weak`
207207
}
208208

209+
/// Test that a panic from a destructor does not leak the allocation.
210+
#[test]
211+
#[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
212+
fn panic_no_leak() {
213+
use std::alloc::{AllocError, Allocator, Global, Layout};
214+
use std::panic::{AssertUnwindSafe, catch_unwind};
215+
use std::ptr::NonNull;
216+
217+
struct AllocCount(Cell<i32>);
218+
unsafe impl Allocator for AllocCount {
219+
fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
220+
self.0.set(self.0.get() + 1);
221+
Global.allocate(layout)
222+
}
223+
unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
224+
self.0.set(self.0.get() - 1);
225+
unsafe { Global.deallocate(ptr, layout) }
226+
}
227+
}
228+
229+
struct PanicOnDrop;
230+
impl Drop for PanicOnDrop {
231+
fn drop(&mut self) {
232+
panic!("PanicOnDrop");
233+
}
234+
}
235+
236+
let alloc = AllocCount(Cell::new(0));
237+
let rc = Rc::new_in(PanicOnDrop, &alloc);
238+
assert_eq!(alloc.0.get(), 1);
239+
240+
let panic_message = catch_unwind(AssertUnwindSafe(|| drop(rc))).unwrap_err();
241+
assert_eq!(*panic_message.downcast_ref::<&'static str>().unwrap(), "PanicOnDrop");
242+
assert_eq!(alloc.0.get(), 0);
243+
}
244+
209245
#[allow(unused)]
210246
mod pin_coerce_unsized {
211247
use alloc::rc::{Rc, UniqueRc};

0 commit comments

Comments
 (0)