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

[Michael Niedermayer]

On Wed, Sep 11, 2019 at 11:18:47PM +0200, Tomas Härdin wrote:
> 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. 

There is something wrong with this argument.
Because one could say this about anything that is not static
no matter if true or false. And theres no  practical way to 
disproof it even if one waited, because maybe one just didnt wait
long enough ...


[...]
> 
> 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. 

Everyone probably has timelimits on jobs but these timeouts are still
a big problem. And i think this was discussed before ...

I think if you just think about what timeout to use for each case
A. a web browser loading image, video and audio files
B. a online video encoding service
C. ...

I think you will find that there is no timeout that works well
You just do not want 50 byte files or files with the resolution of an icon to
take days to decode


> Resources would be better spent gearing the fuzzing
> toward finding memory corruption issues, since the harm from them is
> far more serious.

Then fixing the timeouts would be a priority as they hold the fuzzer
up from finding other issues.
Time spend waiting for a timeout is time the fuzzer cannot search for
other issues

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190912/4ba8c17d/attachment.sig>