[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