Skip to content

Commit 01a11ac

Browse files
committed
audio/drivers/sdl_audio: NULL-check fifo_new returns in microphone and speaker init
Two parallel unchecked fifo_new() return values in sdl_audio.c. === sdl_microphone_open_mic === mic->sample_buffer = fifo_new(bufsize); if (tmp) { fifo_write(mic->sample_buffer, tmp, bufsize); ... } fifo_new can return NULL on OOM (see libretro-common/queues/ fifo_queue.c - malloc wrapper + inner fifo_initialize_internal can both fail). The 'if (tmp)' block guards the fifo_write call only by whether the separate 'tmp' calloc succeeded - if tmp succeeded but fifo_new failed, fifo_write(NULL, tmp, bufsize) NULL-derefs on the first 'buffer->end' access. Even if we skip the prefill, hot-path audio callbacks later (sdl_audio_record_cb, sdl_microphone_read via the FIFO_READ_AVAIL macro + fifo_read) index into mic->sample_buffer unconditionally and crash the same way on the next incoming audio callback. Fix: NULL-check mic->sample_buffer; 'goto error' on OOM. The existing 'error' label only did 'free(mic); return NULL'. This worked for the pre-existing goto site (device_id == 0, reached before mic->lock and mic->cond get allocated) but leaks mic->lock, mic->cond, and mic->device_id for the new goto site (reached after lines 230-231 slock_new/scond_new and line 192 SDL_OpenAudioDevice). Expanded the error label to: if (mic->device_id > 0) SDL_CloseAudioDevice(mic->device_id); #ifdef HAVE_THREADS slock_free(mic->lock); scond_free(mic->cond); #endif free(mic); slock_free/scond_free are NULL-tolerant so the old goto site (device_id == 0) path is unchanged: device_id guard short- circuits the close, lock/cond are NULL-as-zero-initialised by the outer calloc so slock_free/scond_free no-op. New goto site gets proper cleanup. === sdl_audio_init === sdl->speaker_buffer = fifo_new(bufsize); if (tmp) { fifo_write(sdl->speaker_buffer, tmp, bufsize); ... } Same pattern, same bug. Hot-path sdl_audio_write (lines ~623 onwards) uses FIFO_WRITE_AVAIL and fifo_write on speaker_buffer unconditionally; sdl_audio_playback_cb similarly uses fifo_read. Fix: NULL-check sdl->speaker_buffer. Bail via sdl_audio_free which already NULL-checks speaker_device (line ~716), NULL- checks speaker_buffer (line ~721), and HAVE_THREADS frees are NULL-tolerant. Only need to explicitly free 'tmp' because ownership hasn't transferred to the fifo yet. === Not a bug, verified clean === Every other alloc site in the input and audio driver sweeps triaged cleanly: * input/drivers/udev_input.c - 3 sites (touch state arrays), all NULL-checked and coupled by sibling 'if (!ptr)' blocks. * input/drivers/winraw_input.c - 2 sites NULL-checked via goto error. * input/drivers/rwebinput_input.c - 1 site (already fixed in an earlier commit in this series - realloc-to-tmp). * input/drivers_hid/{wiiusb_hid,libusb_hid,iohidmanager_hid}.c - 4 sites total, all NULL-checked. * input/drivers_keyboard/keyboard_event_xkb.c - 1 site, 'if (!(x = calloc(...)))' idiom. * input/drivers_joypad/xinput_hybrid_joypad.c - 1 site NULL- checked. * input/drivers/{switch,qnx,android}_input.c - 3 sites, all use the 'if (!(x = calloc(...)))' idiom. * audio/drivers/tinyalsa.c - 2 sites, -ENOMEM return + NULL- check. * audio/drivers/coreaudio.c - 2 sibling sites NULL-checked. * audio/drivers/{xenon360,switch,ps3,pipewire,alsathread, alsa}_audio.c - each single-site, NULL-checked via the driver-init pattern. === Thread-safety === sdl_microphone_open_mic and sdl_audio_init both run on the main thread during driver init, before the SDL audio callback thread starts. The audio callbacks (sdl_audio_record_cb, sdl_audio_playback_cb) are the reason the speaker/mic buffer consistency matters - but they don't start firing until SDL_PauseAudioDevice(..., false) which is the last call before both init functions return. No lock discipline changes. === Reachability === Both sites fire once per audio driver init, i.e. at RetroArch startup with the SDL audio driver selected, and again on any audio-settings change that re-inits the driver (sample rate change, latency change). fifo_new's allocation is bounded by 'device_spec.samples * 2-4 * bytes_per_sample' - a few kilobytes on typical configs, bigger for high-latency setups but never large enough to be OOM-prone on desktop. On handheld/embedded builds with memory-tight configs this remains a realistic fault site.
1 parent 55bd49c commit 01a11ac

1 file changed

Lines changed: 45 additions & 0 deletions

File tree

audio/drivers/sdl_audio.c

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,22 @@ static void *sdl_microphone_open_mic(void *driver_context, const char *device,
241241

242242
RARCH_DBG("[SDL audio] Initialized microphone sample queue with %u bytes.\n", bufsize);
243243

244+
/* NULL-check fifo_new: hot-path audio callbacks
245+
* (sdl_microphone_read_cb, sdl_microphone_read) index into
246+
* mic->sample_buffer unconditionally via FIFO_READ_AVAIL and
247+
* fifo_read/fifo_write. fifo_* NULL-deref on a NULL buffer
248+
* pointer (see libretro-common/queues/fifo_queue.c). Bail to
249+
* the existing 'error' label on OOM - it frees 'mic' and
250+
* returns NULL, same as the outer calloc-failure path at the
251+
* top of this function. 'tmp' is freed by the 'if (tmp)'
252+
* block below, not here (calloc may have succeeded even if
253+
* fifo_new failed, so defer tmp cleanup to after the guard). */
254+
if (!mic->sample_buffer)
255+
{
256+
free(tmp);
257+
goto error;
258+
}
259+
244260
if (tmp)
245261
{
246262
fifo_write(mic->sample_buffer, tmp, bufsize);
@@ -251,6 +267,19 @@ static void *sdl_microphone_open_mic(void *driver_context, const char *device,
251267
return mic;
252268

253269
error:
270+
/* HAVE_THREADS may have init'd mic->lock and mic->cond between
271+
* the SDL_OpenAudioDevice call and the fifo_new below.
272+
* slock_free/scond_free are NULL-tolerant so the earlier goto
273+
* error site (device_id == 0) where these are still NULL is
274+
* fine too. 'mic' is always non-NULL at this label because
275+
* the calloc at the top of the function returns early on
276+
* failure without goto'ing here. */
277+
if (mic->device_id > 0)
278+
SDL_CloseAudioDevice(mic->device_id);
279+
#ifdef HAVE_THREADS
280+
slock_free(mic->lock);
281+
scond_free(mic->cond);
282+
#endif
254283
free(mic);
255284
return NULL;
256285
}
@@ -591,6 +620,22 @@ static void *sdl_audio_init(const char *device,
591620
tmp = calloc(1, bufsize);
592621
sdl->speaker_buffer = fifo_new(bufsize);
593622

623+
/* NULL-check fifo_new: hot-path audio callbacks
624+
* (sdl_audio_playback_cb, sdl_audio_write) index into
625+
* sdl->speaker_buffer unconditionally via FIFO_WRITE_AVAIL /
626+
* FIFO_READ_AVAIL and fifo_read/fifo_write. fifo_* NULL-deref
627+
* on a NULL buffer. Bail via sdl_audio_free which already
628+
* handles partial init state (speaker_device guard at line
629+
* ~716, speaker_buffer guard at line ~721, HAVE_THREADS locks
630+
* nullable). 'tmp' is owned here on failure since we haven't
631+
* transferred it to the fifo yet. */
632+
if (!sdl->speaker_buffer)
633+
{
634+
free(tmp);
635+
sdl_audio_free(sdl);
636+
return NULL;
637+
}
638+
594639
if (tmp)
595640
{
596641
fifo_write(sdl->speaker_buffer, tmp, bufsize);

0 commit comments

Comments
 (0)