Skip to content

Commit 1871bd1

Browse files
committed
record/drivers/record_ffmpeg: fix OOM bugs in audio/video buffer init, resample, and flush
Six OOM bugs across record_ffmpeg.c. Most have the same underlying pattern: av_realloc is used as 'av_realloc(x, ...)' with the result self-assigned to x, which on OOM both leaks the old buffer AND leaves an associated *_frames / size counter claiming the grown size while the allocation is the old size (or NULL). On the next call the 'size > counter' growth-guard is skipped, and a stale NULL is passed to the downstream memcpy / fifo_read / convert call. === planarize_audio: planar_buf realloc-assign-self === handle->audio.planar_buf = av_realloc(handle->audio.planar_buf, ...); if (!handle->audio.planar_buf) return; handle->audio.planar_buf_frames = handle->audio.frames_in_buffer; On OOM: planar_buf goes NULL (old buffer leaked), function returns without bumping planar_buf_frames. Next call with frames_in_buffer <= planar_buf_frames would skip the realloc branch and fall through to planarize_float/s16 with a NULL planar_buf. Fix: realloc-to-tmp. === ffmpeg_audio_resample: three stacked realloc-assign-selfs === handle->audio.float_conv = av_realloc(handle->audio.float_conv, ...); if (!handle->audio.float_conv) return; handle->audio.float_conv_frames = aud->frames; /* ... */ handle->audio.resample_out_frames = aud->frames * ratio + 16; handle->audio.resample_out = av_realloc(handle->audio.resample_out, ...); if (!handle->audio.resample_out) return; /* ... */ handle->audio.fixed_conv_frames = MAX(...); handle->audio.fixed_conv = av_realloc(handle->audio.fixed_conv, ...); Same pattern, three times in one 'if (aud->frames > float_conv_frames)' block. Worst case: first realloc succeeds and bumps float_conv_frames to aud->frames; second realloc fails; function returns with float_conv grown, resample_out still old-size (or NULL), float_conv_frames claiming new size. Next call with aud->frames == float_conv_frames skips the entire block and passes the stale resample_out to convert_s16_to_float / convert_float_to_s16 downstream. Fix: realloc each into a local tmp (new_float_conv, new_resample_out, new_fixed_conv), commit each pointer only after its own alloc succeeds, and defer the *_frames counter bumps until after their corresponding allocation has succeeded. === ffmpeg_init_video: video->outbuf + conv_frame_buf unchecked === video->outbuf_size = 1 << 23; video->outbuf = (uint8_t*)av_malloc(video->outbuf_size); video->frame_drop_ratio = params->frame_drop_ratio; ... video->conv_frame_buf = (uint8_t*)av_malloc(size); memset(video->conv_frame_buf, 0, size); Both unchecked. conv_frame_buf is immediately NULL-deref'd by the memset, and av_image_fill_arrays a few lines down writes frame->data pointers based on it. outbuf is used later by the encoder's avcodec_encode_video calls. Fix: NULL-check both, return false. The caller (ffmpeg_new) already has 'if (!ffmpeg_init_video(handle)) goto error' at line 1073, and the error label calls ffmpeg_free which handles partial-init state via av_freep guards. === ffmpeg_init_muxer_pre: ctx + ctx->url unchecked === ctx = avformat_alloc_context(); handle->muxer.ctx = ctx; ... ctx->url = (char*)av_malloc(_len); strlcpy(ctx->url, handle->params.filename, _len); avformat_alloc_context can return NULL on OOM; the immediate 'ctx->url = ...' NULL-derefs. av_malloc for ctx->url is also unchecked; strlcpy NULL-derefs on OOM. Fix: two NULL-checks, return false. ffmpeg_free cleans up handle->muxer.ctx via avformat_free_context which NULL- tolerates and frees ctx->url as part of its teardown. === ffmpeg_flush_buffers: audio_buf + video_buf unchecked === void *video_buf = av_malloc(...); ... if (audio_buf_size) audio_buf = av_malloc(audio_buf_size); do { if (handle->config.audio_enable) if (FIFO_READ_AVAIL(...) >= audio_buf_size) fifo_read(handle->audio_fifo, audio_buf, audio_buf_size); /* NULL-deref */ if (FIFO_READ_AVAIL(handle->attr_fifo) >= sizeof(attr_buf)) fifo_read(..., video_buf, ...); /* NULL-deref */ } while (did_work); Both av_malloc sites unchecked. fifo_read does memcpy into the destination pointer (see libretro-common/queues/ fifo_queue.c) which NULL-derefs on a NULL destination. Fix: gate both fifo drain branches on 'buf != NULL'. Also gate the final ffmpeg_flush_audio call on audio_buf != NULL since its internal fifo_read path would NULL-deref the same way. The pattern of NULL-gating is already in place in the sibling ffmpeg_thread function (lines 1791, 1804 pre-patch) which got it right; this patch just brings ffmpeg_flush_buffers in line with that. === Not a bug, verified clean === Other av_malloc / av_realloc sites audited this pass: * record_ffmpeg.c:379, 388 (ffmpeg_init_audio audio->buffer / outbuf) - NULL-checked and symmetric with a goto-error-free teardown via ffmpeg_free. * record_ffmpeg.c:1036 (ffmpeg_new handle calloc) - NULL- checked. * record_ffmpeg.c:1744, 1748 (ffmpeg_thread video_buf / audio_buf) - unchecked allocs but already NULL-gated at every subsequent use site (lines 1791, 1804). Other subdirs swept this pass: * wifi/drivers/ (connmanctl.c, nmcli.c) - zero raw alloc sites, both drivers use shell-exec pattern. * record/drivers/record_wav.c - zero raw alloc sites. * location/drivers/android.c:40 - the single androidlocation_t calloc is NULL-checked with a 'dealloc' goto label that properly unwinds. === Thread-safety === planarize_audio and ffmpeg_audio_resample run on the record worker thread (ffmpeg_thread) or the main thread during finalize. ffmpeg_init_video and ffmpeg_init_muxer_pre run during driver init on the main thread before the worker thread is spawned. ffmpeg_flush_buffers runs during finalize after deinit_thread has joined the worker. The *_frames counters are single-writer from whichever thread is in the convert/resample path (worker during recording, main during finalize) so no lock changes are needed; the out-of-sync bug was purely about the single writer leaving its own state in an inconsistent window. === Reachability === Every bug here fires during active video/audio recording: * ffmpeg_init_video / ffmpeg_init_muxer_pre: once per record-session start. * planarize_audio: every audio frame when the codec wants planar input (most modern codecs). * ffmpeg_audio_resample: every audio frame when sample rate conversion is needed. * ffmpeg_flush_buffers: once per record-session stop/finalize. Memory pressure during recording is realistic - ffmpeg buffers can be large (the video outbuf is 8MB fixed at line 525) and the encoder itself holds multi-MB internal state.
1 parent 5d1272a commit 1871bd1

1 file changed

Lines changed: 84 additions & 19 deletions

File tree

record/drivers/record_ffmpeg.c

Lines changed: 84 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -524,12 +524,23 @@ static bool ffmpeg_init_video(ffmpeg_t *handle)
524524
* clues how big this buffer should be. */
525525
video->outbuf_size = 1 << 23;
526526
video->outbuf = (uint8_t*)av_malloc(video->outbuf_size);
527+
/* NULL-check outbuf: caller (ffmpeg_new) returns false via goto
528+
* error on our false return, and ffmpeg_free handles partial
529+
* init state via av_freep guards. */
530+
if (!video->outbuf)
531+
return false;
527532

528533
video->frame_drop_ratio = params->frame_drop_ratio;
529534

530535
size = av_image_get_buffer_size(video->pix_fmt, param->out_width,
531536
param->out_height, 1);
532537
video->conv_frame_buf = (uint8_t*)av_malloc(size);
538+
/* NULL-check conv_frame_buf: the memset on the next line
539+
* NULL-derefs on OOM, as does av_image_fill_arrays below
540+
* which writes into frame->data pointers based on
541+
* conv_frame_buf. */
542+
if (!video->conv_frame_buf)
543+
return false;
533544
/* Zero the buffer so padding pixels (from rounding odd dimensions
534545
* up to even) are black rather than uninitialized. */
535546
memset(video->conv_frame_buf, 0, size);
@@ -844,9 +855,19 @@ static bool ffmpeg_init_muxer_pre(ffmpeg_t *handle)
844855
#endif
845856
ctx = avformat_alloc_context();
846857
handle->muxer.ctx = ctx;
858+
/* NULL-check ctx: ffmpeg_free cleans up handle->muxer.ctx via
859+
* avformat_free_context which NULL-tolerates (see ffmpeg
860+
* docs). ctx->* writes below NULL-deref on OOM. */
861+
if (!ctx)
862+
return false;
847863
#if !FFMPEG3
848864
_len = MIN(strlen(handle->params.filename) + 1, PATH_MAX_LENGTH);
849865
ctx->url = (char*)av_malloc(_len);
866+
/* NULL-check ctx->url: strlcpy on the next line NULL-derefs
867+
* on OOM. ctx->url is freed by avformat_free_context as
868+
* part of the muxer teardown in ffmpeg_free. */
869+
if (!ctx->url)
870+
return false;
850871
strlcpy(ctx->url, handle->params.filename, _len);
851872
#else
852873
strlcpy(ctx->filename, handle->params.filename, sizeof(ctx->filename));
@@ -1349,11 +1370,21 @@ static void planarize_audio(ffmpeg_t *handle)
13491370

13501371
if (handle->audio.frames_in_buffer > handle->audio.planar_buf_frames)
13511372
{
1352-
handle->audio.planar_buf = av_realloc(handle->audio.planar_buf,
1373+
/* realloc-to-tmp: av_realloc follows C realloc contract and
1374+
* leaves the old buffer intact on failure. Pre-patch self-
1375+
* assigned planar_buf to the return, which on OOM would
1376+
* leak the old buffer AND leave planar_buf_frames (and
1377+
* subsequently the 'frames_in_buffer > planar_buf_frames'
1378+
* guard above) claiming the old size. On next call with
1379+
* the same or smaller frame count the guard would skip the
1380+
* realloc branch and pass a now-NULL planar_buf to
1381+
* planarize_float/s16 below, NULL-derefing. */
1382+
void *new_buf = av_realloc(handle->audio.planar_buf,
13531383
handle->audio.frames_in_buffer * handle->params.channels *
13541384
handle->audio.sample_size);
1355-
if (!handle->audio.planar_buf)
1385+
if (!new_buf)
13561386
return;
1387+
handle->audio.planar_buf = new_buf;
13571388

13581389
handle->audio.planar_buf_frames = handle->audio.frames_in_buffer;
13591390
}
@@ -1478,32 +1509,56 @@ static void ffmpeg_audio_resample(ffmpeg_t *handle,
14781509

14791510
if (aud->frames > handle->audio.float_conv_frames)
14801511
{
1481-
handle->audio.float_conv = (float*)av_realloc(handle->audio.float_conv,
1512+
/* Three stacked realloc-assign-self patterns pre-patch, all
1513+
* leaked on OOM and all left the associated *_frames
1514+
* counter out of sync with the actual allocation. Worst
1515+
* case: the second or third av_realloc fails after the
1516+
* first (or first two) succeeded, float_conv_frames was
1517+
* already bumped to the new size, and the next call would
1518+
* skip this whole 'if (aud->frames > ..._frames)' block
1519+
* and pass a NULL buffer to the downstream convert/
1520+
* resample calls.
1521+
*
1522+
* Fix: realloc into tmp locals, commit each pointer only
1523+
* after its own alloc succeeds, and defer the
1524+
* float_conv_frames / resample_out_frames /
1525+
* fixed_conv_frames counter bumps until the corresponding
1526+
* allocation has succeeded. */
1527+
float *new_float_conv;
1528+
float *new_resample_out;
1529+
int16_t *new_fixed_conv;
1530+
size_t new_resample_out_frames;
1531+
size_t new_fixed_conv_frames;
1532+
1533+
new_float_conv = (float*)av_realloc(handle->audio.float_conv,
14821534
aud->frames * handle->params.channels * sizeof(float));
1483-
if (!handle->audio.float_conv)
1535+
if (!new_float_conv)
14841536
return;
1537+
handle->audio.float_conv = new_float_conv;
1538+
handle->audio.float_conv_frames = aud->frames;
14851539

1486-
handle->audio.float_conv_frames = aud->frames;
14871540
/* To make sure we don't accidentally overflow. */
1488-
handle->audio.resample_out_frames = aud->frames
1489-
* handle->audio.ratio + 16;
1490-
handle->audio.resample_out = (float*)
1541+
new_resample_out_frames = aud->frames * handle->audio.ratio + 16;
1542+
new_resample_out = (float*)
14911543
av_realloc(handle->audio.resample_out,
1492-
handle->audio.resample_out_frames *
1544+
new_resample_out_frames *
14931545
handle->params.channels * sizeof(float));
1494-
if (!handle->audio.resample_out)
1546+
if (!new_resample_out)
14951547
return;
1548+
handle->audio.resample_out = new_resample_out;
1549+
handle->audio.resample_out_frames = new_resample_out_frames;
14961550

1497-
handle->audio.fixed_conv_frames = MAX(
1551+
new_fixed_conv_frames = MAX(
14981552
handle->audio.resample_out_frames,
14991553
handle->audio.float_conv_frames);
1500-
handle->audio.fixed_conv = (int16_t*)av_realloc(
1554+
new_fixed_conv = (int16_t*)av_realloc(
15011555
handle->audio.fixed_conv,
1502-
handle->audio.fixed_conv_frames *
1556+
new_fixed_conv_frames *
15031557
handle->params.channels * sizeof(int16_t));
1504-
1505-
if (!handle->audio.fixed_conv)
1558+
if (!new_fixed_conv)
15061559
return;
1560+
handle->audio.fixed_conv = new_fixed_conv;
1561+
handle->audio.fixed_conv_frames = new_fixed_conv_frames;
15071562
}
15081563

15091564
if (handle->audio.use_float || handle->audio.resampler)
@@ -1622,7 +1677,13 @@ static void ffmpeg_flush_buffers(ffmpeg_t *handle)
16221677

16231678
did_work = false;
16241679

1625-
if (handle->config.audio_enable)
1680+
/* Gate audio-fifo drain on audio_buf non-NULL: audio_buf
1681+
* is an OOM-prone av_malloc above, and fifo_read does
1682+
* memcpy-into-destination which NULL-derefs on a NULL
1683+
* audio_buf. On OOM we skip audio flush - the fifo
1684+
* retains its data and is freed unread by the caller's
1685+
* subsequent ffmpeg_free teardown. */
1686+
if (handle->config.audio_enable && audio_buf)
16261687
{
16271688
if (FIFO_READ_AVAIL(handle->audio_fifo) >= audio_buf_size)
16281689
{
@@ -1637,7 +1698,9 @@ static void ffmpeg_flush_buffers(ffmpeg_t *handle)
16371698
}
16381699
}
16391700

1640-
if (FIFO_READ_AVAIL(handle->attr_fifo) >= sizeof(attr_buf))
1701+
/* Gate video-fifo drain on video_buf non-NULL: same
1702+
* reasoning as the audio branch. */
1703+
if (video_buf && FIFO_READ_AVAIL(handle->attr_fifo) >= sizeof(attr_buf))
16411704
{
16421705
fifo_read(handle->attr_fifo, &attr_buf, sizeof(attr_buf));
16431706
fifo_read(handle->video_fifo, video_buf,
@@ -1649,8 +1712,10 @@ static void ffmpeg_flush_buffers(ffmpeg_t *handle)
16491712
}
16501713
}while (did_work);
16511714

1652-
/* Flush out last audio. */
1653-
if (handle->config.audio_enable)
1715+
/* Flush out last audio. Skip on OOM - audio_buf is the
1716+
* destination for ffmpeg_flush_audio's internal fifo_read
1717+
* (via ffmpeg_push_audio_thread) and NULL would NULL-deref. */
1718+
if (handle->config.audio_enable && audio_buf)
16541719
ffmpeg_flush_audio(handle, audio_buf, audio_buf_size);
16551720

16561721
/* Flush out last video. */

0 commit comments

Comments
 (0)