[FFmpeg-devel] [PATCH] lavf/mov.c: Allocate buffer in case of long metadata entries.

[Michael Niedermayer]

On Fri, Oct 17, 2014 at 02:33:27PM +0200, Thilo Borgmann wrote:
> Am 16.10.14 04:47, schrieb Michael Niedermayer:
> > On Mon, Oct 13, 2014 at 09:40:42AM +0200, Thilo Borgmann wrote:
> >> Am 11.10.14 16:19, schrieb Nicolas George:
> >>> [...]
> >>
> >> all remarks applied.
> >>
> >> -Thilo
> >>
> > 
> >>  mov.c |   16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> >> cabb6e51de7f9329603561773f209b6a948478ce  0001-lavf-mov.c-Allocate-buffer-in-case-of-long-metadata-.patch
> >> From 5a14ef97ffc7d82dea5644c736e6dc2de2079e89 Mon Sep 17 00:00:00 2001
> >> From: Thilo Borgmann <thilo.borgmann at mail.de>
> >> Date: Mon, 13 Oct 2014 09:36:17 +0200
> >> Subject: [PATCH] lavf/mov.c: Allocate buffer in case of long metadata entries.
> >>
> >> ---
> >>  libavformat/mov.c | 16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index 4ff46dd..8d6d074 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -261,7 +261,9 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>  #ifdef MOV_EXPORT_ALL_METADATA
> >>      char tmp_key[5];
> >>  #endif
> >> -    char str[1024], key2[16], language[4] = {0};
> >> +    char str_small[1024], key2[16], language[4] = {0};
> >> +    char *str = str_small;
> >> +    char *pstr = NULL;
> >>      const char *key = NULL;
> >>      uint16_t langcode = 0;
> >>      uint32_t data_type = 0, str_size;
> >> @@ -358,13 +360,17 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>      if (atom.size < 0)
> >>          return AVERROR_INVALIDDATA;
> >>  
> >> -    str_size = FFMIN3(sizeof(str)-1, str_size, atom.size);
> >> -
> >>      if (parse)
> >>          parse(c, pb, str_size, key);
> >>      else {
> >> +        if (str_size > sizeof(str_small)-1) { // allocate buffer for long data field
> >> +            pstr = str = av_malloc(str_size);
> >> +            if (!pstr)
> >> +                return AVERROR(ENOMEM);
> >> +        }
> >> +
> >>          if (data_type == 3 || (data_type == 0 && (langcode < 0x400 || langcode == 0x7fff))) { // MAC Encoded
> >> -            mov_read_mac_string(c, pb, str_size, str, sizeof(str));
> >> +            mov_read_mac_string(c, pb, str_size, str, str_size);
> > 
> > this seems to store UTF8, which can require more space than str_size
> 
> New patch attached using a worst-case size of twice the input string size if
> the input is in utf8.
> 
> Tested only with non utf8 by now - I would appreciate it if someone could test
> this with UTF8 metadata or tell me how to generate/where to get a suitable file.
> 
> -Thilo
> 

>  mov.c |   27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> c6706cd53f0c804d993ba5dec8112faf1b9e84e5  0001-lavf-mov.c-Allocate-buffer-in-case-of-long-metadata-.patch
> From 1a59272e3d333c784e9f4857cd3aa6542ad28d6d Mon Sep 17 00:00:00 2001
> From: Thilo Borgmann <thilo.borgmann at mail.de>
> Date: Fri, 17 Oct 2014 14:30:30 +0200
> Subject: [PATCH] lavf/mov.c: Allocate buffer in case of long metadata entries.
> 
> ---
>  libavformat/mov.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 4ff46dd..a48877d 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -261,7 +261,9 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  #ifdef MOV_EXPORT_ALL_METADATA
>      char tmp_key[5];
>  #endif
> -    char str[1024], key2[16], language[4] = {0};
> +    char str_small[1024], key2[16], language[4] = {0};
> +    char *str = str_small;
> +    char *pstr = NULL;
>      const char *key = NULL;
>      uint16_t langcode = 0;
>      uint32_t data_type = 0, str_size;
> @@ -358,15 +360,28 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      if (atom.size < 0)
>          return AVERROR_INVALIDDATA;
>  
> -    str_size = FFMIN3(sizeof(str)-1, str_size, atom.size);
> -
>      if (parse)
>          parse(c, pb, str_size, key);
>      else {
> +#define LONG_META_ALLOC() {                   \
> +        if (str_size > sizeof(str_small)-1) { \
> +            pstr = str = av_malloc(str_size); \
> +            if (!pstr)                        \
> +                return AVERROR(ENOMEM);       \
> +        }                                     \
> +}
> +
>          if (data_type == 3 || (data_type == 0 && (langcode < 0x400 || langcode == 0x7fff))) { // MAC Encoded
> -            mov_read_mac_string(c, pb, str_size, str, sizeof(str));
> +            int str_size_in = str_size;
> +            str_size <<= 1; // worst-case requirement for output string in case of utf8 coded input
> +            // allocate buffer for long data field if necessary
> +            LONG_META_ALLOC();
> +            mov_read_mac_string(c, pb, str_size_in, str, str_size);
>          } else {
> -            int ret = avio_read(pb, str, str_size);
> +            int ret;
> +            // allocate buffer for long data field if necessary
> +            LONG_META_ALLOC();
> +            ret = avio_read(pb, str, str_size);

i think it would be simpler to always allocate with no str_small
local buffer case.
Also always allocating 2x the size should avoid the macro


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141017/1217941c/attachment.asc>