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

[Nicolas George]

Le decadi 20 vendémiaire, an CCXXIII, Thilo Borgmann a écrit :
> Hi,
> 
> trying to fix ticket #4018.
> 
> Metadata in mov is silently truncated to 1023 bytes.
> This patch allocated a buffer in case of entries found that exceed 1023 bytes.
> Fixes ticket #4018 for me.
> 
> Maybe check str_size against an upper limit?
> 
> -Thilo

> >From 365bec36b3b7f1925cfa2310d979a63ef8e3a7e8 Mon Sep 17 00:00:00 2001
> From: Thilo Borgmann <thilo.borgmann at mail.de>
> Date: Sat, 11 Oct 2014 16:09:07 +0200
> Subject: [PATCH] lavf/mov.c: Allocate buffer in case of long metadata entries.
> 
> ---
>  libavformat/mov.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 4ff46dd..136b1d5 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -358,24 +358,33 @@ 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 {

> +        char *pstr = str;

If you rename str (maybe str_small) and call pstr str instead, I believe
that makes the patch simpler.

> +        if (str_size > sizeof(str)-1) { // allocate buffer for long data field
> +            pstr = 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, pstr, str_size);
>          } else {
> -            int ret = avio_read(pb, str, str_size);
> +            int ret = avio_read(pb, pstr, str_size);
>              if (ret != str_size)
>                  return ret < 0 ? ret : AVERROR_INVALIDDATA;
> -            str[str_size] = 0;
> +            pstr[str_size] = 0;
>          }
>          c->fc->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
> -        av_dict_set(&c->fc->metadata, key, str, 0);
> +        av_dict_set(&c->fc->metadata, key, pstr, 0);
>          if (*language && strcmp(language, "und")) {
>              snprintf(key2, sizeof(key2), "%s-%s", key, language);
> -            av_dict_set(&c->fc->metadata, key2, str, 0);
> +            av_dict_set(&c->fc->metadata, key2, pstr, 0);
> +        }
> +

> +        if (str_size > sizeof(str)-1) { // free buffer for long data field
> +            av_freep(&pstr);

I think "if (pstr != str)" would be more robust.

Even more robust, if a bit more verbose:

    char *pstr = str, *alloc_str = NULL;
    if (...) {
	pstr = alloc_str = malloc(...);
    }
    ...
    av_freep(alloc_str); // unconditionnal

>          }
>      }
>      av_dlog(c->fc, "lang \"%3s\" ", language);

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141011/b2c87f49/attachment.asc>