[FFmpeg-devel] [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr

Michael Niedermayer michael at niedermayer.cc
Wed Feb 8 04:18:18 EET 2017


On Tue, Feb 07, 2017 at 03:46:02PM -0800, Matthew Wolenetz wrote:
> Updated to SIZE_MAX. Thank you for your comments.
> 
> 
> On Thu, Dec 15, 2016 at 5:23 PM, Andreas Cadhalpun <
> andreas.cadhalpun at googlemail.com> wrote:
> 
> > On 15.12.2016 03:25, James Almer wrote:
> > > On 12/14/2016 10:39 PM, Andreas Cadhalpun wrote:
> > >> On 15.12.2016 00:34, Matthew Wolenetz wrote:
> > >>>
> > >>> From fd878457cd55690d4a27d74411b68a30c9fb2313 Mon Sep 17 00:00:00 2001
> > >>> From: Matt Wolenetz <wolenetz at chromium.org>
> > >>> Date: Fri, 2 Dec 2016 18:10:39 -0800
> > >>> Subject: [PATCH] lavf/mov.c: Avoid heap allocation wrap in
> > mov_read_hdlr
> > >>>
> > >>> Core of patch is from paul at paulmehta.com
> > >>> Reference https://crbug.com/643950
> > >>> ---
> > >>>  libavformat/mov.c | 2 ++
> > >>>  1 file changed, 2 insertions(+)
> > >>>
> > >>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> > >>> index 2a69890..7254505 100644
> > >>> --- a/libavformat/mov.c
> > >>> +++ b/libavformat/mov.c
> > >>> @@ -739,6 +739,8 @@ static int mov_read_hdlr(MOVContext *c,
> > AVIOContext *pb, MOVAtom atom)
> > >>>
> > >>>      title_size = atom.size - 24;
> > >>>      if (title_size > 0) {
> > >>> +        if (title_size >= UINT_MAX)
> > >>
> > >> I think this should use SIZE_MAX.
> > >
> > > title_size is int64_t and SIZE_MAX is UINT64_MAX on x86_64.
> >
> > Yes, but the argument of av_malloc is size_t.
> >
> > >>
> > >>> +            return AVERROR_INVALIDDATA;
> > >>>          title_str = av_malloc(title_size + 1); /* Add null terminator
> > */
> >
> > So this should cast the argument to size_t to fix the issue on x86_64:
> >           title_str = av_malloc((size_t)title_size + 1); /* Add null
> > terminator */
> >
> > Best regards,
> > Andreas
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >

>  mov.c |    2 ++
>  1 file changed, 2 insertions(+)
> 7141b8dfebcafab0db3b8f3f332068916718b093  643950-lavf-mov.c-Avoid-heap-allocation-wrap-in-mov_read_hd.patch
> From 9ce997c0cc31b3609031590b57e64587acc2aa87 Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz <wolenetz at google.com>
> Date: Wed, 14 Dec 2016 15:24:42 -0800
> Subject: [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr
> 
> Core of patch is from paul at paulmehta.com
> Reference https://crbug.com/643950
> 
> Signed-off-by: Matt Wolenetz <wolenetz at chromium.org>
> ---
>  libavformat/mov.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 6fd43a0a4e..4b86e0fd36 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -742,6 +742,8 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  
>      title_size = atom.size - 24;
>      if (title_size > 0) {
> +        if (title_size >= SIZE_MAX)
> +            return AVERROR_INVALIDDATA;

I know this was suggested here but the code below clearly doesnt
support title_size > INT_MAX, so the check should be for that
Also from the point of view of sanity, this is a string identifing
the handler if iam not mistaken,
if that is megabytes or more there is something strange going on,
so bailing out on 2gb should not be a problem

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170208/f97a2bab/attachment.sig>


More information about the ffmpeg-devel mailing list