[FFmpeg-devel] [PATCH] avcodec/libx264: Use av_memdup() where appropriate

[James Almer]

On 11/7/2021 8:33 AM, Andreas Rheinhardt wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> Will apply tonight unless there are objections.
> 
>   libavcodec/libx264.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 0766b4a950..29546ebf06 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -985,10 +985,9 @@ static av_cold int X264_init(AVCodecContext *avctx)
>               if (nal[i].i_type == NAL_SEI) {
>                   av_log(avctx, AV_LOG_INFO, "%s\n", nal[i].p_payload+25);
>                   x4->sei_size = nal[i].i_payload;
> -                x4->sei      = av_malloc(x4->sei_size);
> +                x4->sei      = av_memdup(nal[i].p_payload, x4->sei_size);
>                   if (!x4->sei)
>                       return AVERROR(ENOMEM);
> -                memcpy(x4->sei, nal[i].p_payload, nal[i].i_payload);
>                   continue;

This loop will leak x4->sei if libx264 returns a x264_nal_t array with 
more than one SEI NALU.
I assume x264_encoder_headers() is expected to only return one such NALU 
with the unregistered user data SEI containing the encode settings 
string, and i can see looking at the source code that currently it does 
as much, but it's not explicit in the documentation at all (It does not 
even mention that it returns a SEI NALU, only SPS/PPS), so in theory it 
could also at any point in the future start including global metadata 
like mastering display or content light and not be considered an API break.
So maybe the av_malloc should be replaced with av_realloc and the memcpy 
kept around with an offset to merge two or more SEI NALUs if present, to 
be safe.

Also, that av_log() line printing the payload is making the same risky 
assumption (And the offset is wrong, it should be 24).

>               }
>               memcpy(p, nal[i].p_payload, nal[i].i_payload);
>