Re: [FFmpeg-devel] [PATCH 1/2] avformat/vividas: Check packet size

[Anton Khirnov <anton@khirnov.net>]

Quoting Michael Niedermayer (2022-09-29 00:35:09)
> On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2022-09-22 20:08:51)
> > > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int'
> > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/vividas.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> > > index e9954f73ed0..e8efe49a5c0 100644
> > > --- a/libavformat/vividas.c
> > > +++ b/libavformat/vividas.c
> > > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s,
> > >  
> > >      if (viv->sb_entries[viv->current_sb_entry].flag == 0) {
> > >          uint64_t v_size = ffio_read_varlen(pb);
> > > +        int last, last_start;
> > >  
> > >          if (!viv->num_audio)
> > >              return AVERROR_INVALIDDATA;
> > > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s,
> > >              start = ffio_read_varlen(pb);
> > >              pcm_bytes = ffio_read_varlen(pb);
> > >  
> > > -            if (i > 0 && start == 0)
> > > -                break;
> > > +            if (i > 0) {
> > > +                if (start == 0)
> > > +                    break;
> > > +                if (start < last || start - (unsigned)last > INT_MAX)
> > 
> > What is the second condition for?
> 
> start is signed int so are "copyies" of it
> "start < last" would allow a negative last with a large start
> the 2nd check handles that.
> the difference of consequtive values are stored as int later
> 
> The patch tried to leave the storage types and check for it.
> The types could be changed or some other checks could be used
> I was undecided on this patch a bit too. I picked this mainly
> to keep changes more minimal but maybe this was not the best
> choice

Checking that start >= 0 would fix this as well, wouldn't it? I don't
think it makes sense for it to be negative.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".