diff mbox series

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

Message ID DBAPR03MB6664A6CD42377B64E1DD794E8F909@DBAPR03MB6664.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel] avcodec/libx264: Use av_memdup() where appropriate
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Nov. 7, 2021, 11:33 a.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Will apply tonight unless there are objections.

 libavcodec/libx264.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

James Almer Nov. 7, 2021, 12:43 p.m. UTC | #1
On 11/7/2021 8:33 AM, Andreas Rheinhardt wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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);
>
diff mbox series

Patch

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;
             }
             memcpy(p, nal[i].p_payload, nal[i].i_payload);