[FFmpeg-devel] [PATCH] fix for bfi buffer overread (roundup issue 2497)

[Michael Niedermayer]

On Thu, Jan 06, 2011 at 08:44:44AM -0500, Daniel Kang wrote:
> When the input resolution is too large, the bfi decoder overreads the
> data buffer. The patch attached adds a sanity check to prevent this.
> ffmpeg will still fail in decoding this with an assertion error (the
> video data itself is invalid with the sample), but it does not buffer
> overread.
> 
> The roundup issue is 2497.

>  bfi.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> f75684d72bc0d5aa9b9e00887eb627ce31b2ac29  bfi_buffer_sanity_check.diff
> From 21d730f77e910810c1d3b2b69ce167d2da702be9 Mon Sep 17 00:00:00 2001
> From: Daniel Kang <daniel.d.kang at gmail.com>
> Date: Wed, 5 Jan 2011 23:46:33 -0500
> Subject: [PATCH] Sanity check on buffer reads
> 
> ---
>  libavcodec/bfi.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/libavcodec/bfi.c b/libavcodec/bfi.c
> index 91c8f6d..00631f0 100644
> --- a/libavcodec/bfi.c
> +++ b/libavcodec/bfi.c
> @@ -47,7 +47,7 @@ static av_cold int bfi_decode_init(AVCodecContext * avctx)
>  static int bfi_decode_frame(AVCodecContext * avctx, void *data,
>                              int *data_size, AVPacket *avpkt)
>  {
> -    const uint8_t *buf = avpkt->data;
> +    const uint8_t *buf = avpkt->data, *buf2 = avpkt->data;
>      int buf_size = avpkt->size;
>      BFIContext *bfi = avctx->priv_data;
>      uint8_t *dst = bfi->dst;
> @@ -99,6 +99,11 @@ static int bfi_decode_frame(AVCodecContext * avctx, void *data,
>          unsigned int code = byte >> 6;
>          unsigned int length = byte & ~0xC0;
> 
> +        if (buf-buf2 >= buf_size) {
> +            av_log(NULL, AV_LOG_ERROR, "Input resolution larger than actual frame.\n");
                      ^^^^
should be avctx, so the user knows from where the error message comes from.

also with buf_end= avpkt->data + buf_size
the check becomes buf >= buf_end which is a bit simpler and maybe slightly more
readable though thats nitpicking

also the following can still overread:
        switch (code) {

        case 0:                //Normal Chain
            bytestream_get_buffer(&buf, dst, length);
            dst += length;


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110106/03a1d64d/attachment.pgp>