Skip to content
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

VecView: fix memory leak of drop #465

Merged
merged 2 commits into from
May 24, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 54 additions & 22 deletions src/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,34 +5,37 @@ use core::{
ops, ptr, slice,
};

/// Workaround forbidden specialization of Drop
pub trait VecDrop {
// SAFETY: drop_with_len will be called to call drop in place the first `len` elements of the buffer.
// Only the Owned buffer (`[MaybeUninit<T>; N]`) must drop the items
// and the view (`[MaybeUninit<T>]`) drops nothing.
// `drop_with_len `assumes that the buffer can contain `len` elements.
unsafe fn drop_with_len(&mut self, len: usize);
pub trait VecBuffer {
reitermarkus marked this conversation as resolved.
Show resolved Hide resolved
type T;

fn as_vecview(vec: &VecInner<Self>) -> &VecView<Self::T>;
fn as_mut_vecview(vec: &mut VecInner<Self>) -> &mut VecView<Self::T>;
}

impl<T> VecDrop for [MaybeUninit<T>] {
unsafe fn drop_with_len(&mut self, _len: usize) {
// Case of a view, drop does nothing
impl<T, const N: usize> VecBuffer for [MaybeUninit<T>; N] {
type T = T;

fn as_vecview(vec: &VecInner<Self>) -> &VecView<Self::T> {
vec
}
fn as_mut_vecview(vec: &mut VecInner<Self>) -> &mut VecView<Self::T> {
vec
}
}

impl<T, const N: usize> VecDrop for [MaybeUninit<T>; N] {
unsafe fn drop_with_len(&mut self, len: usize) {
// NOTE(unsafe) avoid bound checks in the slicing operation
// &mut buffer[..len]
// SAFETY: buffer[..len] must be valid to drop given the safety requirement of the trait definition.
let mut_slice = slice::from_raw_parts_mut(self.as_mut_ptr() as *mut T, len);
// We drop each element used in the vector by turning into a `&mut [T]`.
ptr::drop_in_place(mut_slice);
impl<T> VecBuffer for [MaybeUninit<T>] {
type T = T;

fn as_vecview(vec: &VecInner<Self>) -> &VecView<Self::T> {
vec
}
fn as_mut_vecview(vec: &mut VecInner<Self>) -> &mut VecView<Self::T> {
vec
}
}

/// <div class="warn">This is private API and should not be used</div>
pub struct VecInner<B: ?Sized + VecDrop> {
pub struct VecInner<B: ?Sized + VecBuffer> {
len: usize,
buffer: B,
}
Expand Down Expand Up @@ -1572,10 +1575,12 @@ impl<T, const N: usize, const M: usize> From<[T; M]> for Vec<T, N> {
}
}

impl<T: ?Sized + VecDrop> Drop for VecInner<T> {
impl<T: ?Sized + VecBuffer> Drop for VecInner<T> {
fn drop(&mut self) {
let mut_slice = VecBuffer::as_mut_vecview(self).as_mut_slice();
// We drop each element used in the vector by turning into a `&mut [T]`.
// SAFETY: the buffer contains initialized data for the range 0..self.len
unsafe { self.buffer.drop_with_len(self.len) }
unsafe { ptr::drop_in_place(mut_slice) }
}
}

Expand Down Expand Up @@ -1953,7 +1958,7 @@ mod tests {

use static_assertions::assert_not_impl_any;

use crate::Vec;
use super::{Vec, VecView};

// Ensure a `Vec` containing `!Send` values stays `!Send` itself.
assert_not_impl_any!(Vec<*const (), 4>: Send);
Expand Down Expand Up @@ -2014,6 +2019,33 @@ mod tests {
assert_eq!(Droppable::count(), 0);
}

#[test]
fn drop_vecview() {
droppable!();

{
let v: Vec<Droppable, 2> = Vec::new();
let mut v: Box<VecView<Droppable>> = Box::new(v);
v.push(Droppable::new()).ok().unwrap();
v.push(Droppable::new()).ok().unwrap();
assert_eq!(Droppable::count(), 2);
v.pop().unwrap();
assert_eq!(Droppable::count(), 1);
}

assert_eq!(Droppable::count(), 0);

{
let v: Vec<Droppable, 2> = Vec::new();
let mut v: Box<VecView<Droppable>> = Box::new(v);
v.push(Droppable::new()).ok().unwrap();
v.push(Droppable::new()).ok().unwrap();
assert_eq!(Droppable::count(), 2);
}

assert_eq!(Droppable::count(), 0);
}

#[test]
fn eq() {
let mut xs: Vec<i32, 4> = Vec::new();
Expand Down
Loading