FFI around mpv, are my data types sound?

code review
1

I'm working on a libmpv client crate.

My main concern is the handling of data that crosses the FFI boundary, which can be found in src/types/, specifically, node.rs, node_array.rs, node_map.rs, and byte_array.rs.

The Node type is an enum that can hold, among other things, a NodeArray or a NodeMap, each of which contains other Nodes which themselves may be NodeArrays or NodeMaps. A Node can also contain a ByteArray, though there is no further nesting after that.

The bindgen types that actually must be sent to mpv are:

#[repr(C)]
#[derive(Copy, Clone)]
pub struct mpv_node {
    pub u: mpv_node__bindgen_ty_1,
    pub format: mpv_format,
}

#[repr(C)]
#[derive(Copy, Clone)]
pub union mpv_node__bindgen_ty_1 {
    pub string: *mut ::std::os::raw::c_char,
    pub flag: ::std::os::raw::c_int,
    pub int64: i64,
    pub double_: f64,
    pub list: *mut mpv_node_list,
    pub ba: *mut mpv_byte_array,
}

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct mpv_node_list {
    pub num: ::std::os::raw::c_int,
    pub values: *mut mpv_node,
    // keys is only valid if the mpv_format tag in the containing mpv_node is MPV_FORMAT_NODE_MAP, otherwise, NULL
    pub keys: *mut *mut ::std::os::raw::c_char,
}

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct mpv_byte_array {
    pub data: *mut ::std::os::raw::c_void,
    pub size: usize,
}

Since these types contain raw pointers to structures with different layouts than the native Rust types I'm using for the more ergonomic API, I have to first deconstruct the Rust types into an intermediate type which can hold any owned buffers[1], as well as the underlying mpv representation, and that intermediate step is primarily what I'm concerned about.

Here direct links to the MpvRepr types for each of the above: MpvNode, MpvNodeArray, MpvNodeMap, and MpvByteArray.

And then here are direct links to the ToMpvRepr implementations for each: Node, NodeArray, NodeMap, and ByteArray.

These MpvRepr types are not public, and are designed such that they are only used in the types MpvSend implementation; here is Node's, for example. As such, their lifetime should be pretty short, limited only to the completion of the mpv function call (which is wrapped in the lambda passed into to_mpv(), see here for an example).

However, because I'm dealing with raw pointers, I'm a bit concerned about maintaining validity. In my first attempt at this, I goofed and didn't Box the underlying mpv types, like mpv_node. When I started writing my set of test cases, the first few passed, but once I started nesting more deeply, I started getting some corruption. I guess after a few months of Rust I forgot about stack vs. heap.

This was fixed by Box'ing the underlying mpv_* structure and ensuring all the other fields which are referenced by that structure are allocated on the heap somehow (for example, in NodeArray, _flat_reprs, which node_list contains a pointer to).

However, after reading about Pin, I'm a bit concerned about the possibility of data moving. As far as I know, Box shouldn't move it's allocation, and Vec shouldn't if it doesn't reallocate (which I try to prevent by creating the Vec with the correct capacity). But I'm wondering if there's something I'm overlooking, and whether I should be using Pin? These structures aren't exactly self-referential, but they do have raw pointers to other heap structures, which do have to be stored, even if only for a very limited lifetime.

I'll take any feedback on any part of the library, the main user interface is handle.rs and the docs are live here: libmpv_client - Rust

However my primary goal is feedback on the way I'm handling the conversion to mpv-compatible types, and any potential soundness or pointer validity issues.

Thank you for any advice you can give, I'm very grateful!

P.S., one idea I already have, though I don't think it'll really have a practical effect, is moving the _flat_reprs in NodeArray and the _flat_reprs and _flat_keys in NodeMap to be Box'd slices, so there is a clear delineation between the buffers which get sent to mpv as pointers, and the Vecs which exist merely to keep the intermediate MpvRepr alive.

  1. The biggest reason why I have to store these intermediate types is because Node can hold a CString, which we have to own. If not, then each of these types could just have a to_mpv() which takes a reference to the original type and returns the relevant mpv_* data type already constructed; however, since NodeMap and NodeArray could contain a Node which is a string, in which case we need to retain ownership so it's CString doesn't get dropped. There's also the matter of the arrays of mpv_nodes in NodeArray and NodeMap, but I had a different solution for that as well at one point. I thought about flattening out all of the referenced CStrings in the whole Node tree into a single Vec<CString>, but the logic for this got very complicated, so I went with this sort of recursive solution. ↩︎

2

I think this is the reason why your design seems overly complicated.

you don't have to define a separate hierachy of (ffi-incompatible) types in parallel along the ffi types, in order to make the rust API ergonomic. if you can just reuse the ffi types directly (or, layout compatible #[repr(transparent)] wrappers), you can avoid deeply marshaling the data back and forth, which is not only inefficient, but also very error prone.

the key insight is you don't have to use owned types all the time, sometimes it's easier to borrow.

I'm not familiar with libmpv, I took a glimpse at client.h, and here's a sketch how I would have designed the rust API:

  • I will either make bindgen generate an enum for mpv_format, or create a custom enum that is bidirectional convertible (i.e. impl Into ...) to and from sys::mpv_format

  • add fallible conversion methods (and/or type querying methods) to mvp_node (or rather, a transparent wrapper type outside of the -sys crate),

    • a transparent mpv_node wrapper type:
    #[repr(transparent)]
struct Node(mpv_node);
    • scalars can be converted by value:
    impl Node {
	fn as_flag(&self) -> Option<bool> {
		match self.0.format {
			...
		}
	}
	fn as_i64(&self) -> Option<i64> { todo!() }
	fn as_f64(&self) -> Option<f64> { todo!() }
}
    • pointers are converted to references (mut variants are omitted):
    impl Node {
  	fn as_cstr(&self) -> Option<&CStr> { todo!() }
	fn as_byte_array(&self) -> Option<&ByteArray> { todo!() }
	fn as_node_array(&self) -> Option<&NodeArray> { todo!() }
	fn as_node_map(&self) -> Option<&NodeMap> { todo!() }
    • optional: convenient methods (using str instead CStr, data format query, etc):
    impl Node {
	fn as_str(&self) -> Option<&str> {
		self
    		.as_cstr()
    		.map(|cs| cs.to_str().expect("libmpv strings are always utf8 encoded"))
	}
  	fn is_flag(&self) -> bool { self.as_flag().is_some() }
	// is_i64(), is_f64(), is_str(), ...
}
    • empirically, most use cases expect the data in certain formats, so as_str() is usually more useful then an enum that represents all possible data types, but if that's needed, it can be implemented as borrowed form too:
    enum NodeVariants<'a> {
	None,
	// scalars
	Flag(bool),
	I64(i64),
	F64(f64),
	// pointers
	Str(&'a str),
	NodeArray(&'a NodeArray),
	NodeMap(&'a NodeMap),
	ByteArray(&'a ByteArray),
}

impl Node {
	fn as_variants(&self) -> NodeVariants<'_> {
		match self.0.format {
			...
		}
	}
}

  • implement idiomatic APIs for other high level wrapper types

    • note, these type represents data owned by ffi, rust code should not construct them, but only obtain references of them from the API.

      user code cannot construct them without public constructors, if we field is private.

    • the wrapper for mpv_byte_array is very straitforward: Deref can be implemented to provide an idiomatic API (DerefMut is omitted):

    #[repr(transparent)]
struct ByteArray(mpv_byte_array);
/// ByteArray can be accessed just like a byte slice
impl Deref for ByteArray {
	type Target = [u8];
	fn deref(&self) -> &Self::Target {
		unsafe { std::slice::from_raw_parts(self.0.data as *const u8, self.0.size) }
	}
}
    • we can create 2 different wrapper types for mpv_node_list:
    #[repr(transparent)]
struct NodeArray(mpv_node_list);

#[repr(transparent)]
struct NodeMap(mpv_node_list);
    • for NodeArray, we provide idiomatic rust API via Deref (and DerefMut), note how we can recursively borrow (but not own) the nested Node:
    impl Deref for NodeArray {
	type Target = [Node];
	fn deref(&self) -> &Self::Target {
		// Node is transparent wrapper for mpv_node, pointer cast is valid
		unsafe { std::slice::from_raw_parts(self.0.values as *mut Node, self.0.num as usize) }
	}
}
    • I don't know how NodeMap will be used, but as an illustration, let's make it traversable as key-value pairs --- this also makes it compatible to the HashMap::from_iter() API:
    // raw, low-level APIs, encapsulate raw pointer access
impl NodeMap {
	fn as_keys(&self) -> &[*const c_char] {
		unsafe { std::slice::from_raw_parts(self.0.keys, self.0.num as usize) }
	}
	fn as_values(&self) -> &[Node] {
		unsafe { std::slice::from_raw_parts(self.0.values as *mut Node, self.0.num as usize) }
	}
	fn keys_cstr(&self) -> impl Iterator<Item = &CStr> {
		self
			.as_keys()
			.iter()
			.map(|&ptr| unsafe { CStr::from_ptr(ptr) })
	}
}
// higher level, idiomatic APIs
impl NodeMap {
	fn keys(&self) -> impl Iterator<Item = &str> {
		self.keys_cstr().map(|cs| cs.to_str().expect("utf8"))
	}
	fn values(&self) -> impl Iterator<Item = &Node> {
		self.as_values().iter()
	}
	fn iter(&self) -> impl Iterator<Item = (&str, &Node)> {
		self.keys().zip(self.values())
	}
}

  • because the above Node type is used recursively, we cannot use it for resource management, instead, we create a "root" node, let's call it NodeOwner (or whatever):

    #[repr(transparent)]
struct NodeOwner(Node);
/// impl `Deref` so it can use the methods of `Node`
impl Deref for NodeOwner {
	type Target = Node;
	fn deref(&self) -> &Self::Target {
		&self.0
	}
}
/// release memory on drop
impl Drop for NodeOwner {
	fn drop(&mut self) {
		unsafe {
			mpv_free_node_contents(self as *mut Self as *mut mpv_node)
		}
	}
}
    • note, higher level APIs should not use "out" pointers like the C ffi, instead, wrappers should use MaybeUninit internally and return NodeOwner by value.

  • last, but not least, the APIs discussed so far are NOT suitable for data created and owned by rust code.

  • this can be implemented in a similar way, but I don't have the time to look into it.

    • the tricky part is how to handle ownership. the idea is use a single "root" node as the owner of all the nested data, similar to the NodeOwner type above.

    • when a piece of data is pushed/inserted, the ownership is transferred to the "root" node

    • when the "root" node is dropped, it recursively drops the nested data

    • on the surface, it seems like a paralell type hierarchy is used again, but the difference is, in this case, the rust types are completely compatible with the ffi types, so you don't need to marshal deeply-nested data across ffi boundary, you can just use the pointer directly.

  • with a bit of generic and ownership trickery (specifically, consider: Deref, AsRef, Into, Vec::into_boxed_slice(), Box::into_raw(), etc), I'm pretty sure the result API can be made very ergonomics to use.

3

There's a crate that helps implement Rust API with proper memory management on top of pointers to C structs:

4

Thank you for all the insight! I'll try to incorporate some of these.

One thing I will note is that, based on my understanding (which may be faulty), the client is expected to copy out any data passed in my mpv; any pointers are read-only, and the data may be free'd after execution of the event loop is released back to mpv. Likewise, mpv makes the same promise of never modifying any data passed in, and copying it if needed (such as assigning to a property).

For the receiving case, I could see an implementation similar to what you've laid out here, perhaps returning primitives directly, but returning a NodeRef or something similar that implements ToOwned which could be used if the crate user needs to store off values for longer than the mpv lifetime (which as I understand it, cannot be guaranteed).

For the case of sending data to mpv, ideally I'd like to keep a similar interface to what I've already got; here is a prime example of usage (this example utilizes the node_map and node_array macros I wrote, see the examples on those pages for what they evaluate to).

My main concern remains guaranteeing that any pointers to Rust-owned data remain valid until mpv copies out the data (which it does not notify the client of), though I believe it's safe to assume this happens within the function which the client calls passing the data in. Does anything need to be done to ensure this, such as Pinning?

5

Thanks, I'll take a look at this!

6

I'm actually looking deeper into this and it appears that the data pointed to is only free when the respective mpv_free* function is called. You are expected not to write to this data, but I don't think mpv will free it out from under you. In that case, perhaps a NodeRef that implements ToOwned into Node could work great, because a function such as get_property() could return a Cow<NodeRef>, coping the data into an owned Node if the user needs to write to it, but otherwise keeping it as just a set of pointers.

7

for the receiving case, another possibility is to directly implement similar methods on the ffi types generated by bindgen, if you don't mind mixing some handwritten code with machine generated code in the -sys crate.

the code I showed is just an example how the APIs could look, it all depends on the use case. for example, instead of traversal as key-value pairs, the mpv_node_map can provide hashmap-like lookup API, only it's on average linear time instead of constant time:

impl mpv_node_map {
    fn get(&self, key: &str) -> Option<&mpv_node> {
        self
            .as_keys()
            .iter()
            .position(|s| s==key)
            .and_then(|i| self.as_values().get(i))
    }
}

the node_array!() and node_map!() macros are very neat indeed. but you can implement the same API using ffi compatible types too, (you do need to re-implement required APIs of standard rust types such as Vec, HashMap, etc)

it's all trade-offs, like any other programming problems: you either use the rust standard library representation for convenience, then do a deep nested conversion at the end, or you use ffi compatible representations and do the conversion incrementally during the construction of the data structure.

an ffi compatible owned Node type for rust:

#[repr(transparent)
struct Node(mpv_node);
impl Deref for Node { ... }

here's the direct constructors.

one possible alternative is to use From, which make it easy to use generics and macros.

impl Node {
    fn none() -> Self { todo!() }
    // scalers are easy
    fn flag(value: bool) -> Self { todo!() }
    fn i64(value: value) -> Self { todo!() }
    fn f64(value: value) -> Self { todo!() }
    // heap allocated data needs conversion, example:
    fn str(value: impl Into<String>) -> Self {
        Self::cstr(CString::new(value.into()).expect("string with internal NUL")
    }
    fn cstr(value: impl Into<CString>) -> Self {
        let ptr = CString::into_raw(value.into());
        todo!()
    }
    fn node_array(value: impl Into<NodeArray>) -> Self {
        let ptr = Box::into_raw(Boxx::new(value.into()));
        todo!()
    }
    fn node_map(inner: impl Into<NodeMap>) -> Self { todo!() }
    fn byte_array(inner: Vec<u8>) -> Self { todo!() }
}

corresponding destructor:

impl Drop for Node {
    fn drop(&mut self) {
        match self.0.format {
            // do nothing for scalar values
            Format::None | Format::Flag | Format::I64 | Format::U64 => {},
            Format::String => unsafe {
                // `ptr: *mut c_char` is obtained from `CString::into_raw()`
                let ptr = self.0.u.string;
                drop(CString::from_raw(ptr));
            }
            Format::NodeArray => unsafe {
                // `ptr: *mut mpv_node_list` is obtained from `Box<NodeArray>::into_raw()`
                let ptr = self.0.u.list;
                drop(Box::<NodeArray>::from_raw(ptr));
            }
            //...
        }
    }
}

the ffi compatible NodeArray and NodeMap types are the tricky part.

there are many ways to make them idomatic to use. for example, I can implement From<Vec<Node>> and/or From<Box<[Node]>> for NodeArray, which is very cheap, so that I can use Vec to construct the nodes incrementally. this is essentially using Vec as a "builder" for NodeArray:

#[repr(transparent)]
struct NodeArray(mpv_node_list);
impl Deref for NodeArray { ... }

impl From<Box<[Node]>> for NodeArray {
    fn from(nodes: Box<[Node]>) -> Self {
        let num = nodes.len() as c_int;
        let values = Box::into_raw(nodes) as *mut mpv_node;
        Self(mpv_node_list {
            num,
            values,
            keys: null_mut(),
        })
    }
}

and the destructor:

impl Drop for NodeArray {
    fn drop(&mut self) {
        let ptr = unsafe { std::ptr::slice_from_raw_parts_mut(self.0.values, self.0.num as usize) };
        drop(Box::from_raw(ptr));
    }
}

another way is to implement Vec-like APIs directly on ArrayNode, such as push(), pop(), etc. this is trickier to implement, since mpv_node_listdoesn't have acapacity` field. one workaround is to encode the capability into the type itself:

#[repr(transparent)]
struct NodeArray<const CAP: usize>(mpv_node_list, PhantomData<[(); CAP]>);
impl<const CAP: usize> Deref for NodeArray<CAP> { ... }

constructor and destructor:

impl<const CAP: usize> NodeArray<CAP> {
    fn new() -> Self {
        const UNINIT: MaybeUninit<Node> = MaybeUninit::uninit();
        let storage = Box::new([UNINIT; CAP]);
        let ptr = Box::into_raw(storage);
        Self(mpv_node_list{
            num: 0,
            values: ptr as _,
            keys: null_mut(),
        })
    }
}
impl<const CAP: usize> Drop for NodeArray<CAP> {
    fn drop(&mut self) {
        let mut storage = Box::<[MaybeUninit<Node>; CAP]>::from_raw(self.0.values as _);
        for i = 0.. self.0.num as usize {
            unsafe { storage[i].assume_init_drop(); }
        }
    }
}

push and pop:

impl<const CAP: usize> NodeArray<CAP> {
    fn push(&mut self, node: Node) {
        assert!(self.0.num < CAP as c_int);
        unsafe {
            self.0.values.add(self.0.num as usize).write(node);
        }
        self.0.num += 1;
    }
    fn pop(&mut self) -> Option<Node> {
        if self.0.num == 0 {
            return None;
        }
        self.0.num -= 1;
        let node = unsafe {
            self.0.values.add(self.0.num as usize).read()
        };
        Some(node)
    }   
}

one downside of this approach is the generic is contagious.

now comes to the most tricky bit: NodeMap. it's interesting that the above push()/pop()-style API is probably easier for NodeMap. this example I won't use const generic for the capacity, but just give it a hard-coded value:

const CAP: usize = 8;

#[repr(transparent)]
struct NodeMap(mpv_node_list);
impl Deref for NodeMap { ... }
/// constructor
impl NodeMap {
	fn new() -> Self {
		const UNINIT: MaybeUninit<Node> = MaybeUninit::uninit();
		let values = Box::new([UNINIT; CAP]);
		let keys = Box::new([null::<c_char>(); CAP]);
		Self(mpv_node_list {
			num: 0,
			values: Box::into_raw(values) as _,
			keys: Box::into_raw(keys) as _,
		})
	}
}
/// destructor
impl Drop for NodeMap {
	fn drop(&mut self) {
		unsafe {
			let mut values = Box::<[MaybeUninit<Node>; CAP]>::from_raw(self.0.values as _);
			let keys = Box::<[*mut c_char; CAP]>::from_raw(self.0.keys as _);
			for i in 0..self.0.num as usize {
				values[i].assume_init_drop();
				drop(CString::from_raw(keys[i]));
			}
		}
	}
}
/// insert API, (I didn't bother to do "remove()")
impl NodeMap {
	fn insert(&mut self, key: impl Into<String>, value: Node) {
		let i = self.0.num as usize;
		assert!(i < CAP);
		let key = CString::new(key.into()).expect("string with internal NUL");
		unsafe {
			self.0.values.add(i).write(value);
			self.0.keys.add(i).write(CString::into_raw(key));
		}
		self.0.num += 1;
	}
}

an alternative method is to use a builder type, which doesn't have the exact memory layout but convertible (as efficiently as possible), something like:

struct NodeMapBuilder {
    keys: Vec<*const c_char>,
    values: Vec<Node>,
}

so how does the usage look like? using generics only (without macros), I imagine something like this is possible:

// the same example as the one in the docs with `node_array!()`
handle.command_node(Node::node_array([
    Node::str("frame-step"),
    Node::integer(20),
    Node::str("mute")
]));
// same data but different style
handle.command_node(
    NodeArray::from_iter([
        "frame-step".into(),
        20.into(),
        "mute",into(),
    ])
    .into()
);
// same example as the one with `node_map!()`
handle.command_node(Node::node_map([
    ("name", "show-text".into()),
    ("text", "peekaboo!".into()),
    ("duration", 500.into()),
]));
// or a different style
handle.command_node(
    NodeMap::from_iter([
        ("name", Node::str("show-text")),
        ("text", Node::str("peekaboo!")),
        ("duration", Node::i64(500)),
    ])
    .into()
);

I'd say this API is already very ergonomic, which can be improved even further with custom macros.