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

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Wed, May 26, 2010 at 4:18 PM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> Ronald S. Bultje a ?crit :
>>     
>>> On Wed, May 26, 2010 at 3:20 PM, Sebastian Vater
>>> <cdgs.basty at googlemail.com> wrote:
>>>       
>>>> +            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?
>>     
>
> Imagine that buf_size=0, so buf==buf_end directly. The topmost if will
> catch that. Now imagine that buf is exactly 1 byte + length long.
> After one loop iteration, length won't be checked (because that's only
> done on entry into the loop, and not before every iteration) and const
> int8_t value = *buf++ will overread beyond the buffer bounds.
>   

Ahh, but this isn't actually a problem, because AVPacket->data has
FF_INPUT_BUFFER_PADDING_SIZE.
So it will still read in valid memory area...and regardless of what the
contents of that byte is, the if's will
catch that, since they all check buf >= buf_end. ;-)

I have adressed that issue from the very beginning by taking a look at
the allocation routine for AVPacket->data. ;-)

But maybe I could add a comment about this?

-- 

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