Is this the correct way to drop the memory?

Hi y'all.
Inspired by a previous post I played around with null terminated c strings.
Because I didn't want to write actual c code I created the c strings directly in rust, which meant I was also stuck with cleaning up the memory I had allocated for the strings.
I was just wondering if I was doing this correctly here. Miri doesn't complain anymore about memory leaks but it kind of seems a bit weird how I have to recreate the original Vec and each element of CString inside. I mean I guess it makes sense, you want to drop the original CStrings as well as the Vec.

use std::ffi::c_char;
use std::ffi::CStr;
use std::ffi::CString;

#[repr(C)]
pub struct CNameList {
    pub names: *mut *mut c_char,
    pub len: usize,
}

impl CNameList {
    fn new(names: &[&str]) -> Self {
        let mut names: Vec<_> = names
            .into_iter()
            .map(|&name| CString::new(name).unwrap().into_raw())
            .collect();
        let len = names.len();
        names.shrink_to_fit();
        let buf = names.leak().as_mut_ptr();

        Self {
            names: buf,
            len: len,
        }
    }
}

impl std::ops::Drop for CNameList {
    fn drop(&mut self) {
        let ptr = self.names;
        let len = self.len;
        let cap = len; // Vector was shrunk to fit such that cap == len.
        let names = unsafe { Vec::from_raw_parts(ptr, len, cap) };
        for name_ptr in names {
            unsafe { let _ = CString::from_raw(name_ptr); }
        }
    }
}

fn main() {
    let s = CNameList::new(&["Peter", "Simone", "Goethe"]);

    let devices = unsafe { std::slice::from_raw_parts(s.names, s.len as usize) };
    for &device_ptr in devices {
        let device = unsafe { CStr::from_ptr(device_ptr) };
        dbg!(device);
    }
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.78s
     Running `target/debug/playground`
[src/main.rs:46] device = "Peter"
[src/main.rs:46] device = "Simone"
[src/main.rs:46] device = "Goethe"

By the way, I know I shouldn't expose the raw pointers outside the CNameList structure. If for example I change

    for &device_ptr in devices {
        let device = unsafe { CStr::from_ptr(device_ptr) };
        dbg!(device);
    }

to

    for &device_ptr in devices {
        let device = unsafe { CString::from_raw(device_ptr) };
        dbg!(device);
    }

I get a double-free error.

Yes, your code is correct, but I think it could be improved:

  1. Instead of shrink_to_fit, you should prefer into_boxed_slice. Then you can use Box::into_raw and Box::from_raw instead of .leak() and Vec::from_raw_parts respectively.
  2. You should move the unsafe in main into methods on CNameList.
  3. I would modify Drop so that it doesn't leak strings when CString::from_raw panics. (Even though it shouldn't panic in this case.) You can do that by writing a #[repr(transparent)] wrapper around *mut c_char that frees the CString on drop, and then have the Box::from_raw create a Box<[YourWrapper]> instead and just drop the box.
3 Likes

Yes that makes sense, thank you.

Of course. Anything else would be horrible.

This here I don't fully understand. Could you provide some code that demonstrates this?

You defined a type like this one:

#[repr(transparent)]
struct CStringDropper {
    ptr: *mut c_char,
}

impl Drop for CStringDropper {
    fn drop(&mut self) {
        drop(CString::from_raw(self.ptr));
    }
}

Then when you drop your container, you do this:

impl Drop for CNameList {
    fn drop(&mut self) {
        let ptr = self.names as *mut CStringDropper;
        let len = self.len;
        drop(Box::from_raw(std::ptr::slice_from_raw_parts_mut(ptr, len)));
    }
}

Since the destructor of box will call the destructor of the elements, then this will clear up all of the strings. Additionally, since CStringDropper is #[repr(transparent)], the cast is not a problem.

1 Like

Will the box<[CStringDropper]>::drop() continue dropping the rest of the CStringDroppers even if one of them panics?

Yes.

A panic in itself doesn't prevent dropping. Unwinding runs destructors just like normal scope exit; if you are not managing memory yourself, then things like

let v = vec![String::from("a"), String::from("b"), String::from("c")];
panic!();

still do not leak resources. (Of course, this applies recursively.)

The situation where you need to think about panics is when a panic can occur in the middle of your manual Drop impl. But a panic outside a Drop impl will still call destructors.

Trying to understand this I found the following quote in the docs for the Drop trait.

Panics

Given that a panic! will call drop as it unwinds, any panic! in a drop implementation will likely abort.

Note that even if this panics, the value is considered to be dropped; you must not cause drop to be called again. This is normally automatically handled by the compiler, but when using unsafe code, can sometimes occur unintentionally, particularly when using ptr::drop_in_place.

Doesn't that mean that it doesn't matter if I separate the drop into different stack frames as the panic! in the drop will most likely abort? I guess this is for protection against "double dropping".

This doesn't have anything to do with drop; recursive panic is simply not something the language supports. While a panic is being unwound, how should another panic be handled?

This is not true. Drop is not only called upon panic. A panic in a Drop that was a result of normal scope exit will not abort, it will generate a regular panic and be unwound.

As far as I’m aware .shrink_to_fit() does not guarantee that cap == len afterwards.

A typical example is zero-sized types but the documentation hints at that the allocator is allowed to say “hey dear Vec, even after shrinking as much as possible, there just needs to be this much spare space used in my allocation scheme anyways, so feel free to keep that!?” and the Vec might thus keep some non-zero extra capacity, even in non-zero-sized cases.

Zero-sized case:

fn main() {
    let mut x: Vec<()> = vec![()];
    x.shrink_to_fit(); // just to make sure it REALLY “fits” tightly…
    dbg!(x.capacity(), x.len());
}
[src/main.rs:4] x.capacity() = 18446744073709551615
[src/main.rs:4] x.len() = 1
1 Like

Ahh so this sentence pertains only to secondary panics in a drop. I.e. a panic happening in a drop while a panic is actively being handled. I think I understand it now. Yeah I guess the sentence says exactly this, but I didn't pick up on it.

Yes, yes, I know. But I can see how you would think that I didn't get that.

Ah thanks. I wasn't aware of that.

Yeah, and it's also not related to drop – any panic-while-panic will abort.

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.