Re: [FFmpeg-devel] [PATCH v2] avformat/imfdec: check track valid before use it

[Pierre-Anthony Lemieux <pal@sandflow.com>]

On Fri, Aug 26, 2022 at 1:37 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Steven Liu:
> > fix CID: 1512414
> > And return AVERROR_INVALIDDATA when get_next_track_with_minimum_timestamp
> > incorrect in imf_read_packet;
> >
> > Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> > ---
> >  libavformat/imfdec.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
> > index 5bbe7a53f8..08f342bc1a 100644
> > --- a/libavformat/imfdec.c
> > +++ b/libavformat/imfdec.c
> > @@ -697,8 +697,9 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma
> >          }
> >      }
> >
> > -    av_log(s, AV_LOG_DEBUG, "Found next track to read: %d (timestamp: %lf / %lf)\n",
> > -           track->index, av_q2d(track->current_timestamp), av_q2d(minimum_timestamp));
> > +    if (track)
> > +        av_log(s, AV_LOG_DEBUG, "Found next track to read: %d (timestamp: %lf / %lf)\n",
> > +               track->index, av_q2d(track->current_timestamp), av_q2d(minimum_timestamp));
>
> Coverity actually complained about track being uninitialized, which this
> patch does not address. And the reason it does this is that it doesn't
> understand the algorithm: track will always be initialized in the first
> iteration of the loop.

Is it possible to tell coverity that  c->track_count > 0 is a
pre-condition, or should we modify the loop/algorithm?

> (If there is a first iteration of the loop -- is
> this actually guaranteed? A file without tracks seems to be pretty useless.)

imfdec currently assumes that (a) imf_read_packet() is not called if
there are no streams/tracks and (b) a track will always be found.

(b) will be true for a conformant IMF Composition, but I am not sure
it can always be true for a malformed one.

I think imf_read_packet() can probably be hardened. Perhaps do this as
a patch separately from addressing the coverity issue?

> FYI: In Coverity's analysis there are loop iterations, but it just
> assumed that track is not initialized in the loop (which boils down to
> saying that it presumed the tracks' current_timestamp to be invalid
> (denominator 0). I hope this can't happen.
> (There is btw another issue: The initialization of minimum_timestamp
> presumes that int are 32bit which need not be true.)

INT32_MAX -> INT_MAX should fix this right?

>
> >      return track;
> >  }
> >
> > @@ -760,6 +761,8 @@ static int imf_read_packet(AVFormatContext *s, AVPacket *pkt)
> >      AVRational next_timestamp;
> >
> >      track = get_next_track_with_minimum_timestamp(s);
> > +    if (!track)
> > +        return AVERROR_INVALIDDATA;
> >
> >      ret = get_resource_context_for_timestamp(s, track, &resource);
> >      if (ret)
>
> _______________________________________________
> 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".
_______________________________________________
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".