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

[Ronald S. Bultje]

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.

(Will review the whole patch later.)

Ronald