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

Sebastian Vater cdgs.basty
Wed May 26 17:24:03 CEST 2010

Ronald S. Bultje a ?crit :
> 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.

Just like all the other functions starting with decode ;-)
So they're all consistent now in this regard.
That was the reason I did it here, also.

Since dst_size can be never <= 0 in the first run (just like in
decodeplane8/32) because avctx->width is > 0 always now (we were adding
a check for that some time ago, right).

The old routine just had the for because it used FFMIN instead of proper
error handling.

Remaining the last check, length > remaining buffer, these have to be
done in dependency if we do a memcpy / memset or noop.
Like FFMIN and FFMIN3 were used in the old version depending on what we do.

BTW, counting backward with a do ... while here makes the error checking
simpler (note that the if before memcpy/memset part uses dst_size, i.e.
remaining destination  allowed to fill up). so it's easier to read:
if (dst_size > length)

instead of:
if (dst_size - x > length)

> 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; }

That sounds much better, indeed, yes.
Another possibility would be though to change the error messages in each
(i.e. make them more detailed), what do you think of this?


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

More information about the ffmpeg-devel mailing list