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

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Mon, May 24, 2010 at 9:28 AM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> 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 ;-)
>>     
>
> So make it NOconst (because it makes no difference, I've told you this
> before) unsigned value = *buf++; if (buf < 128) ..., and then you
> don't need the cast.
>   

This makes the code larger and slower actually (I tried this out when
you told me that for the first time), because there are three checks to
be done:
0 <= value < 128
128 <= value < 255
value == 255

Making it unsigned I have to do sth. like:
if (value < 128) {
} else if (value < 255) {
} else {
}

This is slower than just:
if (value >= 0) { // No extra cmp instruction required, it just uses the
flag setting of the movzbl (%esi),al to test if it's >= 0
} else if (value != -128) {
} else {
}

But what all this has to do with const? The const is there to indicate
that value isn't changed after first read.

-- 

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