[FFmpeg-devel,2/2,v2] avformat/matroskadec: use av_fast_realloc to reallocate ebml list arrays

Submitted by James Almer on Sept. 3, 2019, 11:03 p.m.

Details

Message ID 20190903230300.927-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Sept. 3, 2019, 11:03 p.m.
Speeds up the process considerably.

Fixes ticket #8109.

Suggested-by: nevcairiel
Suggested-by: cehoyos
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroskadec.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Andreas Rheinhardt Sept. 3, 2019, 11:36 p.m.
James Almer:
> Speeds up the process considerably.
> 
> Fixes ticket #8109.
> 
> Suggested-by: nevcairiel
> Suggested-by: cehoyos
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/matroskadec.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 439ee462a5..0f227eb33d 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax {
>  
>  typedef struct EbmlList {
>      int nb_elem;
> +    unsigned int alloc_elem_size;
>      void *elem;
>  } EbmlList;
>  
> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
>          data = (char *) data + syntax->data_offset;
>          if (syntax->list_elem_size) {
>              EbmlList *list = data;
> -            void *newelem = av_realloc_array(list->elem, list->nb_elem + 1,
> -                                                   syntax->list_elem_size);
> +            void *newelem;
> +
> +            if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size)
> +                return AVERROR(ENOMEM);
> +
> +            newelem = av_fast_realloc(list->elem,
> +                                      &list->alloc_elem_size,
> +                                      (list->nb_elem + 1) * syntax->list_elem_size);
If this fails, list->alloc_elem_size will be zero, yet list->elem and
list->nb_elem might be non NULL/zero. I actually think that this will
probably work, but it is nevertheless ugly.

- Andreas
James Almer Sept. 4, 2019, 1:53 a.m.
On 9/3/2019 8:36 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Speeds up the process considerably.
>>
>> Fixes ticket #8109.
>>
>> Suggested-by: nevcairiel
>> Suggested-by: cehoyos
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/matroskadec.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 439ee462a5..0f227eb33d 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax {
>>  
>>  typedef struct EbmlList {
>>      int nb_elem;
>> +    unsigned int alloc_elem_size;
>>      void *elem;
>>  } EbmlList;
>>  
>> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
>>          data = (char *) data + syntax->data_offset;
>>          if (syntax->list_elem_size) {
>>              EbmlList *list = data;
>> -            void *newelem = av_realloc_array(list->elem, list->nb_elem + 1,
>> -                                                   syntax->list_elem_size);
>> +            void *newelem;
>> +
>> +            if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size)
>> +                return AVERROR(ENOMEM);
>> +
>> +            newelem = av_fast_realloc(list->elem,
>> +                                      &list->alloc_elem_size,
>> +                                      (list->nb_elem + 1) * syntax->list_elem_size);
> If this fails, list->alloc_elem_size will be zero, yet list->elem and
> list->nb_elem might be non NULL/zero. I actually think that this will
> probably work, but it is nevertheless ugly.

It should work.

I'll push this tomorrow if no one objects, then backport to supported
branches.
Carl Eugen Hoyos Sept. 4, 2019, 4:21 a.m.
Von meinem iPhone gesendet

> Am 04.09.2019 um 01:03 schrieb James Almer <jamrial@gmail.com>:
> 
> Speeds up the process considerably.
> 
> Fixes ticket #8109.
> 
> Suggested-by: nevcairiel
> Suggested-by: cehoyos
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> libavformat/matroskadec.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 439ee462a5..0f227eb33d 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax {
> 
> typedef struct EbmlList {
>     int nb_elem;
> +    unsigned int alloc_elem_size;
>     void *elem;
> } EbmlList;
> 
> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
>         data = (char *) data + syntax->data_offset;
>         if (syntax->list_elem_size) {
>             EbmlList *list = data;
> -            void *newelem = av_realloc_array(list->elem, list->nb_elem + 1,
> -                                                   syntax->list_elem_size);
> +            void *newelem;

> +            if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size)
> +                return AVERROR(ENOMEM);

I would have naively expected this to use INT_MAX.

And please reconsider backporting random fixes for issues that are not regressions.

Carl Eugen
James Almer Sept. 4, 2019, 4:37 a.m.
On 9/4/2019 1:21 AM, Carl Eugen Hoyos wrote:
> 
> 
> Von meinem iPhone gesendet
> 
>> Am 04.09.2019 um 01:03 schrieb James Almer <jamrial@gmail.com>:
>>
>> Speeds up the process considerably.
>>
>> Fixes ticket #8109.
>>
>> Suggested-by: nevcairiel
>> Suggested-by: cehoyos
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> libavformat/matroskadec.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 439ee462a5..0f227eb33d 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax {
>>
>> typedef struct EbmlList {
>>     int nb_elem;
>> +    unsigned int alloc_elem_size;
>>     void *elem;
>> } EbmlList;
>>
>> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
>>         data = (char *) data + syntax->data_offset;
>>         if (syntax->list_elem_size) {
>>             EbmlList *list = data;
>> -            void *newelem = av_realloc_array(list->elem, list->nb_elem + 1,
>> -                                                   syntax->list_elem_size);
>> +            void *newelem;
> 
>> +            if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size)
>> +                return AVERROR(ENOMEM);
> 
> I would have naively expected this to use INT_MAX.

The size parameter in av_fast_realloc() is unsigned int.
av_add_index_entry() does the exact same check.

> 
> And please reconsider backporting random fixes for issues that are not regressions.

Despite not being a regression, the speed up from this fix on real world
files is considerable, so it's worth backporting to at least release/4.2.

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
James Almer Sept. 4, 2019, 1:33 p.m.
On 9/4/2019 1:37 AM, James Almer wrote:
> On 9/4/2019 1:21 AM, Carl Eugen Hoyos wrote:
>>
>>
>> Von meinem iPhone gesendet
>>
>>> Am 04.09.2019 um 01:03 schrieb James Almer <jamrial@gmail.com>:
>>>
>>> Speeds up the process considerably.
>>>
>>> Fixes ticket #8109.
>>>
>>> Suggested-by: nevcairiel
>>> Suggested-by: cehoyos
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> libavformat/matroskadec.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index 439ee462a5..0f227eb33d 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax {
>>>
>>> typedef struct EbmlList {
>>>     int nb_elem;
>>> +    unsigned int alloc_elem_size;
>>>     void *elem;
>>> } EbmlList;
>>>
>>> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
>>>         data = (char *) data + syntax->data_offset;
>>>         if (syntax->list_elem_size) {
>>>             EbmlList *list = data;
>>> -            void *newelem = av_realloc_array(list->elem, list->nb_elem + 1,
>>> -                                                   syntax->list_elem_size);
>>> +            void *newelem;
>>
>>> +            if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size)
>>> +                return AVERROR(ENOMEM);
>>
>> I would have naively expected this to use INT_MAX.
> 
> The size parameter in av_fast_realloc() is unsigned int.
> av_add_index_entry() does the exact same check.
> 
>>
>> And please reconsider backporting random fixes for issues that are not regressions.
> 
> Despite not being a regression, the speed up from this fix on real world
> files is considerable, so it's worth backporting to at least release/4.2.

Pushed.

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 439ee462a5..0f227eb33d 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -110,6 +110,7 @@  typedef const struct EbmlSyntax {
 
 typedef struct EbmlList {
     int nb_elem;
+    unsigned int alloc_elem_size;
     void *elem;
 } EbmlList;
 
@@ -1236,8 +1237,14 @@  static int ebml_parse(MatroskaDemuxContext *matroska,
         data = (char *) data + syntax->data_offset;
         if (syntax->list_elem_size) {
             EbmlList *list = data;
-            void *newelem = av_realloc_array(list->elem, list->nb_elem + 1,
-                                                   syntax->list_elem_size);
+            void *newelem;
+
+            if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size)
+                return AVERROR(ENOMEM);
+
+            newelem = av_fast_realloc(list->elem,
+                                      &list->alloc_elem_size,
+                                      (list->nb_elem + 1) * syntax->list_elem_size);
             if (!newelem)
                 return AVERROR(ENOMEM);
             list->elem = newelem;
@@ -1490,6 +1497,7 @@  static void ebml_free(EbmlSyntax *syntax, void *data)
                     ebml_free(syntax[i].def.n, ptr);
                 av_freep(&list->elem);
                 list->nb_elem = 0;
+                list->alloc_elem_size = 0;
             } else
                 ebml_free(syntax[i].def.n, data_off);
         default: