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

[Romain Vimont]

On Sun, Sep 20, 2020 at 01:52:15AM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> On Sun, Sep 20, 2020 at 01:22:24AM +0200, Romain Vimont wrote:
> > 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.)
> 
> Note that in this case, there's the same issue between both C parts
> since they live in different shared objects. I don't know how this
> is handled in practice when it matters (probably by saying «we make
> sure there is only one CRT for both the core and the plugins) but
> having rust doesn't change this situation, since the C runtime will
> be linked against by the C plugin and not the Rust part of the plugin.

Yes, thank you. So there is in fact no problem ;)

> But if I'm not wrong, es_out_Del is called in the demux too, and
> what's left with the es_out_id_t is freed in the demux too, so it should
> be in the same shared object.
> 
> Likewise, the prototype of es_out_Add is:
> 
>     VLC_USED
>     static inline es_out_id_t * es_out_Add( es_out_t *out, const es_format_t *fmt )
>     {
>         return out->cbs->add( out, NULL, fmt );
>     }
> 
> So the es_format_t should not leak to the core too and is correctly
> released in the module with the same CRT.

Oh yes, in this specific case the es_format_t can even be destroyed
"naturally" from Rust (it does not transfer ownership to C). Thank you.

Regards