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

[Justin Ruggles]

Michael Niedermayer wrote:
> 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
>>
[...]
>>                  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)

Yep. It's sorta both. I like the latter option better, but it shouldn't 
be in a patch for which it doesn't matter.

>> +            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 !

You're right.  I need to either change the check or change the message.

> 
>> +            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

If this function returns an error (for some odd reason 0 is error...) 
then no decoding will be attempted.  It doesn't make sense to resize the 
buffers when the information you're basing the new sizes on is incorrect.

But regardless, the other patch I submitted fixes the same issue with 
much less complexity.  So consider this one withdrawn.

Some of the same issues do apply though, so thank you for reviewing it.

Thanks,
Justin