[FFmpeg-devel] [PATCH] buffer read sanity check (issue 2503)

[Michael Niedermayer]

On Fri, Jan 07, 2011 at 10:02:25AM -0500, Daniel Kang wrote:
> On Fri, Jan 7, 2011 at 9:46 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> >  On Thu, Jan 06, 2011 at 11:51:58PM -0500, Daniel Kang wrote:
> > > ffmpeg does not check for buffer overreading in the dpx decoder. For
> > > invalid headers, the target_packet_size or width could be too large. The
> > > patch attached adds a sanity check.
> >
> > >  dpx.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > e3c6cd5042d736935c2c5a730c93ee96f108b988  dpx_buffer_sanity_check.diff
> > > From ae6ec1ce796735a666afb6b911c8c9e594b1eb8a Mon Sep 17 00:00:00 2001
> > > From: Daniel Kang <daniel.d.kang at gmail.com>
> > > Date: Thu, 6 Jan 2011 23:34:05 -0500
> > > Subject: [PATCH] dpx buffer overread sanity check
> > >
> > > ---
> > >  libavcodec/dpx.c |    9 +++++++++
> > >  1 files changed, 9 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
> > > index f92b3d0..588ec6c 100644
> > > --- a/libavcodec/dpx.c
> > > +++ b/libavcodec/dpx.c
> > > @@ -55,6 +55,7 @@ static int decode_frame(AVCodecContext *avctx,
> > >                          AVPacket *avpkt)
> > >  {
> > >      const uint8_t *buf = avpkt->data;
> > > +    const uint8_t *buf_end = avpkt->data + avpkt->size;
> > >      int buf_size       = avpkt->size;
> > >      DPXContext *const s = avctx->priv_data;
> > >      AVFrame *picture  = data;
> > > @@ -174,6 +175,10 @@ static int decode_frame(AVCodecContext *avctx,
> > >          case 16:
> > >              if (source_packet_size == target_packet_size) {
> > >                  for (x = 0; x < avctx->height; x++) {
> > > +                    if (buf + target_packet_size*avctx->width > buf_end)
> > {
> > > +                        av_log(avctx, AV_LOG_ERROR, "Overread buffer.
> > Invalid header?\n");
> > > +                        return -1;
> > > +                    }
> > >                      memcpy(ptr, buf, target_packet_size*avctx->width);
> > >                      ptr += stride;
> > >                      buf += source_packet_size*avctx->width;
> > > @@ -182,6 +187,10 @@ static int decode_frame(AVCodecContext *avctx,
> > >                  for (x = 0; x < avctx->height; x++) {
> > >                      uint8_t *dst = ptr;
> > >                      for (y = 0; y < avctx->width; y++) {
> > > +                        if (buf + target_packet_size > buf_end) {
> > > +                            av_log(avctx, AV_LOG_ERROR, "Overread
> > buffer. Invalid header?\n");
> > > +                            return -1;
> > > +                        }
> >
> > something like
> >
> > if(source_packet_size * avctx->width > buf_end - buf)
> > at a single spot should be able to catch all i think
> 
> 
> It would be possible to move the check outside the loop, but I am not
> sure if target_packet_size*avctx->width*avctx->height would overflow or
> not. Should I check for this instead?

i think it cant overflow, and i think its source not target
source is whats added to buf and we check against buf

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

There will always be a question for which you do not know the correct awnser.
-------------- 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/20110107/f43d5e52/attachment.pgp>