Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit e473783

Browse files
committed
Auto merge of #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 2dece5b + 02ee639 commit e473783

File tree

5 files changed

+139
-21
lines changed

5 files changed

+139
-21
lines changed

‎library/alloc/src/rc.rs

Lines changed: 17 additions & 11 deletions
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
}

‎library/alloc/src/sync.rs

Lines changed: 9 additions & 7 deletions
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

‎library/alloc/tests/arc.rs

Lines changed: 38 additions & 2 deletions
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() {

‎library/alloc/tests/boxed.rs

Lines changed: 38 additions & 0 deletions
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

‎library/alloc/tests/rc.rs

Lines changed: 37 additions & 1 deletion
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)

AltStyle によって変換されたページ (->オリジナル) /