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

[Ronald S. Bultje]

Hi,

On Wed, May 26, 2010 at 4:29 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> 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?

I'm not terribly comfortable with it, but if others think it's OK then
I won't object.

Ronald