[FFmpeg-devel] [PATCH] mpegpicture: use coded_width/coded_height to allocate frame

Michael Niedermayer michael at niedermayer.cc
Thu Nov 24 18:57:43 EET 2016


On Thu, Nov 24, 2016 at 05:45:38PM +0100, Michael Niedermayer wrote:
> On Thu, Nov 24, 2016 at 02:14:59AM +0100, Andreas Cadhalpun wrote:
> > On 23.11.2016 15:01, Michael Niedermayer wrote:
> > > On Mon, Nov 07, 2016 at 10:32:29PM +0100, Andreas Cadhalpun wrote:
> > >> This fixes a heap-buffer-overflow in ff_er_frame_end when decoding mss2 with
> > >> coded_width/coded_height larger than width/height.
> > >>
> > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> > >> ---
> > >>  libavcodec/mpegpicture.c | 12 ++++++------
> > >>  1 file changed, 6 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c
> > >> index 6748fc2..70b4d3c 100644
> > >> --- a/libavcodec/mpegpicture.c
> > >> +++ b/libavcodec/mpegpicture.c
> > >> @@ -108,15 +108,15 @@ static int alloc_frame_buffer(AVCodecContext *avctx,  Picture *pic,
> > >>          avctx->codec_id != AV_CODEC_ID_VC1IMAGE  &&
> > >>          avctx->codec_id != AV_CODEC_ID_MSS2) {
> > >>          if (edges_needed) {
> > >> -            pic->f->width  = avctx->width  + 2 * EDGE_WIDTH;
> > >> -            pic->f->height = avctx->height + 2 * EDGE_WIDTH;
> > >> +            pic->f->width  = avctx->coded_width  + 2 * EDGE_WIDTH;
> > >> +            pic->f->height = avctx->coded_height + 2 * EDGE_WIDTH;
> > >>          }
> > >>  
> > >>          r = ff_thread_get_buffer(avctx, &pic->tf,
> > >>                                   pic->reference ? AV_GET_BUFFER_FLAG_REF : 0);
> > >>      } else {
> > >> -        pic->f->width  = avctx->width;
> > >> -        pic->f->height = avctx->height;
> > >> +        pic->f->width  = avctx->coded_width;
> > >> +        pic->f->height = avctx->coded_height;
> > >>          pic->f->format = avctx->pix_fmt;
> > >>          r = avcodec_default_get_buffer2(avctx, pic->f, 0);
> > >>      }
> > >> @@ -135,8 +135,8 @@ static int alloc_frame_buffer(AVCodecContext *avctx,  Picture *pic,
> > >>                           (EDGE_WIDTH >> (i ? chroma_x_shift : 0));
> > >>              pic->f->data[i] += offset;
> > >>          }
> > >> -        pic->f->width  = avctx->width;
> > >> -        pic->f->height = avctx->height;
> > >> +        pic->f->width  = avctx->coded_width;
> > >> +        pic->f->height = avctx->coded_height;
> > >>      }
> > > 
> > > why would the error concealment code need a differently sized output
> > > than the decoder itself ?
> > 
> > This code is rather convoluted, so it's not quite obvious, however the
> > needed buffer size for the error concealment is determined by the
> > MpegEncContext.width/height, which is set via:
> > mss2_decode_init
> > |-> wmv9_init
> >     |-> ff_msmpeg4_decode_init
> >         |-> ff_h263_decode_init
> >             |-> ff_mpv_decode_init:
> >     s->width           = avctx->coded_width;
> >     s->height          = avctx->coded_height;
> > 
> > Thus the buffer needs the same size.
> 
> Is it correct that your cases uses
> decode_wmv9() -> vc1_decode_i_blocks() ?
> these decode a rectangele up to end_mb_y, end_mb_x
> does this mismatch with what later code accesses ?
> 

> would using end_mb_* in the EC code fix this ? (or disabling EC if
> they mismatch)

Note, for this sadly end_mb_* in MSS2 would need to be treated
differently than other codecs as it has different semantics
disabling EC on end_mb_ mismatch might be easier

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161124/e89d49a5/attachment.sig>


More information about the ffmpeg-devel mailing list