diff mbox

[FFmpeg-devel] lavf/mov: Do not blindly allocate stts entries

Message ID CAB0OVGoe_WTaSUpaw513zLUX49ZyYZ+dNFGhh3qpUjN7BqM_Sg@mail.gmail.com
State Accepted
Headers show

Commit Message

Carl Eugen Hoyos Dec. 31, 2017, 9:35 p.m. UTC 
2017-12-31 22:26 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 2017-12-31 22:17 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>> On Sat, Dec 30, 2017 at 02:36:39PM +0100, Carl Eugen Hoyos wrote:
>>> 2017-12-29 23:37 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>>> > 2017-11-28 21:32 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>>> >> On Mon, Nov 27, 2017 at 05:24:14AM +0100, Carl Eugen Hoyos wrote:
>>> >
>>> >>>      for (i = 0; i < entries && !pb->eof_reached; i++) {
>>> >>> -        int sample_duration;
>>> >>> +        int sample_duration, ret;
>>> >>>          unsigned int sample_count;
>>> >>> +        if (i > sc->stts_count) {
>>> >>> +            ret = av_reallocp_array(&sc->stts_data,
>>> >>> +                                    FFMIN(sc->stts_count * 2LL, entries),
>>> >>> +                                    sizeof(*sc->stts_data));
>>> >>
>>> >> this should use a variant of av_fast_realloc
>>> >
>>> > Do you prefer the new patch?
>>> > The old variant here looks slightly saner to me.
>>>
>>> Attached is what you possibly had in mind.
>>>
>>> Please review, Carl Eugen
>>
>>>  mov.c |   13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>> cc7986179fe0ddc394457e8543d9ae907b49373c  0001-lavf-mov-Use-av_fast_realloc-in-mov_read_stts.patch
>>> From f5fcd9ed1e5ce604c358a3787f1977277005ebb5 Mon Sep 17 00:00:00 2001
>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>> Date: Sat, 30 Dec 2017 14:34:41 +0100
>>> Subject: [PATCH] lavf/mov: Use av_fast_realloc() in mov_read_stts().
>>>
>>> Avoids large allocations for short files with invalid stts entry.
>>> Fixes bugzilla 1102.
>>> ---
>>>  libavformat/mov.c |   13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 2064473..1e97652 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -2850,13 +2850,22 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>          av_log(c->fc, AV_LOG_WARNING, "Duplicated STTS atom\n");
>>>      av_free(sc->stts_data);
>>>      sc->stts_count = 0;
>>> -    sc->stts_data = av_malloc_array(entries, sizeof(*sc->stts_data));
>>> -    if (!sc->stts_data)
>>> +    if (entries >= INT_MAX / sizeof(*sc->stts_data))
>>>          return AVERROR(ENOMEM);
>>
>> this leaves a stale pointer on error in sc->stts_data
>
> New patch attached.

Which would not have worked as intended, new variant attached.

Carl Eugen

Comments

Michael Niedermayer Jan. 1, 2018, 3:17 p.m. UTC | #1 
On Sun, Dec 31, 2017 at 10:35:28PM +0100, Carl Eugen Hoyos wrote:
> 2017-12-31 22:26 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> > 2017-12-31 22:17 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> >> On Sat, Dec 30, 2017 at 02:36:39PM +0100, Carl Eugen Hoyos wrote:
> >>> 2017-12-29 23:37 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> >>> > 2017-11-28 21:32 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> >>> >> On Mon, Nov 27, 2017 at 05:24:14AM +0100, Carl Eugen Hoyos wrote:
> >>> >
> >>> >>>      for (i = 0; i < entries && !pb->eof_reached; i++) {
> >>> >>> -        int sample_duration;
> >>> >>> +        int sample_duration, ret;
> >>> >>>          unsigned int sample_count;
> >>> >>> +        if (i > sc->stts_count) {
> >>> >>> +            ret = av_reallocp_array(&sc->stts_data,
> >>> >>> +                                    FFMIN(sc->stts_count * 2LL, entries),
> >>> >>> +                                    sizeof(*sc->stts_data));
> >>> >>
> >>> >> this should use a variant of av_fast_realloc
> >>> >
> >>> > Do you prefer the new patch?
> >>> > The old variant here looks slightly saner to me.
> >>>
> >>> Attached is what you possibly had in mind.
> >>>
> >>> Please review, Carl Eugen
> >>
> >>>  mov.c |   13 +++++++++++--
> >>>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>> cc7986179fe0ddc394457e8543d9ae907b49373c  0001-lavf-mov-Use-av_fast_realloc-in-mov_read_stts.patch
> >>> From f5fcd9ed1e5ce604c358a3787f1977277005ebb5 Mon Sep 17 00:00:00 2001
> >>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> >>> Date: Sat, 30 Dec 2017 14:34:41 +0100
> >>> Subject: [PATCH] lavf/mov: Use av_fast_realloc() in mov_read_stts().
> >>>
> >>> Avoids large allocations for short files with invalid stts entry.
> >>> Fixes bugzilla 1102.
> >>> ---
> >>>  libavformat/mov.c |   13 +++++++++++--
> >>>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >>> index 2064473..1e97652 100644
> >>> --- a/libavformat/mov.c
> >>> +++ b/libavformat/mov.c
> >>> @@ -2850,13 +2850,22 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>          av_log(c->fc, AV_LOG_WARNING, "Duplicated STTS atom\n");
> >>>      av_free(sc->stts_data);
> >>>      sc->stts_count = 0;
> >>> -    sc->stts_data = av_malloc_array(entries, sizeof(*sc->stts_data));
> >>> -    if (!sc->stts_data)
> >>> +    if (entries >= INT_MAX / sizeof(*sc->stts_data))
> >>>          return AVERROR(ENOMEM);
> >>
> >> this leaves a stale pointer on error in sc->stts_data
> >
> > New patch attached.
> 
> Which would not have worked as intended, new variant attached.
> 
> Carl Eugen

>  mov.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> c6ced82c5a98fbdf831ff4cd4809e4a02d6823c0  0001-lavf-mov-Use-av_fast_realloc-in-mov_read_stts.patch
> From 9eaf0b56245820194e8e1bee0e3730f3c7362158 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> Date: Sun, 31 Dec 2017 22:30:57 +0100
> Subject: [PATCH] lavf/mov: Use av_fast_realloc() in mov_read_stts().
> 
> Avoids large allocations for short files with invalid stts entry.
> Fixes bugzilla 1102.
> ---
>  libavformat/mov.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

should be ok

thx

[...]
Carl Eugen Hoyos Jan. 1, 2018, 9:31 p.m. UTC | #2 
2018-01-01 16:17 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> On Sun, Dec 31, 2017 at 10:35:28PM +0100, Carl Eugen Hoyos wrote:
>> 2017-12-31 22:26 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>> > 2017-12-31 22:17 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>> >> On Sat, Dec 30, 2017 at 02:36:39PM +0100, Carl Eugen Hoyos wrote:
>> >>> 2017-12-29 23:37 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>> >>> > 2017-11-28 21:32 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>> >>> >> On Mon, Nov 27, 2017 at 05:24:14AM +0100, Carl Eugen Hoyos wrote:
>> >>> >
>> >>> >>>      for (i = 0; i < entries && !pb->eof_reached; i++) {
>> >>> >>> -        int sample_duration;
>> >>> >>> +        int sample_duration, ret;
>> >>> >>>          unsigned int sample_count;
>> >>> >>> +        if (i > sc->stts_count) {
>> >>> >>> +            ret = av_reallocp_array(&sc->stts_data,
>> >>> >>> +                                    FFMIN(sc->stts_count * 2LL, entries),
>> >>> >>> +                                    sizeof(*sc->stts_data));
>> >>> >>
>> >>> >> this should use a variant of av_fast_realloc
>> >>> >
>> >>> > Do you prefer the new patch?
>> >>> > The old variant here looks slightly saner to me.
>> >>>
>> >>> Attached is what you possibly had in mind.
>> >>>
>> >>> Please review, Carl Eugen
>> >>
>> >>>  mov.c |   13 +++++++++++--
>> >>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> >>> cc7986179fe0ddc394457e8543d9ae907b49373c  0001-lavf-mov-Use-av_fast_realloc-in-mov_read_stts.patch
>> >>> From f5fcd9ed1e5ce604c358a3787f1977277005ebb5 Mon Sep 17 00:00:00 2001
>> >>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> >>> Date: Sat, 30 Dec 2017 14:34:41 +0100
>> >>> Subject: [PATCH] lavf/mov: Use av_fast_realloc() in mov_read_stts().
>> >>>
>> >>> Avoids large allocations for short files with invalid stts entry.
>> >>> Fixes bugzilla 1102.
>> >>> ---
>> >>>  libavformat/mov.c |   13 +++++++++++--
>> >>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> >>> index 2064473..1e97652 100644
>> >>> --- a/libavformat/mov.c
>> >>> +++ b/libavformat/mov.c
>> >>> @@ -2850,13 +2850,22 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> >>>          av_log(c->fc, AV_LOG_WARNING, "Duplicated STTS atom\n");
>> >>>      av_free(sc->stts_data);
>> >>>      sc->stts_count = 0;
>> >>> -    sc->stts_data = av_malloc_array(entries, sizeof(*sc->stts_data));
>> >>> -    if (!sc->stts_data)
>> >>> +    if (entries >= INT_MAX / sizeof(*sc->stts_data))
>> >>>          return AVERROR(ENOMEM);
>> >>
>> >> this leaves a stale pointer on error in sc->stts_data
>> >
>> > New patch attached.
>>
>> Which would not have worked as intended, new variant attached.
>>
>> Carl Eugen
>
>>  mov.c |   17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>> c6ced82c5a98fbdf831ff4cd4809e4a02d6823c0  0001-lavf-mov-Use-av_fast_realloc-in-mov_read_stts.patch
>> From 9eaf0b56245820194e8e1bee0e3730f3c7362158 Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> Date: Sun, 31 Dec 2017 22:30:57 +0100
>> Subject: [PATCH] lavf/mov: Use av_fast_realloc() in mov_read_stts().
>>
>> Avoids large allocations for short files with invalid stts entry.
>> Fixes bugzilla 1102.
>> ---
>>  libavformat/mov.c |   17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> should be ok

Patch applied.

Thank you, Carl Eugen
diff mbox

Patch

From 9eaf0b56245820194e8e1bee0e3730f3c7362158 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Sun, 31 Dec 2017 22:30:57 +0100
Subject: [PATCH] lavf/mov: Use av_fast_realloc() in mov_read_stts().

Avoids large allocations for short files with invalid stts entry.
Fixes bugzilla 1102.
---
 libavformat/mov.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2064473..22faecf 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2830,7 +2830,7 @@  static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     AVStream *st;
     MOVStreamContext *sc;
-    unsigned int i, entries;
+    unsigned int i, entries, alloc_size = 0;
     int64_t duration=0;
     int64_t total_sample_count=0;
 
@@ -2848,15 +2848,24 @@  static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     if (sc->stts_data)
         av_log(c->fc, AV_LOG_WARNING, "Duplicated STTS atom\n");
-    av_free(sc->stts_data);
+    av_freep(&sc->stts_data);
     sc->stts_count = 0;
-    sc->stts_data = av_malloc_array(entries, sizeof(*sc->stts_data));
-    if (!sc->stts_data)
+    if (entries >= INT_MAX / sizeof(*sc->stts_data))
         return AVERROR(ENOMEM);
 
     for (i = 0; i < entries && !pb->eof_reached; i++) {
         int sample_duration;
         unsigned int sample_count;
+        unsigned min_entries = FFMIN(FFMAX(i, 1024 * 1024), entries);
+        MOVStts *stts_data = av_fast_realloc(sc->stts_data, &alloc_size,
+                                             min_entries * sizeof(*sc->stts_data));
+        if (!stts_data) {
+            av_freep(&sc->stts_data);
+            sc->stts_count = 0;
+            return AVERROR(ENOMEM);
+        }
+        sc->stts_count = min_entries;
+        sc->stts_data = stts_data;
 
         sample_count=avio_rb32(pb);
         sample_duration = avio_rb32(pb);
-- 
1.7.10.4