Add better support for loading SDL2 mixer sounds and music from memory

[kaphula]

Adds support for loading music and sound files directly from memory regardless of their file format.

I removed the static lifetime restriction from Music and transformed it to similar function as Chunk's counterpart. This should be fine?

Fixes: #1482

How should the crate versions be updated, bump both sdl2 and sdl2-sys to 0.38.0?

[kaphula]

Okay, I can already pretty much say that the Music::from_bytes does not work like this as I managed to make it segfault. from_static_bytes needs to remain its own thing with static lifetime and from_bytes needs to use SDL_RWFromMem internally I guess. I will see if I can get it to work.

[kaphula]

I added different variants for different scenarios when Music is used from bytes.

• If music is created from a slice of bytes, then the bytes' lifetime is tied to the returned Music instance. Music::from_bytes
• If music is created from a slice of static bytes, then the bytes' lifetime is static and it is not an issue with Music. Music::from_static_bytes
• If music is created from owned bytes, then the returned Music instance takes ownership of the given bytes and drops them when the Music instance is dropped. Music::from_owned_bytes

[Cobrand]

Leave the sdl2 and sdl2-sys version as-is (0.37), they will be bumped just before a release on crates.io.

Music::from_static_bytes is redundant with from_bytes, it should be removed. What didn't work and caused your segfault is that you made a Music<'static> from &'a [u8], but with just the from_bytes impl you can create both Music<'static> from &'static [u8] and Music<'a> from &'a [u8].

[kaphula]

Good point. from_static_bytes is now gone and version is back to 0.37.

[Cobrand]

Looks good, but now there is a breaking change because from_static_bytes does not exist anymore.

Could you re-add from_static_bytes but as deprecated? Something like

    #[deprecated(since="0.38.0", note="use `from_bytes` instead")]
    pub fn from_static_bytes(buf: &'static [u8]) -> Result<Music<'static>, String> {
        Self::from_bytes(buf)
    }

And finally, can you add a small line to the changelog? Technically it doesn't break anything, but you did implement a more generic from_bytes.

[antonilol]

I don't think it is possible unless you implement some hard to use function callback, or have the function block for the duration of the music.
When creating the Music struct with some lifetime, there is no way of enforcing that the lifetime is longer than the duration of the music. You also can't rely on a Drop impl stopping the music before its lifetime ends.

An example to show it is unsound would look something like this:

let bytes = vec![0,1,2,3]; // etc...
let mus = Music::from_bytes(&bytes);
mus.play(-1);
core::mem::forget(mus); // do not run drop
core::mem::forget(bytes);
// sleep for some time like 1 second to give it some time to read from the already dropped Vec

[Cobrand]

I don't think it is possible unless you implement some hard to use function callback, or have the function block for the duration of the music. When creating the Music struct with some lifetime, there is no way of enforcing that the lifetime is longer than the duration of the music. You also can't rely on a Drop impl stopping the music before its lifetime ends.

An example to show it is unsound would look something like this:

let bytes = vec![0,1,2,3]; // etc...
let mus = Music::from_bytes(&bytes);
mus.play(-1);
core::mem::forget(mus); // do not run drop
core::mem::forget(bytes);
// sleep for some time like 1 second to give it some time to read from the already dropped Vec

I think you're right on the unsoundness, but that happens for all Music<'a>, not just the one created from this function. I think at this point the play and all playback related commands of Mixer have a flawed API, making them unsound if not using 'static. We probably need an overhaul on that front.

[antonilol]

The newly added examples use include_bytes!() which just gets bytes from a file at compile time that becomes &'static [u8] at runtime (embedded in binary). This does not need from_bytes and works fine with from_static_bytes. The need for from_bytes would be for things like receiving audio files from the network or some other way where they cannot be embedded in the binary or read from a file.

[Cobrand]

This goes beyond just this PR, this whole API is unsound. Any Music<'a> is unsound.

I haven't tried, but I'm 90% sure this is a use-after-free:

let buffer: Vec<u8> = load_some_music_bytes(); // some music here as compressed mp3 for example
let music: Music = Music::from_bytes(&buffer);
music.play(-1);
drop(music);
drop(buffer);
// use after free

The reason is that the buffer is read from SDL2 but not copied like Chunks are. So if the buffer drops, SDL_Mixer is reading free'd data. Since there is nothing tying a music playback to a lifetime, we can drop both the music and the buffer while it is still being used.

We can do this with anything that generates a Music<'a>, not just a from_bytes. Because of this, the whole lifetime on Music is useless, unsound and actually makes you think that the API handles lifetimes properly, when it does not. So we need a overhaul of the SDL_Mixer API.

[antonilol]

Any Music<'a> is unsound.

We can do this with anything that generates a Music<'a>, not just a from_bytes. Because of this, the whole lifetime on Music is useless, unsound and actually makes you think that the API handles lifetimes properly, when it does not.

There is an exception, a function like fn from_bytes(bytes: &[u8], f: impl FnOnce(Music<'_>)) can be sound (using catch unwind) because it can in all cases stop the music before the end of the lifetime. The function calls the callback and then stops the music if it is still playing. The lifetime ensures the Music object does not escape the callback.
In a real API you probably also want some mechanism to allow users to return something, like fn from_bytes<R>(bytes: &[u8], f: impl FnOnce(Music<'_>) -> R) -> R.

[Cobrand]

But your solution, even if it works, is really hacky and unergonomic for Rust. A real solution would be to have a MusicPlayback<'a>, which lifetime depends on Music<'a>. Something like fn play(&'a self, loops: i32) -> MusicPlayback<'a> in Music. This would prevent all cases of the music being dropped while it is still playing, and then we would need to implement Drop on MusicPlayback that stops the music, and maybe another method is_playing on MusicPlayback to see if it's stopped and safe to drop.

Either way, the current Mixer's Music API does not make sense and needs an overhaul.

[kaphula]

If I understand this correctly, if stopping is delegated to MusicPlayback<'a> 's drop function and you create 2 music objects and use them like this:

let mus1_playback = mus1.play(-1);
mus2.play(-1);  
drop(mus1_playback);

The mus1 stops its playback by SDL2 internally when mus2 starts its playback. However, when mus1_playback is dropped, its internal state is still set to be playing, and thus it will halt music playback globally causing mus2 to be stopped even though it should be playing. I also do not quite get how you would use this in practice since you may have to start the playback of music object in some scope that you exit immediately causing the playback to be dropped instantly, stopping the music. (EDIT: I added playback handle to the code)

The idea of trying to force the API to be safe without manual calls to freeing loaded music seems challenging.

Would not this issue be solved temporarily for now if the API offered only Music::from_static_bytes, Music::from_file and Music::from_owned_bytes as safe functions, and Music::from_bytes would be marked as unsafe with documentation stating it can cause UB in certain situations?

[Cobrand]

The mus1 stops its playback by SDL2 internally when mus2 starts its playback. However, when mus1_playback is dropped, its internal state is still set to be playing, and thus it will halt music playback globally causing mus2 to be stopped even though it should be playing. I also do not quite get how you would use this in practice since you may have to start the playback of music object in some scope that you exit immediately causing the playback to be dropped instantly, stopping the music. (EDIT: I added playback handle to the code)

It would need some logic to prevent stopping the second one when the first one gets dropped, but otherwise yes.

Would not this issue be solved temporarily for now if the API offered only Music::from_static_bytes, Music::from_file and Music::from_owned_bytes as safe functions, and Music::from_bytes would be marked as unsafe with documentation stating it can cause UB in certain situations?

good idea, but don't even add from_bytes because we don't want them to be used by someone that doesn't read the doc and gets a segfault. All safe rust should be safe with no segfaults, no exceptions. And by the one I am not even sure that Music::from_file is safe. Try playing a file and then close the file in the middle of playback, you might get a use after free here also (that's why I'm saying this whole API needs a overhaul)

[kaphula]

I actually meant that from_bytes would really be unsafe function forcing user to use unsafe in addition to reading the warnings in its documentation, but considering my current needs it can be removed completely too for now if that is desired.

It would need some logic to prevent stopping the second one when the first one gets dropped, but otherwise yes.

For this, I am not sure how that kind of logic would be possible unless you can internally ask SDL2 if it is already playing some identifiable music data since the two music instances do not know anything about each other internally.

[kaphula]

I am not sure I understand how to implement your changes. Does this look like what you want:

kaphula@7f72a73

[antonilol]

Yes, almost.

Lines 893 and 894 both create intermediate (immutable) references of which at least one (as far as I know) is unsound, it basically does Box::from_raw on a pointer made from an immutable reference.
You can create a NonNull pointer with NonNull::new_unchecked, just make sure the pointer is non null but this is always the case for pointers that were a Box, and to get the data pointer for the C call (*const u8) from the *const [u8] you can use .cast::<u8>() on the pointer.

[kaphula]

Looks better?

[antonilol]

Looks good to me, I did not check for cargo clippy warnings/errors though, reviewed on my phone.

[antonilol]

@Cobrand this may be merged after your review.
@jagprog5 you want to review too?

[Cobrand]

I don't understand the point of using NonNull<[u8]> instead of Box<[u8]> if we're going to convert it to a Box every time anyway? Otherwise I think it looks good.

Have you thoroughly tested it?

[antonilol]

I don't understand the point of using NonNull<[u8]> instead of Box<[u8]> if we're going to convert it to a Box every time anyway?

Box uniquely owns a heap allocation, here there only ever is a Box before giving a pointer to SDL, or after Mix_FreeMusic. NonNull is just a non null pointer with no uniqueness guarantee.

Have you thoroughly tested it?

Don't know if this question is directed at me, but I have done code review and the example runs and plays the sound correctly.

[Cobrand]

Ok so I tested it and because Music<'a> is impossible to build, there doesn't seem to be any soundness issue.

Box uniquely owns a heap allocation, here there only ever is a Box before giving a pointer to SDL, or after Mix_FreeMusic. NonNull is just a non null pointer with no uniqueness guarantee.

I don't understand your explanation and to me it's still exactly the same behavior as a box.

First of all Drop, which is always called, transforms owned_data into a Box to drop it. There is no scenario where Drop executes but it's not transformed into a box to be dropped and thus de-allocated.

Then if we look at when a owned_data is created, there is only one use-case and it's only coming from a Box. So in 100% of cases it goes (creation) Box -> NonNull<u8> -> Box (drop). At this point why have the NonNull middleman and not just keep Box? owned_data is not accessed by anything else or pub, so nothing can ever change it.

Option<Box<>> and Option<NonNull<>> are functionally the same here, but transforming into a NonNull just adds a layer of indirection which might be misunderstood in the future if someone wants to change it.

[antonilol]

First of all Drop, which is always called, transforms owned_data into a Box to drop it. There is no scenario where Drop executes but it's not transformed into a box to be dropped and thus de-allocated.

Right, at those places the uniqueness guarantee for Box can be upheld.

Then if we look at when a owned_data is created, there is only one use-case and it's only coming from a Box. So in 100% of cases it goes (creation) Box -> NonNull<u8> -> Box (drop). At this point why have the NonNull middleman and not just keep Box? owned_data is not accessed by anything else or pub, so nothing can ever change it.

Box contains a Unique<T> (note that this type is unstable so doc may not be entirely accurate), which has stronger guarantees than NonNull (but the same layout). Box has even stronger guarantees.
Box's documentation: "A pointer type that uniquely owns a heap allocation of type T.". I don't think this holds here because SDL is storing the pointer too somewhere. (A compiler might move the bytes from the heap to the stack invalidating the pointer SDL has, because the compiler thinks it is unique.)
Also, storing a pointer instead of a Box has the advantage that unsafe must be used when potentially causing unsoundness.

Option<Box<>> and Option<NonNull<>> are functionally the same here, but transforming into a NonNull just adds a layer of indirection which might be misunderstood in the future if someone wants to change it.

Option<Box<>> and Option<NonNull<>> in terms of bytes in memory are exactly the same. The functions converting between the two are trivial and are zero cost at runtime.
Possible misunderstanding should be fixed with documentation (comments).