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

[Sebastian Vater]

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)

right?
> 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 (-: