[FFmpeg-devel] [PATCH 2/2] avcodec/g2meet: Check for end of input in jpg_decode_block()

[Tomas Härdin]

tis 2019-09-10 klockan 16:16 +0200 skrev Michael Niedermayer:
> On Mon, Sep 09, 2019 at 11:04:32PM +0200, Tomas Härdin wrote:
> > mån 2019-09-09 klockan 22:16 +0200 skrev Michael Niedermayer:
> > > Fixes: Timeout (100sec -> 0.7sec)
> > > Fixes: 8668/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_G2M_fuzzer-5174143888130048
> > > 
> > > 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/g2meet.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavcodec/g2meet.c b/libavcodec/g2meet.c
> > > index 19e1c130ce..731d29a5d4 100644
> > > --- a/libavcodec/g2meet.c
> > > +++ b/libavcodec/g2meet.c
> > > @@ -244,6 +244,9 @@ static int jpg_decode_block(JPGContext *c, GetBitContext *gb,
> > >      const int is_chroma = !!plane;
> > >      const uint8_t *qmat = is_chroma ? chroma_quant : luma_quant;
> > >  
> > > +    if (get_bits_left(gb) < 1)
> > > +        return AVERROR_INVALIDDATA;
> > > +
> > >      c->bdsp.clear_block(block);
> > >      dc = get_vlc2(gb, c->dc_vlc[is_chroma].table, 9, 3);
> > 
> > Why doesn't the VLC stuff have EOF handling? 
> 
> Because it doesnt need it in most of the cases and the get_vlc code
> is quite speed critical in some codecs.
> Also it would add a error return to cases that previously never
> could receive an error. That would require callers to be changed
> and check for this error in some cases
> 
> 
> > There's bound to be a
> > metric bajillion of cases like this strewn across the codebase..
> 
> if that was the case, the fuzzer would have likely found more cases.

That's only a matter of time. For some formats it may even be
impossible to know whether there are parser issues lurking in there,
even if you employ formal verification.

I've said multiple times that worrying about these timeout things is
mostly a waste of time since any serious user will know to put time
limits on jobs. Resources would be better spent gearing the fuzzing
toward finding memory corruption issues, since the harm from them is
far more serious.

/Tomas