Skip to content

Commit 27ab9e7

Browse files
committed
audio: plug leak and skip redundant copy in mixer add_stream
audio_driver_mixer_add_stream unconditionally malloc+memcpy'd the incoming buf before handing it to the format loader. This was both wasteful and leaky: * For OGG/FLAC/MP3/MOD, the loader stores the pointer into sound->types.XXX.data and audio_mixer_destroy() free()s it on sound completion. With the internal memcpy, that destroy path freed the internal copy, not the caller-owned original - and the original had no free path at all. Every one of the ten callers in task_audio_mixer.c passes img->buf without freeing it after the call, expecting the mixer to take ownership, so every loaded mixer sound leaked the entire source buffer (typical size: tens of kB to a few MB per sound). * For WAV, the loader converts to PCM and free()s the input buffer inline, so only the per-call copy was wasted - no leak, just the duplicate. task_translation.c has the opposite expectation: it passes response->sound_data and explicitly free()s it after the call, requiring a copy. Resolve the ambiguity with an explicit buf_owned flag on audio_mixer_stream_params_t: * true => add_stream takes ownership, either free()ing directly (WAV) or via audio_mixer_destroy (OGG/FLAC/MP3/MOD). On any early failure (bad params, no free slot, loader returns NULL), the buffer is freed so ownership is respected uniformly. * false => add_stream makes a private copy (previous behavior). Caller retains ownership of the original. All ten task_audio_mixer.c callers are updated to set buf_owned = true; the single task_translation.c caller sets it false explicitly with an explanatory comment. This also restructures the early-failure paths to a single 'error:' label so the ownership discipline is expressed once and can't drift. Scope note: the modified API (audio_driver_mixer_add_stream, the audio_mixer_stream_params_t struct) is frontend-only. It lives in audio/audio_driver.{c,h}, is not exposed via libretro.h, is not part of libretro-common, and cannot be reached from cores. The lower-level audio_mixer_* API in libretro-common/audio/audio_mixer.c is unchanged. Thread-safety: add_stream runs on the main thread (task callbacks fire from task_queue_check, which is called from retroarch.c's main loop). audio_mixer_mix runs on the main thread in the normal case and on the audio thread when a core uses the libretro audio-callback extension. The existing code had no locking around mixer slot state; this patch neither introduces nor closes that pre-existing race. The only state change relative to the old code is which pointer lives in stream->buf - same publication visibility, same memory ordering concerns as before.
1 parent b9d45ba commit 27ab9e7

4 files changed

Lines changed: 45 additions & 7 deletions

File tree

audio/audio_driver.c

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1419,7 +1419,7 @@ bool audio_driver_mixer_add_stream(audio_mixer_stream_params_t *params)
14191419
void *buf = NULL;
14201420

14211421
if (params->stream_type == AUDIO_STREAM_TYPE_NONE)
1422-
return false;
1422+
goto error;
14231423

14241424
switch (params->slot_selection_type)
14251425
{
@@ -1436,17 +1436,27 @@ bool audio_driver_mixer_add_stream(audio_mixer_stream_params_t *params)
14361436
default:
14371437
if (!audio_driver_mixer_get_free_stream_slot(
14381438
&free_slot, params->stream_type))
1439-
return false;
1439+
goto error;
14401440
break;
14411441
}
14421442

14431443
if (params->state == AUDIO_STREAM_STATE_NONE)
1444-
return false;
1445-
1446-
if (!(buf = malloc(params->bufsize)))
1447-
return false;
1444+
goto error;
14481445

1449-
memcpy(buf, params->buf, params->bufsize);
1446+
/* If the caller transferred buffer ownership, use it directly;
1447+
* otherwise make a private copy. The OGG/FLAC/MP3/MOD loaders
1448+
* retain the pointer for streaming decode, so taking ownership
1449+
* avoids a needless per-sound duplication and a matching leak
1450+
* (the original caller-owned buffer has nowhere to be freed,
1451+
* since the mixer frees only the internal copy). */
1452+
if (params->buf_owned)
1453+
buf = params->buf;
1454+
else
1455+
{
1456+
if (!(buf = malloc(params->bufsize)))
1457+
return false;
1458+
memcpy(buf, params->buf, params->bufsize);
1459+
}
14501460

14511461
switch (params->type)
14521462
{
@@ -1521,6 +1531,14 @@ bool audio_driver_mixer_add_stream(audio_mixer_stream_params_t *params)
15211531
audio_driver_st.mixer_streams_playing++;
15221532

15231533
return true;
1534+
1535+
error:
1536+
/* On early failure (bad params / no slot), honor the ownership
1537+
* contract: if the caller transferred buf to us, we free it so
1538+
* the caller does not double-free and does not leak. */
1539+
if (params->buf_owned)
1540+
free(params->buf);
1541+
return false;
15241542
}
15251543

15261544
enum audio_mixer_state audio_driver_mixer_get_stream_state(unsigned i)

audio/audio_driver.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,14 @@ typedef struct audio_mixer_stream_params
7373
enum audio_mixer_stream_type stream_type;
7474
enum audio_mixer_type type;
7575
enum audio_mixer_state state;
76+
/* If true, add_stream takes ownership of buf and the caller must
77+
* not free or otherwise reference it after the call returns. The
78+
* buffer will be free()d either directly (WAV, once converted to
79+
* PCM) or when the mixer sound is destroyed (OGG/FLAC/MP3/MOD,
80+
* which retain the buffer for streaming decode).
81+
* If false, add_stream copies buf internally and the caller retains
82+
* ownership of the original memory. */
83+
bool buf_owned;
7684
} audio_mixer_stream_params_t;
7785
#endif
7886

tasks/task_audio_mixer.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ static void task_audio_mixer_handle_upload_ogg(retro_task_t *task,
116116
params.bufsize = img->bufsize;
117117
params.cb = NULL;
118118
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
119+
params.buf_owned = true;
119120

120121
audio_driver_mixer_add_stream(&params);
121122

@@ -147,6 +148,7 @@ static void task_audio_mixer_handle_upload_ogg_and_play(retro_task_t *task,
147148
params.bufsize = img->bufsize;
148149
params.cb = NULL;
149150
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
151+
params.buf_owned = true;
150152

151153
audio_driver_mixer_add_stream(&params);
152154

@@ -178,6 +180,7 @@ static void task_audio_mixer_handle_upload_flac(retro_task_t *task,
178180
params.bufsize = img->bufsize;
179181
params.cb = NULL;
180182
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
183+
params.buf_owned = true;
181184

182185
audio_driver_mixer_add_stream(&params);
183186

@@ -209,6 +212,7 @@ static void task_audio_mixer_handle_upload_flac_and_play(retro_task_t *task,
209212
params.bufsize = img->bufsize;
210213
params.cb = NULL;
211214
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
215+
params.buf_owned = true;
212216

213217
audio_driver_mixer_add_stream(&params);
214218

@@ -240,6 +244,7 @@ static void task_audio_mixer_handle_upload_mp3(retro_task_t *task,
240244
params.bufsize = img->bufsize;
241245
params.cb = NULL;
242246
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
247+
params.buf_owned = true;
243248

244249
audio_driver_mixer_add_stream(&params);
245250

@@ -271,6 +276,7 @@ static void task_audio_mixer_handle_upload_mp3_and_play(retro_task_t *task,
271276
params.bufsize = img->bufsize;
272277
params.cb = NULL;
273278
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
279+
params.buf_owned = true;
274280

275281
audio_driver_mixer_add_stream(&params);
276282

@@ -302,6 +308,7 @@ static void task_audio_mixer_handle_upload_mod(retro_task_t *task,
302308
params.bufsize = img->bufsize;
303309
params.cb = NULL;
304310
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
311+
params.buf_owned = true;
305312

306313
audio_driver_mixer_add_stream(&params);
307314

@@ -333,6 +340,7 @@ static void task_audio_mixer_handle_upload_mod_and_play(retro_task_t *task,
333340
params.bufsize = img->bufsize;
334341
params.cb = NULL;
335342
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
343+
params.buf_owned = true;
336344

337345
audio_driver_mixer_add_stream(&params);
338346

@@ -365,6 +373,7 @@ static void task_audio_mixer_handle_upload_wav(retro_task_t *task,
365373
params.bufsize = img->bufsize;
366374
params.cb = NULL;
367375
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
376+
params.buf_owned = true;
368377

369378
audio_driver_mixer_add_stream(&params);
370379

@@ -396,6 +405,7 @@ static void task_audio_mixer_handle_upload_wav_and_play(retro_task_t *task,
396405
params.bufsize = img->bufsize;
397406
params.cb = NULL;
398407
params.basename = (img->path && *img->path) ? strdup(path_basename_nocompression(img->path)) : NULL;
408+
params.buf_owned = true;
399409

400410
audio_driver_mixer_add_stream(&params);
401411

tasks/task_translation.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,8 @@ static void handle_translation_response(
559559
params.bufsize = response->sound_size;
560560
params.cb = NULL;
561561
params.basename = NULL;
562+
/* Caller frees response->sound_data below; request a copy. */
563+
params.buf_owned = false;
562564

563565
audio_driver_mixer_add_stream(&params);
564566
}

0 commit comments

Comments
 (0)