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

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Mon, May 24, 2010 at 8:58 AM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> Michael Niedermayer a ?crit :
>>     
>>>>  iff.c |   58 ++++++++++++++++++++++++++++++++++++++++------------------
>>>>  1 file changed, 40 insertions(+), 18 deletions(-)
>>>> 8519ee6c6a67610cb02eb831b3261e324bcee6ed  iff-byterun1-error.patch
>>>> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
>>>> index bcf4929..5e4c55a 100644
>>>> --- a/libavcodec/iff.c
>>>> +++ b/libavcodec/iff.c
>>>> @@ -226,27 +226,43 @@ static void decodeplane32(uint32_t *dst, const uint8_t *buf, int buf_size, int p
>>>>   * @param dst_size the destination plane size in bytes
>>>>   * @param buf the source byterun1 compressed bitstream
>>>>   * @param buf_end the EOF of source byterun1 compressed bitstream
>>>> - * @return number of consumed bytes in byterun1 compressed bitstream
>>>> + * @return consumed bytes in compressed bitstream or negative error code otherwise
>>>>  */
>>>> -static int decode_byterun(uint8_t *dst, int dst_size,
>>>> -                          const uint8_t *buf, const uint8_t *const buf_end) {
>>>> +static av_always_inline int decode_byterun(AVCodecContext *const avctx,
>>>> +                                           uint8_t *dst, int dst_size,
>>>> +                                           const uint8_t *buf,
>>>> +                                           const uint8_t *const buf_end) {
>>>>      const uint8_t *const buf_start = buf;
>>>> -    unsigned x;
>>>> -    for (x = 0; x < dst_size && buf < buf_end;) {
>>>> -        unsigned length;
>>>> +    if (buf >= buf_end) {
>>>> +        av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream buffer overflow\n");
>>>> +        return AVERROR_INVALIDDATA;
>>>> +    }
>>>> +    do {
>>>>          const int8_t value = *buf++;
>>>>          if (value >= 0) {
>>>> -            length = value + 1;
>>>> -            memcpy(dst + x, buf, FFMIN3(length, dst_size - x, buf_end - buf));
>>>> +            const int length = (unsigned) value + 1;
>>>>
>>>>         
>>> whats that cast good for?
>>>
>>>       
>> Preventing it to create a sign-extend instruction (at least on m68k)
>> which isn't necessary because value is always positive.
>>     
>
> What if the bytestream is specifically crafted? Sounds bad.
>   
> If it's always "positive", then the "const int8_t value" above should
> be a const uint8_t value (or really an unsigned value, that's much
> faster). If it's <128, then you should check for that and error out
> otherwise.
>   

No it's not always positive, this part of code is just in a if (value >=
0) block, so it should always be positive in that part of code ;-)

-- 

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