| Message ID | 20200215135510.15635-1-onemda@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/utils: avoid unsigned integer overflows | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Sat, 15 Feb 2020, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavformat/utils.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 81ea239a66..ba2621aa28 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -4450,10 +4450,12 @@ void avformat_free_context(AVFormatContext *s) > if (s->oformat && s->oformat->priv_class && s->priv_data) > av_opt_free(s->priv_data); > > + if (s->nb_streams) > for (i = s->nb_streams - 1; i >= 0; i--) Maybe rewrite the loop instead? for (i = s->nb_streams; i-- > 0;) Regards, Marton > ff_free_stream(s, s->streams[i]); > > > + if (s->nb_programs) > for (i = s->nb_programs - 1; i >= 0; i--) { > av_dict_free(&s->programs[i]->metadata); > av_freep(&s->programs[i]->stream_index); > -- > 2.17.1 > > _______________________________________________ > 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".
Marton Balint (12020-02-15): > > + if (s->nb_streams) > > for (i = s->nb_streams - 1; i >= 0; i--) > Maybe rewrite the loop instead? > for (i = s->nb_streams; i-- > 0;) Or for (i = s->nb_streams - 1; i < s->nb_streams; i--) Regards,
On Sat, 15 Feb 2020, Nicolas George wrote: > Marton Balint (12020-02-15): >>> + if (s->nb_streams) >>> for (i = s->nb_streams - 1; i >= 0; i--) >> Maybe rewrite the loop instead? >> for (i = s->nb_streams; i-- > 0;) > > Or > > for (i = s->nb_streams - 1; i < s->nb_streams; i--) Or simply change the loops to normal ascending order? I don't see why these are descending loops in the first place. Regards, Marton
Marton Balint: > > > On Sat, 15 Feb 2020, Nicolas George wrote: > >> Marton Balint (12020-02-15): >>>> + if (s->nb_streams) >>>> for (i = s->nb_streams - 1; i >= 0; i--) >>> Maybe rewrite the loop instead? >>> for (i = s->nb_streams; i-- > 0;) >> >> Or >> >> for (i = s->nb_streams - 1; i < s->nb_streams; i--) > > Or simply change the loops to normal ascending order? I don't see why > these are descending loops in the first place. > Yeah. This sound like the best idea. One just has to call free_stream() instead of ff_free_stream() for this to work. - Andreas
On 2/15/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Marton Balint: >> >> >> On Sat, 15 Feb 2020, Nicolas George wrote: >> >>> Marton Balint (12020-02-15): >>>>> + if (s->nb_streams) >>>>> for (i = s->nb_streams - 1; i >= 0; i--) >>>> Maybe rewrite the loop instead? >>>> for (i = s->nb_streams; i-- > 0;) >>> >>> Or >>> >>> for (i = s->nb_streams - 1; i < s->nb_streams; i--) >> >> Or simply change the loops to normal ascending order? I don't see why >> these are descending loops in the first place. >> > Yeah. This sound like the best idea. One just has to call > free_stream() instead of ff_free_stream() for this to work. But perhaps there is some reason for current order? > - Andreas > _______________________________________________ > 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".
Paul B Mahol: > On 2/15/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >> Marton Balint: >>> >>> >>> On Sat, 15 Feb 2020, Nicolas George wrote: >>> >>>> Marton Balint (12020-02-15): >>>>>> + if (s->nb_streams) >>>>>> for (i = s->nb_streams - 1; i >= 0; i--) >>>>> Maybe rewrite the loop instead? >>>>> for (i = s->nb_streams; i-- > 0;) >>>> >>>> Or >>>> >>>> for (i = s->nb_streams - 1; i < s->nb_streams; i--) >>> >>> Or simply change the loops to normal ascending order? I don't see why >>> these are descending loops in the first place. >>> >> Yeah. This sound like the best idea. One just has to call >> free_stream() instead of ff_free_stream() for this to work. > > But perhaps there is some reason for current order? The reasons for this are historical: 85a57677 added ff_free_stream() by factoring it out of avformat_free_context(). ff_free_stream() was only intended to remove the last (highest index) stream of an AVFormatContext and so the loop needed to be changed to counting down. Meanwhile, Libav factored freeing a stream out, too, but differently (in aeda1121): It added free_stream() (that simply frees without updating the AVFormatContext (that is not among its parameters). c03ffe17 refactored ff_free_stream() into what it is now, but it kept using ff_free_stream() for freeing the stream and kept counting down accordingly. I couldn't find a reason why freeing the programs counts down. There probably is none; anyway, we don't need history to know that one can count upwards in both loops (when using free_stream() directly): We can just read the code. - Andreas
diff --git a/libavformat/utils.c b/libavformat/utils.c index 81ea239a66..ba2621aa28 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -4450,10 +4450,12 @@ void avformat_free_context(AVFormatContext *s) if (s->oformat && s->oformat->priv_class && s->priv_data) av_opt_free(s->priv_data); + if (s->nb_streams) for (i = s->nb_streams - 1; i >= 0; i--) ff_free_stream(s, s->streams[i]); + if (s->nb_programs) for (i = s->nb_programs - 1; i >= 0; i--) { av_dict_free(&s->programs[i]->metadata); av_freep(&s->programs[i]->stream_index);
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavformat/utils.c | 2 ++ 1 file changed, 2 insertions(+)