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

[Stefano Sabatini]

On date Saturday 2010-06-12 20:38:46 +0200, Sebastian Vater encoded:
> Ronald S. Bultje a ?crit :
> > Hi,
> >
> > On Wed, May 26, 2010 at 4:34 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> >   
> >> On Wed, May 26, 2010 at 4:29 PM, Sebastian Vater
> >> <cdgs.basty at googlemail.com> wrote:
> >>     
> >>> Ronald S. Bultje a ?crit :
> >>>       
> >>>> 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.
> >>     
> >
> > I haven't heard anyone else, so I'd prefer if the check was kept in
> > place. Rest of patch looks OK on a rough first check.

I'd prefer to keep the check as it was in the original code too, and
if possible avoid cosmetics+functional wherever possible.

As for what regards the sign-extend instruction related cast, I have
no clue about it but seems like it should go in a separate commit.

Regards.
-- 
FFmpeg = Fascinating and Friendly Mega Pitiless Eccentric Ghost