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).

[Cobrand]

Box's documentation: "A pointer type that uniquely owns a heap allocation of type T.".

This is regarding the "rust" ownership. This is exactly our case: we own that piece of memory, SDL only borrows it from our point of view. SDL can't change its location or even modify it without us ordering it to. If it did, re-transforming it into a Box to destroy it afterwards would be completely unsound.

(A compiler might move the bytes from the heap to the stack invalidating the pointer SDL has, because the compiler thinks it is unique.)

This is straight up false. A Box's contents will always be on the heap with an allocator associated to it a compile time if it's returned or if it belongs to anything else like a struct. It might be optimized within a function if nothing else references it but that's about it. You can make your custom allocator that uses stack or pseudo-heap instead but the default Box never will, and even then it won't be moved out of nowhere. If you want to be really semantic about it, you can use a Pin<Box<_>> which guarantees at compile time that the pointer will never be changed, but this is unnecessary in our case because nothing accesses it but us.

Also, storing a pointer instead of a Box has the advantage that unsafe must be used when potentially causing unsoundness.

Using unsafe when unnecessary is not an advantage. This is just extra steps of unsafe and the potential to have more safety bugs, just for the sake of it. So I stand by it, replace it by a Box and remove the unsafe at creation+deletion.

[antonilol]

we own that piece of memory

Not uniquely if SDL holds a pointer too, moving the Box (into the newly created Music or because users can (semantically) move Music) invalidates other pointers pointing to it. See this example using Miri: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=4db678c43f4ab55e5f8d96543f387893 (top right corner Tools > Miri)

The unique guarantee that Box asserts is too strong in this case, so I suggested to use NonNull.

If it did, re-transforming it into a Box to destroy it afterwards would be completely unsound.

Transforming back to a Box is fine as long as SDL does not read from pointers derived from that memory anymore, which does not happen, Mix_FreeMusic is the last SDL call that could use it, it is called in drop and Box::from_raw right after it.

If you want to be really semantic about it, you can use a Pin<Box<_>>

Box<[u8]> is Unpin so this doesn't do anything. I also don't want to make Music !Unpin and let users only handle pinned pointers to Music (Pin<Box<Music>>, Pin<&mut Music>, etc), this allows storing a Box in the struct but this is not worth it in my opinion.

[Cobrand]

You can clearly see that the underlying pointer does not move when moving a box from a variable to a struct: https://play.rust-lang.org/?version=stable&mode=release&edition=2024&gist=32866dca6fdba1b96a1429c3b90b7565 If the pointer referencing it did move, thousands of programs in the wild would break

MIRI probably reports this as an error as the "worst case" scenario as in "this Box can still be changed by the user while a pointer still refers to it", but a pointer on the heap will NOT change because a Box is moved from a struct to another.

Using Pin is indeed stupid because this doesn't bring anything in our case, but this would be kind of a marker to show that the Box used in the struct cannot be swapped with something else to be dropped later. It would only be useful for the programmer thinking "well, why is this Pin there? doesn't mean anything here". It's still stupid though.

[renski-dev]

MIRI probably complains because of this, from the docs of Unique:

/// Unlike *mut T, Unique<T> behaves "as if" it were an instance of T.
/// It implements Send/Sync if T is Send/Sync. It also implies
/// the kind of strong aliasing guarantees an instance of T can expect:
/// the referent of the pointer should not be modified without a unique path to
/// its owning Unique.

The compiler may skip allocating memory if it decides there is no need for the value to ever exist on the heap. It can be quite clever about it, for example here it knows that the value is only ever observed after being moved to the stack. Leaving the value in the Box does require an allocation, though.

However, I can't find or come up with an example where the compiler makes such an assumption when passing a pointer to a C function (see the analogous case here).

In general, if the docs say it must be unique otherwise we're in UB land, it's wise to do the semantically correct thing, as I've had a case where UB manifested only with LTO enabled, and it was not a fun time.

Edit: fixed links

[antonilol]

You can clearly see that the underlying pointer does not move when moving a box from a variable to a struct: https://play.rust-lang.org/?version=stable&mode=release&edition=2024&gist=32866dca6fdba1b96a1429c3b90b7565

Pointer addresses match, but clearly not the provenance (for more info see https://doc.rust-lang.org/std/ptr/index.html#provenance), as miri is able to flag access through one of the pointers as invalid, miri keeps track of both address and provenance.
There is a reason why the borrow checker does not allow you to move a Box while having references borrowed from it, it would invalidate the references.

[antonilol]

In general, if the docs say it must be unique otherwise we're in UB land, it's wise to do the semantically correct thing, as I've had a case where UB manifested only with LTO enabled, and it was not a fun time.

This is the whole problem with UB, even if the program works now with the UB, it might break with different compiler flags, on a different platform, later in a different compiler version, etc.