[vlc-devel] [PATCH 1/2] libvlc: Add Rust API for writing modules in rust

[Romain Vimont]

On Sun, Sep 20, 2020 at 01:08:51AM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> On Sun, Sep 20, 2020 at 01:00:20AM +0200, Romain Vimont wrote:
> > On Sun, Sep 20, 2020 at 02:39:33AM +0530, Kartik Ohri wrote:
> > >        - Added a utils module to vlccore-rs
> > >                This module can contain commonly used utilities for Rust
> > >                modules. For instance, passing Rust allocated strings to
> > >                C code. This is an easy thing to mess up. Rust allocated
> > >                pointers should not be free()`ed in C code. Hence, a
> > >                utility method is provided in vlccore-rs::utils to
> > >                ensure such issues are avoided.
> >
> > I encounter the same problem (not just for Strings): in addition to
> > correctly transfer ownership between C and Rust, the memory must be
> > deallocated on the same side as it was allocated (either both in C or
> > both in Rust).
> >
> > In particular, passing a Rust-allocated memory to C by just calling
> > mem::forget() in Rust (so that C free() it) is incorrect:
> > https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw
> >
> > The recommended solution is to pass the pointer back to Rust so that it
> > deallocates the memory. But in practice, if we create a VLC core struct
> > from Rust with allocated fields (in my case es_format_t) and transfer
> > its ownership to C, we don't want to release each pointer from Rust (the
> > core will just call es_format_Clean() and free them directly).
> >
> > To solve the problem, you propose to call libc::malloc() from Rust and
> > free() from C (or malloc() from C and libc::free() from Rust). I like
> > this solution, but IIUC, it is not guaranteed to be correct, because the
> > allocators used by Rust and C may be different:
> >
> > > To free the data behind the C pointer you should call a C function
> > > that does it, so that definitely the same C’s allocator is used. If
> > > the pointer comes from C’s malloc, then Rust’s libc::free may work,
> > > except it’s dangerous to do so if Windows DLLs are involved, since
> > > each DLL may have separate runtime.
> 
> AFAIU this, it's safe for our case as long as the underlying C module
> is calling the free here since they both live in the same shared object.
> 
> However, the pointer must not move to the core and be released there.

OK.

(In my case, it's moving to the core, because I pass an es_format_t
created from Rust, so I need to pass for example an allocated
string for psz_language, that will be free()d by the core.)

> > https://users.rust-lang.org/t/idiomatic-way-to-handle-pointer-to-values-returned-by-ffi-call/17085/3
> >
> > > Rust 0.11.0 through 1.31.1 used jemalloc. Rust 1.32.0 changed to use
> > > the system's default allocator.
> > >
> > > Additionally, Rust 1.28.0 introduced a mechanism that applications can
> > > use to replace the global allocator with one of their choosing.
> > >
> > > It's important to note that, although Rust now uses the system's
> > > default allocator by default, that doesn't mean that C libraries use
> > > the same allocator, even if it's literally malloc. For example, on
> > > Windows, if you use a C library that's been compiled with Visual C++
> > > 2008 while your Rust binary has been compiled with Visual Studio 2019
> > > Build Tools, there will be two C runtime libraries loaded in your
> > > process: the C library will use msvcr90.dll while your Rust binary
> > > will use ucrtbase.dll. Each C runtime library manages its own heap, so
> > > memory allocated by one cannot be freed by the other.
> >
> > https://stackoverflow.com/a/57879688/1987178
> >
> > (Now that Rust uses the system allocator by default, my understanding is
> > that it might be safe for VLC, but in theory they could change it again
> > in future versions.)
> >
> > One possible (and generic) way to solve the issue could be to expose
> > (an equivalent of) vlc_alloc() via FFI, so that we could call
> > ffi::vlc_alloc() from Rust. That way, we could safely free() the pointer
> > from C.
> >
> > And we could just use libc::memcpy() to fill the C-allocated memory with
> > any Rust #[repr(C)] data.
> >
> > What do you think?
> >
> > > diff --git a/src/vlccore-rs/src/utils.rs b/src/vlccore-rs/src/utils.rs
> > > new file mode 100644
> > > index 0000000000..3d9e25ac31
> > > --- /dev/null
> > > +++ b/src/vlccore-rs/src/utils.rs
> > > @@ -0,0 +1,19 @@
> > > +use libc::{c_char, malloc};
> > > +use std::ffi::CString;
> > > +use std::ptr::null_mut;
> > > +use std::slice::from_raw_parts_mut;
> > > +
> > > +pub unsafe fn convert_string_to_ptr(string: String) -> *mut c_char {
> > > +    if string.is_empty() {
> > > +        return std::ptr::null_mut();
> > > +    }
> > > +    let size = string.len() + 1;
> > > +    CString::new(string)
> > > +        .map(|ptr| {
> > > +            let c_ptr = malloc(size * std::mem::size_of::<u8>()) as *mut u8;
> >                            ^ this malloc
> >
> > > +            let slice = from_raw_parts_mut(c_ptr, size);
> > > +            slice[..size].copy_from_slice(ptr.as_bytes_with_nul());
> > > +            c_ptr as *mut c_char
> > > +        })
> > > +        .unwrap_or(null_mut())
> > > +}
> >
> > Regards
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel