[FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 14:35, Michael Niedermayer wrote:
> On Wed, Aug 16, 2017 at 09:48:43AM +0200, Tomas Härdin wrote:
>> On 2017-08-16 05:04, Michael Niedermayer wrote:
>>> Fixes: OOM
>>> Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>   libavcodec/zmbv.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
>>> index f126515bd1..861098a0f2 100644
>>> --- a/libavcodec/zmbv.c
>>> +++ b/libavcodec/zmbv.c
>>> @@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>       // Needed if zlib unused or init aborted before inflateInit
>>>       memset(&c->zstream, 0, sizeof(z_stream));
>>> -    c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);
>>> +    if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) {
>> Are width and height constrained somewhere? If both end up around
>> 1<<32 then the multiplication can overflow.
> width and height are signed integers and checked in avcodec_open2()
>
> I can replicate the check here if you think depending on a distant
> check is too fragile.

Nah, so long as it's checked. Too bad C doesn't have range typed 
integers like Ada

>>> +        av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than max_pixels\n");
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +    c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);
>> Use 255ULL and 64ULL maybe? It's almost conceivable that you could
>> have a file constructed to be (1<<31 - 255)x1 that passes the
>> max_pixels check
> such file could exist but avcodec_open2() will reject this currently.
> I think adding an explicit check of some kind would make sense
> 255ULL and 64ULL wont help as  decomp_size is just a unsigned int and
> its passed into zlib which uses  a uInt ...
>
> ill send a new patch
>
> thx

Right, it might pass max_pixels but then overflow uInt..

/Tomas