[FFmpeg-devel] [PATCH] IFF: Add error checking to byterun1 decoder

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Wed, May 26, 2010 at 3:20 PM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> -    for (x = 0; x < dst_size && buf < buf_end;) {
>> -        unsigned length;
>> -        const int8_t value = *buf++;
>> -        if (value >= 0) {
>> -            length = value + 1;
>> -            memcpy(dst + x, buf, FFMIN3(length, dst_size - x, buf_end - buf));
>> -            buf += length;
>> -        } else if (value > -128) {
>> -            length = -value + 1;
>> -            memset(dst + x, *buf++, FFMIN(length, dst_size - x));
>> -        } else { // noop
>> -            continue;
>> -        }
>> -        x += length;
>> +    if (buf < buf_end) {
>> +        do {
>> +            const int8_t value = *buf++;
>>     
>
> Your patch now mixes indentation and functional changes.
>   

Oooooops...will fix this when the next point was clarified.

>   
>> +            if (value >= 0) {
>> +                const int length = (unsigned) value + 1;
>> +                if (length > dst_size || length > (int) (buf_end - buf)) // overflow?
>> +                    break;
>> +                memcpy(dst, buf, length);
>> +                dst_size -= length;
>> +                dst += length;
>> +                buf += length;
>> +            } else if (value > -128) {
>> +                const int length = (unsigned) -value + 1;
>> +                if (length > dst_size || buf >= buf_end) // overflow?
>> +                    break;
>> +                memset(dst, *buf++, length);
>> +                dst_size -= length;
>> +                dst += length;
>> +            } else if (buf >= buf_end) { // noop, return error on overflow, though
>> +                break;
>> +            }
>> +        } while (dst_size);
>> +    }
>>     
>
> This can overread with a specially crafted buffer because you're not
> checking that buf < buf_end at the start of each loop iteration, which
> the original code did do.
>   

As already said, I do more approciate checks in these if's...at the end
it's ensured that it doesn't overread.
Or do you mean sth. else which I missed out? Could you give an example
for overreading then?

-- 

Best regards,
                   :-) Basty/CDGS (-: