[FFmpeg-devel] [PATCH 2/2] avcodec/zmbv: Check that the raw input is large enough to contain MVs or an intra frame

[Paul B Mahol]

On 9/17/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Mon, Sep 17, 2018 at 09:58:18AM +0200, Paul B Mahol wrote:
>> On 9/17/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
>> > On Sun, Sep 16, 2018 at 10:16:05AM +0200, Paul B Mahol wrote:
>> >> On 9/16/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
>> >> > Fixes: Timeout
>> >> > Fixes:
>> >> > 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704
>> >> >
>> >> > 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 | 11 ++++++++++-
>> >> >  1 file changed, 10 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
>> >> > index 9e27a2caad..1133bdf7ba 100644
>> >> > --- a/libavcodec/zmbv.c
>> >> > +++ b/libavcodec/zmbv.c
>> >> > @@ -409,6 +409,7 @@ static int decode_frame(AVCodecContext *avctx,
>> >> > void
>> >> > *data, int *got_frame, AVPac
>> >> >      int zret = Z_OK; // Zlib return code
>> >> >      int len = buf_size;
>> >> >      int hi_ver, lo_ver, ret;
>> >> > +    int min_size;
>> >> >
>> >> >      /* parse header */
>> >> >      if (len < 1)
>> >> > @@ -510,7 +511,11 @@ static int decode_frame(AVCodecContext *avctx,
>> >> > void
>> >> > *data, int *got_frame, AVPac
>> >> >          memset(c->prev, 0, avctx->width * avctx->height * (c->bpp /
>> >> > 8));
>> >> >          c->decode_intra= decode_intra;
>> >> >      }
>> >> > -
>> >> > +    if (c->flags & ZMBV_KEYFRAME) {
>> >> > +        min_size = avctx->width * avctx->height * (c->bpp / 8);
>> >>
>> >> This is pure guessing?
>> >
>> > theres a bit of logic behind it but ultimatly yes, its guessing
>> >
>> > The logic
>> > If the input is smaller for a raw keyframe the buffer would not be
>> > fully updated.
>> > If the buffer is not fully updated then this should be a inter frame
>> > not a keyframe.
>> >
>> > The 2nd part, is that all files i found
>> > (http://samples.mplayerhq.hu/V-codecs/ZMBV/)
>> > follow that assumtation
>> >
>> > The 3rd is that the source code for zmbv i could fine (dosbox)
>> > seems to never write uncompressed keyframes.
>> >
>> > The 4th is that the multimedia wiki doesnt seem to say anything about
>> > the
>> > uncompressed keyframe size
>> >
>> > so i thought its better to use this as the minimal size and avoid the
>> > issue the fuzzer found.
>> >
>> > Its quite possible that i have missed something.
>> > Also we could ask for a sample in the failure case or do something else
>> > if
>> > you
>> > have some other suggestion ?
>>
>
>> Yes, drop patch and leave code as is.
>
> I would prefer to fix the issue in some form. Do you have some suggestion
> or preferrance about how to fix this issue ?

No, you are not interested in fixing real issues.
Please do not commit hacks like this patch.

Nothing in zlib guarantees that size will fit under your random constraints.

>
>
>>
>> Also why this codec needs fuzzing?
>
> it needs fuzzing as much or as little as any other codec. Theres
> nothing special on it
>
> Also as i was looking over this earlier today, i could improve the check
> by also including compressed frames. And i found a kind of unrelated
> bug in the codec. Ill resend the related parts of the patchset
>
> Thanks
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The smallest minority on earth is the individual. Those who deny
> individual rights cannot claim to be defenders of minorities. - Ayn Rand
>