Memory corruption in SDL Audio

[univta0001]

Hi, i am porting my SDL2 code to SDL3 code. It will check that the audio is initialized or not. If not, the program will skip the audio portion. Otherwise the audio is played using queue instead of callback.

However, recently found out that in the line "let audio_stream = if let Ok(audio_subsystem) = audio_subsystem {" is causing issue. The audio_subsystem will be deallocated too early and causing ACCESS_VIOLATION or HEAP_CORRUPTION error. This can be solved by changing to "let audio_stream = if let Ok(ref audio_subsystem) = audio_subsystem {".

This issue did not happened in SDL2 crate but in SDL3 crate.

Shown below is the code to demonstrate the problem.

use sdl3::Error;
use sdl3::audio::{AudioFormat, AudioSpec, AudioStream};
use std::time::Duration;

fn gen_wave(bytes_to_write: i32) -> Vec<i16> {
    // Generate a square wave
    let tone_volume = 1_000i16;
    let period = 44100 / 256;
    let sample_count = bytes_to_write;
    let mut result = Vec::new();

    for x in 0..sample_count {
        result.push(if (x / period) % 2 == 0 {
            tone_volume
        } else {
            -tone_volume
        });
    }
    result
}

fn main() -> Result<(), Error> {
    let sdl_context = sdl3::init()?;
    let audio_subsystem = sdl_context.audio();

    let desired_spec = AudioSpec {
        freq: Some(44100),
        channels: Some(1),
        format: Some(AudioFormat::s16_sys()),
    };

    // To fix the crash
    // let audio_stream = if let Ok(ref audio_subsystem) = audio_subsystem {
    let audio_stream = if let Ok(audio_subsystem) = audio_subsystem {
        let audio_device = audio_subsystem.open_playback_device(&desired_spec)?;
        let stream = AudioStream::open_device_stream(audio_device.id(), Some(&desired_spec))?;
        stream.resume().expect("Unable to resume playback");
        Some(stream)
    } else {
        None
    };


    if let Some(stream) = &audio_stream {
        // Put data to stream
        let wave = gen_wave(44100 * 2);

        let mut byte_buffer: Vec<u8> = Vec::new();
        for &item in &wave {
            let data = item.to_ne_bytes();
            byte_buffer.push(data[0]);
            byte_buffer.push(data[1]);
        }
        let _ = stream.put_data(&byte_buffer);
    }
    
    // Play for 1.0 seconds
    std::thread::sleep(Duration::from_millis(1000));

    Ok(())
}

[revmischa]

Okay thanks! Want to contribute a fix?

[antonilol]

Looks like the problem is that AudioDevice and/or AudioStream do not require that AudioSubsystem is still around, causing it to call SDL_QuitSubSystem too early.
There are basically two ways of fixing this:

• Bind AudioDevice to AudioSubsystem with a lifetime parameter, the compiler will enforce that any AudioDevice is dropped after AudioSubsystem (breaking change, might require users to fix lifetime errors, otherwise no performance overhead)
• Keep an AudioSubsystem in AudioDevice, SDL_QuitSubSystem will be called when the last of either AudioDevice or AudioSubsystem is dropped (not a breaking change, possible performance overhead because of the extra reference counting)

[antonilol]

Both are possible (letting users choose) by adding a new methods (with something like a _borrowed suffix) that do the lifetime stuff, and change the existing method to do the non breaking refcounting.

[revmischa]

I don't think AudioDevice should be dropped very frequently so not too worried about adding ref counting there

[antonilol]

And what about AudioStream? A quick look at the code suggests that they can be bound to zero or one AudioDevice

[revmischa]

Hm, that I'm less sure of but ref counting seems pretty cheap