[FFmpeg-devel] [PATCH] mss2: Fix buffer overflow.

Reimar Döffinger Reimar.Doeffinger at gmx.de
Fri Feb 26 13:47:40 CET 2016


On 26.02.2016, at 11:26, wm4 <nfxjfg at googlemail.com> wrote:

> On Thu, 25 Feb 2016 22:39:51 +0100
> Reimar Döffinger <Reimar.Doeffinger at 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.Doeffinger at 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.Doeffinger at 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.

While that's the for possibly all codecs, I meant I couldn't see clear documentation that expanding via height > coded_height would be impossible if some codec had use for it.
Either way I'll push it soonish if nobody has comments on the patch itself...


More information about the ffmpeg-devel mailing list