diff mbox series

[FFmpeg-devel,1/2] avformat/utils: avoid unsigned integer overflows

Message ID 20200215135510.15635-1-onemda@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avformat/utils: avoid unsigned integer overflows
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Paul B Mahol Feb. 15, 2020, 1:55 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/utils.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Marton Balint Feb. 15, 2020, 4:29 p.m. UTC | #1
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".
Nicolas George Feb. 15, 2020, 4:33 p.m. UTC | #2
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,
Marton Balint Feb. 15, 2020, 4:35 p.m. UTC | #3
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
Andreas Rheinhardt Feb. 15, 2020, 6:06 p.m. UTC | #4
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
Paul B Mahol Feb. 15, 2020, 7:56 p.m. UTC | #5
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".
Andreas Rheinhardt Feb. 15, 2020, 9:14 p.m. UTC | #6
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 mbox series

Patch

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);