andelf / rust-sdl2_mixer

Rust bindings for sdl2_mixer
Apache License 2.0
13 stars 17 forks source link

Undefined behavior when dropping Music after mix_quit has been called #32

Closed crumblingstatue closed 8 years ago

crumblingstatue commented 9 years ago

If you call mix::quit before dropping a Music, dropping it will trigger undefined behavior and causes a segmentation fault on my system.

This is unacceptable, as this can be caused by safe Rust code.

One solution is to just let the music leak if the library is not initialized anymore.

Another solution is to change the architecture of the library, as was done with rust-sdl2, to be able to tie the lifetime of a music to the lifetime of the sdl2_mixer context.

I haven't checked, but this issue most likely also applies to Chunk and maybe some other types that implement Drop.

Here is a minimal test case:

extern crate sdl2 as sdl;
extern crate sdl2_mixer as mix;

fn main() {
    let _sdl_ctx = sdl::init().everything().unwrap();
    mix::init(mix::INIT_MODPLUG);
    mix::open_audio(mix::DEFAULT_FREQUENCY, mix::DEFAULT_FORMAT, 2, 4096).unwrap();
    // http://modarchive.org/index.php?request=view_by_moduleid&query=96492
    let mus = mix::Music::from_file("sdv_3.mod".as_ref()).unwrap();
    mus.play(0).unwrap();
    ::std::thread::sleep_ms(3000);
    mix::quit();
}
andelf commented 9 years ago

Thanks. I prefer to change ths architecture of the library.

Can you post the minimal fail code here?

crumblingstatue commented 9 years ago

@andelf I edited the original post and added a test case.

andelf commented 9 years ago

:( The code runs ok under my macbook. sdl_* libs installed by homebrew.

crumblingstatue commented 9 years ago

Whether it runs fine for you specifically or not, the fact still remains that it triggers undefined behavior. It might run fine on some systems, it might crash on others. It might silently corrupt the program state on yet others. Undefined behavior must be eliminated, and it is forbidden in Rust to let safe code trigger it.

andelf commented 9 years ago

I'll try to tie the lifetime of a music to the lifetime of the sdl2_mixer context.

MarkDDR commented 8 years ago

With that last pull request, this issue should be solved and can be closed

crumblingstatue commented 8 years ago

With that last pull request, this issue should be solved and can be closed

Alright, I can't reproduce this issue anymore (in fact, it even works without sdl::init() or mix::init() being called), so I'm going to close it.