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

[Steven Liu]

Pierre-Anthony Lemieux <pal at sandflow.com> 于2022年8月27日周六 00:06写道：
>
> On Fri, Aug 26, 2022 at 9:01 AM Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com> wrote:
> >
> > Pierre-Anthony Lemieux:
> > > On Fri, Aug 26, 2022 at 1:37 AM Andreas Rheinhardt
> > > <andreas.rheinhardt at 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 at 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?
> > >
> >
> > The typical way to do this is to add an av_assert1 or av_assert2;
> > but this must only be done if it is indeed ensured that the assert will
> > not be triggered.
> >
> > >> (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.
> > >
> >
> > Can't we make it true by adding the relevant checks to read_header?
>
> Yes.
Can imf add or remove track when processing? Looks like the live
streaming change resolution or bitrate when playing.
>
> >
> > > 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?
> > >
> >
> > Yes.
> >
> > >>
> > >>>      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 at ffmpeg.org
> > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >>
> > >> To unsubscribe, visit link above, or email
> > >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".