[FFmpeg-devel] [PATCH] FLAC decoder segfault reading metadata

[Michael Niedermayer]

On Sun, Sep 30, 2007 at 11:11:12PM -0400, Justin Ruggles wrote:
> Well...I thought I had looked this over long enough, but I guess not. Here 
> is a better patch.
>
> -Justin
>

> Index: libavcodec/flac.c
> ===================================================================
> --- libavcodec/flac.c	(revision 10629)
> +++ libavcodec/flac.c	(working copy)
> @@ -176,7 +176,7 @@
>   */
>  static int metadata_parse(FLACContext *s)
>  {
> -    int i, metadata_last, metadata_type, metadata_size, streaminfo_updated=0;
> +    int metadata_last, metadata_type, metadata_size, streaminfo_updated=0;
>  
>      if (show_bits_long(&s->gb, 32) == MKBETAG('f','L','a','C')) {
>          skip_bits(&s->gb, 32);
> @@ -191,21 +191,45 @@
>                     " metadata block: flag = %d, type = %d, size = %d\n",
>                     metadata_last, metadata_type, metadata_size);
>              if (metadata_size) {
> +                int bits_left = s->gb.size_in_bits - get_bits_count(&s->gb);
> +                if(((bits_left + 7) >> 3) < metadata_size) {
> +                    skip_bits_long(&s->gb, bits_left);
> +                    break;
> +                }
>                  switch (metadata_type) {
>                  case METADATA_TYPE_STREAMINFO:
> +                    if(streaminfo_updated || metadata_size != FLAC_STREAMINFO_SIZE) {
> +                        metadata_last = 1;
> +                        break;
> +                    }
>                      metadata_streaminfo(s);
>                      streaminfo_updated = 1;
>                      break;
>  

>                  default:
> -                    for (i=0; i<metadata_size; i++)
> -                        skip_bits(&s->gb, 8);
> +                    skip_bits_long(&s->gb, 8*metadata_size);
>                  }

what does this change do in this patch?!
this is either a cosmetic change (or if metadata_size<0 its wrong)


> +            } else if(metadata_type == METADATA_TYPE_STREAMINFO) {
> +                metadata_last = 1;
>              }
>          } while (!metadata_last);
>  
> -        if (streaminfo_updated)
> +        if (streaminfo_updated) {
> +            /* check for invalid values in streaminfo */
> +            if(s->min_blocksize < 16 || s->max_blocksize < 16) {
> +                av_log(s->avctx, AV_LOG_DEBUG, "invalid block size. must be >= 16.\n");
> +                return 0;
> +            }
> +            if(!s->samplerate) {
> +                av_log(s->avctx, AV_LOG_DEBUG, "invalid sample rate. must be > 0.\n");
> +                return 0;
> +            }

if it must be >0 then check for it being <=0 !


> +            if(s->bps < 4) {
> +                av_log(s->avctx, AV_LOG_DEBUG, "invalid bits-per-sample. must be >= 4.\n");
> +                return 0;
> +            }
>              allocate_buffers(s);
> +        }

patch rejected, you cant just skip reallocating
the buffers, the return of this function is not checked nothing will
stop the randomly sized buffers from being used

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker, user
questions for the command line tools ffmpeg, ffplay, ... as well as questions
about how to use libav* should be sent to the ffmpeg-user mailinglist.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071002/d1ed8774/attachment.pgp>