[FFmpeg-devel] [libav-devel] [PATCH 5/6] Fixed segfaults on corruped smaker streams in the decoder.

[Laurent Aimar]

On Mon, Sep 12, 2011 at 11:59:56PM +0200, Michael Niedermayer wrote:
> On Mon, Sep 12, 2011 at 11:43:18PM +0200, Laurent Aimar wrote:
> > On Mon, Sep 12, 2011 at 11:28:44PM +0200, Reimar Döffinger wrote:
> > > On Sun, Sep 11, 2011 at 07:56:46PM +0200, Laurent Aimar wrote:
> > > > @@ -653,6 +659,8 @@ static int smka_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> > > >      } else { //8-bit data
> > > >          for(i = stereo; i >= 0; i--)
> > > >              pred[i] = get_bits(&gb, 8);
> > > > +        if (stereo + unp_size > data_size)
> > > > +            return -1;
> > > 
> > > This can overflow.
> > > if (unp_size < 0 || unp_size > data_size - stereo)
> > > should probably be safe.
> >  No it doesn't because of the surrounding code BUT I saw a bug
> > in this patch (data_size is a pointer, a '*' is missing)
> 
> fixed
> 
> 
> > 
> >  It can also be made a bit simpler. I will propose a better patch
> > later.
> 
> i guess you plan something like: ?
Nearly, this condition is a tad too strict when bits is 1.
I think it should be (unp_size & ~bits) + stereo > *data_size

> 
> --- a/libavcodec/smacker.c
> +++ b/libavcodec/smacker.c
> @@ -594,7 +594,7 @@ static int smka_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
>      }
>      stereo = get_bits1(&gb);
>      bits = get_bits1(&gb);
> -    if (unp_size & 0xC0000000 || unp_size > *data_size) {
> +    if (unp_size & 0xC0000000 || unp_size + stereo > *data_size) {
>          av_log(avctx, AV_LOG_ERROR, "Frame is too large to fit in buffer\n");
>          return -1;
>      }
> @@ -659,8 +659,6 @@ static int smka_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
>      } else { //8-bit data
>          for(i = stereo; i >= 0; i--)
>              pred[i] = get_bits(&gb, 8);
> -        if (stereo + unp_size > *data_size)
> -            return -1;
>          for(i = 0; i < stereo; i++)
>              *samples8++ = pred[i];
>          for(i = 0; i < unp_size; i++) {
> 
> i can just commit it already as ive already written it :)

No problem and sorry for the typo.

-- 
fenrir