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

[Ronald S. Bultje]

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.

Ronald