On Thu, 25 Feb 2016 22:39:51 +0100 Reimar Döffinger <reimar.doeffin...@gmx.de> wrote:
> On Thu, Feb 25, 2016 at 09:25:08PM +0100, wm4 wrote: > > On Thu, 25 Feb 2016 21:06:46 +0100 > > Reimar Döffinger <reimar.doeffin...@gmx.de> wrote: > > > > > Reported as https://trac.mplayerhq.hu/ticket/2264 but have > > > not been able to reproduce with FFmpeg-only. > > > I have no idea what coded_height is used for here exactly, > > > so this might not be the best fix. > > > Fixes the following chain of events: > > > ff_mss12_decode_init sets coded_height while not setting height. > > > ff_mpv_decode_init then copies coded_height into MpegEncContext height. > > > This is then used by init_context_frame to allocate the data structures. > > > However the wmv9rects are validated/initialized based on avctx->height, > > > not > > > avctx->coded_height. > > > Thus the decode_wmv9 function will try to decode a larger video that we > > > allocated data structures for, causing out-of-bounds writes. > > > > > > Signed-off-by: Reimar Döffinger <reimar.doeffin...@gmx.de> > > > --- > > > libavcodec/mss12.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavcodec/mss12.c b/libavcodec/mss12.c > > > index 6b58aa29..d42093b 100644 > > > --- a/libavcodec/mss12.c > > > +++ b/libavcodec/mss12.c > > > @@ -581,8 +581,8 @@ av_cold int ff_mss12_decode_init(MSS12Context *c, int > > > version, > > > return AVERROR_INVALIDDATA; > > > } > > > > > > - avctx->coded_width = AV_RB32(avctx->extradata + 20); > > > - avctx->coded_height = AV_RB32(avctx->extradata + 24); > > > + avctx->coded_width = FFMAX(AV_RB32(avctx->extradata + 20), > > > avctx->width); > > > + avctx->coded_height = FFMAX(AV_RB32(avctx->extradata + 24), > > > avctx->height); > > > if (avctx->coded_width > 4096 || avctx->coded_height > 4096) { > > > av_log(avctx, AV_LOG_ERROR, "Frame dimensions %dx%d too large", > > > avctx->coded_width, avctx->coded_height); > > > > Just a side remark... > > > > I'm wondering why ff_set_dimensions doesn't have coded_width/height > > arguments and checks them. I bet this situation happens often. > > I am not sure there is a fixed requirement for the relation > between coded_height and height that necessarily applies > to all codecs. Well, coded_width is supposed to be the actual frame size, while width applies cropping. So width should always be <= coded_width. For codecs where this should not hold up for some reason, coded_width is obviously inappropriate to use in the first place. > For example I don't know if for mss1 that case I am filtering > out is actually invalid! In which case the FFMAX might > need to go into mss2.c instead... > Note that this mostly happens because the mss2 codec essentially > instantiates multiple (partial) decoders into a single avctx, > and they might have differing requirements. That sounds scary. > Though I admit I am not sure the mss12 code can handle this > case correctly either. > I guess in that way checking it might be good since it means > you at least don't have to guess if the check was just forgotten > or left out intentionally. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel