jedisct1 / rust-libhydrogen

Libhydrogen bindings for Rust.
16 stars 3 forks source link

Memory leak when executing test `utils::tests::test_utils` #11

Closed icmccorm closed 10 months ago

icmccorm commented 10 months ago

I've been developing an experimental version of Miri that can execute foreign functions by interpreting LLVM bytecode.

Miri found the following memory leak when executing the test utils::tests::test_utils:

error: memory leaked: alloc30612 (Rust heap, size: 2, align: 1), allocated here:
   --> /home/icmccorm/git/ffickle/rust/library/alloc/src/alloc.rs:98:9
    |
98  |         __rust_alloc(layout.size(), layout.align())
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: inside `std::alloc::alloc` at .../rust/library/alloc
    ...
    = note: inside `std::ffi::CString::new::<&[u8]>` at .../rust/library/alloc/src/ffi/c_str.rs:316:9: 316:26
note: inside `utils::hex2bin`
   --> src/utils.rs:70:25
    |
70  |         Some(ignore) => CString::new(ignore)
    |                         ^^^^^^^^^^^^^^^^^^^^
note: inside `utils::tests::test_utils`
   --> src/utils.rs:131:33
    |
131 |         let mut bin2: Vec<u8> = utils::hex2bin("#452a#", Some(b"#")).unwrap();
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It seems like the CString instance allocated in hex2bin and exposed with CString::into_raw is never deallocated, since there isn't a corresponding call to CString::from_raw

pub fn hex2bin(hex: &str, ignore: Option<&[u8]>) -> Result<Vec<u8>, HydroError> {
    ...
    let ignore_p = match ignore {
        Some(ignore) => CString::new(ignore)
            .map_err(|_| HydroError::InvalidInput)?
            .into_raw(),
    ...
}
jedisct1 commented 10 months ago

My intuition is that Rust was supposed to deallocate that string once it goes out of scope, like anything else.

How can memory leaks can happen in things that don't come from an unsafe {} block?

jedisct1 commented 10 months ago

Wow, you're right. Looking at the documentation:

    // retake pointer to free memory
    let _ = CString::from_raw(ptr);

Rust FFI is such a footgun. That doesn't make no sense. If RAII doesn't work here, into_raw() should require unsafe.

icmccorm commented 10 months ago

How can memory leaks can happen in things that don't come from an unsafe {} block?

I think the reason why this is allowed to happen in a safe context is because memory leaks are not considered undefined behavior in the Rust spec. For example, you can create a reference cycle with Rc<T> that will leak memory without needing to use unsafe.