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

[Michael Niedermayer]

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.cadhal...@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 
> > >>> 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 p...@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@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 
> 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 p...@paulmehta.com
> Reference https://crbug.com/643950
> 
> Signed-off-by: Matt Wolenetz 
> ---
>  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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Matthew Wolenetz]

Updated to SIZE_MAX. Thank you for your comments.


On Thu, Dec 15, 2016 at 5:23 PM, Andreas Cadhalpun <
andreas.cadhal...@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 
> >>> 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 p...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
From 9ce997c0cc31b3609031590b57e64587acc2aa87 Mon Sep 17 00:00:00 2001
From: Matt Wolenetz 
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 p...@paulmehta.com
Reference https://crbug.com/643950

Signed-off-by: Matt Wolenetz 
---
 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;
 title_str = av_malloc(title_size + 1); /* Add null terminator */
 if (!title_str)
 return AVERROR(ENOMEM);
-- 
2.11.0.483.g087da7b7c-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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 
>>> 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 p...@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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

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 
>> 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 p...@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.

> 
>> +return AVERROR_INVALIDDATA;
>>  title_str = av_malloc(title_size + 1); /* Add null terminator */
>>  if (!title_str)
>>  return AVERROR(ENOMEM);
> 
> Best regards,
> Andreas
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 15.12.2016 00:34, Matthew Wolenetz wrote:
> 
> From fd878457cd55690d4a27d74411b68a30c9fb2313 Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz 
> 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 p...@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.

> +return AVERROR_INVALIDDATA;
>  title_str = av_malloc(title_size + 1); /* Add null terminator */
>  if (!title_str)
>  return AVERROR(ENOMEM);

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel