| Message ID | 75b43580-088b-3d28-8552-f5b64386f83d@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 07.11.2016 22:52, Luca Barbato wrote: > On 07/11/2016 22:32, 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@googlemail.com> >> --- >> libavcodec/mpegpicture.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> > > Do you have a sample to look at the output in that specific case? Yes, and the output looks similar to most fuzzed samples: like garbage. Best regards, Andreas
On 07.11.2016 22:32, 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@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; > } > > if (avctx->hwaccel) { > Ping. It would be good to have this fixed in 3.2.1. Best regards, Andreas
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@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 ? [...]
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@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. Best regards, Andreas
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@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) [...]
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@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 [...]
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; } if (avctx->hwaccel) {
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@googlemail.com> --- libavcodec/mpegpicture.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)