Pierre-Anthony Lemieux: > On Fri, Aug 26, 2022 at 1:37 AM Andreas Rheinhardt > <andreas.rheinha...@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 <l...@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? > 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@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
