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

[Ronald S. Bultje]

Hi,

On Wed, May 26, 2010 at 9:21 AM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> Wow! Never thought that adding normal error checking can actually make
> code run faster instead of slower.
>
> BTW, with the last patch I also forgot to add doxygen new avctx
> parameter, which I fixed with this patch, too.
[..]
> -static int decode_byterun(uint8_t *dst, int dst_size,
> +static 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 {

This is confusing. You're movng from a for() to a do{}while so the
check is not done in the beginning, but as a result you add an extra
check. I'd keep the loop as it was before.

Then in the loop, you can add more if (buf>=end) break; checks, and at
the end of the function you can add the actual error message which is
triplicated throughout the loop now (if (buf >= end) { av_log();
return error; }

> +            const int length = (unsigned) -value + 1;
> +            if (length > dst_size || buf >= buf_end) { // overflow?
> +                av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream buffer overflow\n");
> +                return AVERROR_INVALIDDATA;
> +            }
[..]
> +        } else if (buf >= buf_end) { // noop, return error on overflow, though
> +            av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream buffer overflow\n");
> +            return AVERROR_INVALIDDATA;

Ronald