[FFmpeg-devel] [PATCH] use av_mallocz() in vorbis_comment()

[Måns Rullgård]

Justin Ruggles <justin.ruggles at gmail.com> writes:

> Hi,
>
> This patch avoids allocating memory on the stack based on decoded stream
> values which can be up to 32-bit.  Mans has pointed out that the current
> version is not a security risk, it would just crash with SIGSEGV for
> really large metadata.  This patch skips the single metadata tag if
> allocation fails and continues try to the next tag.
>
> Thanks,
> Justin
>
>
> Index: libavformat/oggparsevorbis.c
> ===================================================================
> --- libavformat/oggparsevorbis.c	(revision 17145)
> +++ libavformat/oggparsevorbis.c	(working copy)
> @@ -71,15 +71,21 @@
>          v++;
>  
>          if (tl && vl) {
> -            char tt[tl + 1];
> -            char ct[vl + 1];
> +            char *tt, *ct;
>  
> +            tt = av_mallocz(tl + 1);
> +            ct = av_mallocz(vl + 1);

Why mallocz?  It's being written again immediately below.

> +            if (!tt || !ct) {
> +                av_freep(&tt);
> +                av_freep(&ct);
> +                av_log(as, AV_LOG_WARNING, "out-of-memory error. skipping VorbisComment tag.\n");
> +                continue;
> +            }
> +
>              for (j = 0; j < tl; j++)
>                  tt[j] = toupper(t[j]);
> -            tt[tl] = 0;
>  
>              memcpy(ct, v, vl);
> -            ct[vl] = 0;
>  
>              // took from Vorbis_I_spec
>              if (!strcmp(tt, "AUTHOR") || !strcmp(tt, "ARTIST"))
> @@ -96,6 +102,9 @@
>                  as->track = atoi(ct);
>              else if (!strcmp(tt, "ALBUM"))
>                  av_strlcpy(as->album, ct, sizeof(as->album));
> +
> +            av_freep(&tt);
> +            av_freep(&ct);
>          }
>      }

Otherwise OK.

-- 
M?ns Rullg?rd
mans at mansr.com