[FFmpeg-devel] libavformat/ffmetadec: use dynamic allocation for line buffer

Submitted by François Revol on Nov. 7, 2018, 1:34 p.m.

Details

Message ID 20181107133443.4978-1-revol@free.fr
State New
Headers show

Commit Message

François Revol Nov. 7, 2018, 1:34 p.m.
When adding thumbnails to OGG files, the line can easily go up to 100kB.

We thus try to allocate the file size or SIZE_MAX to avoid truncation.
---
 libavformat/ffmetadec.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Michael Niedermayer Nov. 8, 2018, 7:50 p.m.
On Wed, Nov 07, 2018 at 02:34:43PM +0100, François Revol wrote:
> When adding thumbnails to OGG files, the line can easily go up to 100kB.
> 
> We thus try to allocate the file size or SIZE_MAX to avoid truncation.
> ---
>  libavformat/ffmetadec.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/ffmetadec.c b/libavformat/ffmetadec.c
> index 3290b3b7bc..ccbff51c03 100644
> --- a/libavformat/ffmetadec.c
> +++ b/libavformat/ffmetadec.c
> @@ -128,16 +128,26 @@ static int read_tag(const uint8_t *line, AVDictionary **m)
>  static int read_header(AVFormatContext *s)
>  {
>      AVDictionary **m = &s->metadata;
> -    uint8_t line[1024];
> +    int64_t line_size = avio_size(s->pb);
> +    uint8_t *line;
> +
> +    if (line_size < 1 || line_size > SIZE_MAX)
> +       line_size = SIZE_MAX;
> +
> +    line = av_malloc(line_size);
> +    if (!line)
> +        return AVERROR(ENOMEM);

this would use alot of memory for large files, also avio_size() will not
work with all inputs
using av_fast_realloc() or similar should avoid both issues


>  
>      while(!avio_feof(s->pb)) {
> -        get_line(s->pb, line, sizeof(line));
> +        get_line(s->pb, line, line_size);
>  

>          if (!memcmp(line, ID_STREAM, strlen(ID_STREAM))) {

out of memory access can happen here


thx

[...]
François Revol Nov. 8, 2018, 8:47 p.m.
Hi,

Le 08/11/2018 à 20:50, Michael Niedermayer a écrit :
>> +    line = av_malloc(line_size);
>> +    if (!line)
>> +        return AVERROR(ENOMEM);
> 
> this would use alot of memory for large files, also avio_size() will not
> work with all inputs

Yes I thought so as well, that was a quick fix for a friend in need.

> using av_fast_realloc() or similar should avoid both issues

You mean, having get_line() reallocate 1kB more each time it runs out
without finding a \n?

Btw, I noticed there was an avio_get_line() which was quite similar,
maybe those could be merged?


> 
>>  
>>      while(!avio_feof(s->pb)) {
>> -        get_line(s->pb, line, sizeof(line));
>> +        get_line(s->pb, line, line_size);
>>  
> 
>>          if (!memcmp(line, ID_STREAM, strlen(ID_STREAM))) {
> 
> out of memory access can happen here

Right. Preallocating at least 1kB would fix it though.


François.
Marton Balint Nov. 9, 2018, 9:06 a.m.
On Thu, 8 Nov 2018, François Revol wrote:

> Hi,
>
> Le 08/11/2018 à 20:50, Michael Niedermayer a écrit :
>>> +    line = av_malloc(line_size);
>>> +    if (!line)
>>> +        return AVERROR(ENOMEM);
>>
>> this would use alot of memory for large files, also avio_size() will not
>> work with all inputs
>
> Yes I thought so as well, that was a quick fix for a friend in need.
>
>> using av_fast_realloc() or similar should avoid both issues
>
> You mean, having get_line() reallocate 1kB more each time it runs out
> without finding a \n?

You should convert everything to use an AVBprint buffer.

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/libavformat/ffmetadec.c b/libavformat/ffmetadec.c
index 3290b3b7bc..ccbff51c03 100644
--- a/libavformat/ffmetadec.c
+++ b/libavformat/ffmetadec.c
@@ -128,16 +128,26 @@  static int read_tag(const uint8_t *line, AVDictionary **m)
 static int read_header(AVFormatContext *s)
 {
     AVDictionary **m = &s->metadata;
-    uint8_t line[1024];
+    int64_t line_size = avio_size(s->pb);
+    uint8_t *line;
+
+    if (line_size < 1 || line_size > SIZE_MAX)
+       line_size = SIZE_MAX;
+
+    line = av_malloc(line_size);
+    if (!line)
+        return AVERROR(ENOMEM);
 
     while(!avio_feof(s->pb)) {
-        get_line(s->pb, line, sizeof(line));
+        get_line(s->pb, line, line_size);
 
         if (!memcmp(line, ID_STREAM, strlen(ID_STREAM))) {
             AVStream *st = avformat_new_stream(s, NULL);
 
-            if (!st)
+            if (!st) {
+                av_free(line);
                 return AVERROR(ENOMEM);
+            }
 
             st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
             st->codecpar->codec_id   = AV_CODEC_ID_FFMETADATA;
@@ -146,8 +156,10 @@  static int read_header(AVFormatContext *s)
         } else if (!memcmp(line, ID_CHAPTER, strlen(ID_CHAPTER))) {
             AVChapter *ch = read_chapter(s);
 
-            if (!ch)
+            if (!ch) {
+                av_free(line);
                 return AVERROR(ENOMEM);
+            }
 
             m = &ch->metadata;
         } else
@@ -160,6 +172,7 @@  static int read_header(AVFormatContext *s)
                                    s->chapters[s->nb_chapters - 1]->time_base,
                                    AV_TIME_BASE_Q);
 
+    av_free(line);
     return 0;
 }