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

[Ronald S. Bultje]

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.

> +            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.

Ronald