Fix string use-after-free

[zekesonxx]

Snippet of example code:

let count: i64 = mpv.get_property("metadata/list/count")?;
println!("Retrieving {} properties", count);
for i in 0..count {
    let key = mpv.get_property::<&str>(&format!("metadata/list/{}/key", i))?;
    let value = mpv.get_property::<&str>(&format!("metadata/list/{}/value", i))?;
    println!("key:'{}', value:'{}'", key, value);
}

which resulted in strange errors:

key:'forev', value:'foreverrr'
key:'mc chr', value:'mc chris'

(full metadata list example)

and bloody confusing gdb printouts (breakpoint on the println line):

(gdb) print key
$1 = &str {data_ptr: 0x7fffdf0323b0 "foreverrr\000", length: 5}
(gdb) print value
$2 = &str {data_ptr: 0x7fffdf032270 "foreverrr\000", length: 9}

---

Some debugging later, I've discovered the original code goes something like:

1. Pass char_ptr (a nullptr) to libmpv
2. Call libmpv, char_ptr is now pointing to libmpv-allocated memory
3. Pass char_ptr into CStr, the new CStr points to libmpv-allocated memory
4. Call to_str on the CStr, the new &str points to libmpv-allocated memory
5. Call mpv_free(char_ptr), mpv frees the memory, except we still have a pointer to it.
6. Return the &str to whoever is calling mpv-rs, which now points to freed memory (big no-no)

Obviously, that's really not okay. This changes &str to a String, and changes OsdString(&str) to OsdString(String). That way we (Rust) own it, and are safe to tell mpv to free the memory it gave us.

---

This change is 

[Cobrand]

I ... I don't even know how I could have written that, when looking at the code the first thing I noticed is that we mpv_free() something we return...

I think we need a complete overhaul of this crate at this point, I created this in my early rust days and this could be so much better with the toolings that we have now. The C bindings are mixed with Rust code, we return raw C enum sometimes... definitely less than ideal.

For instance, there is nothing wrong with setting values with &str, but "getting" &str isn't possible, we have to use a String there. I think the ideal would be to have 2 traits, 1 "AsMpvFormat" and another "FromMpvFormat". Copy-based values like i64 and f64 would be unchanged, but str & String would have drastically different implementations. I think those kinds of changes definitely show that we need a overhaul overall.

The good news is that the core API of mpv is pretty short and we won't have many things to rewrite.

Anyway, thanks for noticing this! I think I'll probably try to re-do that crate from scratch over the next few days, or I'll try to adapt https://github.com/ParadoxSpiral/mpv-rs (a version which @ParadoxSpiral started working on a few months ago) to release something stable and secure on crates.io, but still with a similar API so that people upgrading from mpv-rs 0.2 to 0.3 won't be lost too easily.

If you're interested, we can try to rewrite that together, maybe we'll have better ideas with 2 minds instead of 1.

[marioortizmanero]

Hi @Cobrand, are there any news about the fork? Which repo is supposed to be the "official" one? It'd be great to have an explanation about the situation, since there hasn't been much activity in here lately. If one of the repos are outdated it'd be a good idea to archive it and redirect to the new one.

[Cobrand]

@marioortizmanero There is no "official" one, I was just the first one to implement this crate a long time ago. As mentioned, a big overhaul would be needed to make this crate cleaner, but I won't be the one doing this unfortunately. I don't know how the mpv of ParadoxSpiril holds ( https://github.com/ParadoxSpiral/mpv-rs ), maybe it can considered the official one if he wants, but regardless you can consider this crate as abandonned: I am willing to give the access to crates.io and this repository to anyone who asks, but I don't have the time to take care of this anymore. Sorry about this!

[marioortizmanero]

Don't worry! Thanks a lot for your work :) I'll open an issue in there to let them know.

Edit: I made a comment at https://github.com/ParadoxSpiral/mpv-rs/issues/1