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

Justin Ruggles justinruggles
Wed Oct 3 02:45:40 CEST 2007


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




More information about the ffmpeg-devel mailing list