| Message ID | 20220826064456.92895-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avformat/imfdec: check track valid before use it | expand |
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. (If there is a first iteration of the loop -- is this actually guaranteed? A file without tracks seems to be pretty useless.) 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.) > 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)
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".
Pierre-Anthony Lemieux: > 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? > 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-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".
On Fri, Aug 26, 2022 at 9:01 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Pierre-Anthony Lemieux: > > 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? > > > > 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. > > > 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-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". > > _______________________________________________ > 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".
Pierre-Anthony Lemieux <pal@sandflow.com> 于2022年8月27日周六 00:06写道： > > On Fri, Aug 26, 2022 at 9:01 AM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: > > > > Pierre-Anthony Lemieux: > > > 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? > > > > > > > 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@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". > > > > _______________________________________________ > > 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".
On Sat, Aug 27, 2022 at 5:25 AM Steven Liu <lingjiujianke@gmail.com> wrote: > > Pierre-Anthony Lemieux <pal@sandflow.com> 于2022年8月27日周六 00:06写道： > > > > On Fri, Aug 26, 2022 at 9:01 AM Andreas Rheinhardt > > <andreas.rheinhardt@outlook.com> wrote: > > > > > > Pierre-Anthony Lemieux: > > > > 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? > > > > > > > > > > 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. The number of tracks is fixed and determined when the Composition Playlist (CPL) is parsed. > > > > > > > > > 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-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". > > > > > > _______________________________________________ > > > 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". > _______________________________________________ > 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".
See [1] for a patchset that is intended to address the coverity issue and simplify error handling. [1] https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220907200233.21255-1-pal@sandflow.com/ On Sat, Aug 27, 2022 at 10:25 AM Pierre-Anthony Lemieux <pal@sandflow.com> wrote: > > On Sat, Aug 27, 2022 at 5:25 AM Steven Liu <lingjiujianke@gmail.com> wrote: > > > > Pierre-Anthony Lemieux <pal@sandflow.com> 于2022年8月27日周六 00:06写道： > > > > > > On Fri, Aug 26, 2022 at 9:01 AM Andreas Rheinhardt > > > <andreas.rheinhardt@outlook.com> wrote: > > > > > > > > Pierre-Anthony Lemieux: > > > > > 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? > > > > > > > > > > > > > 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. > > The number of tracks is fixed and determined when the Composition > Playlist (CPL) is parsed. > > > > > > > > > > > > > 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-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". > > > > > > > > _______________________________________________ > > > > 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". > > _______________________________________________ > > 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".
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)); 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)
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(-)