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

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

[Steven Liu]

fix CID: 1512414

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/imfdec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
index 5bbe7a53f8..a97dcc3b8b 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;
 }
 
-- 
2.25.0

_______________________________________________
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] [PATCH 2/2] avformat/dashdec: check val before xmlFree it

[Steven Liu]

fix CID: 1512404
When there have no val value should not xmlFree it.

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/dashdec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index 63bf7e96a5..bf9adf2183 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -1185,8 +1185,10 @@ static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
             }
         }
         node = xmlNextElementSibling(node);
-        xmlFree(val);
-        val = NULL;
+        if (val) {
+            xmlFree(val);
+            val = NULL;
+        }
     }
     return 0;
 }
-- 
2.25.0

_______________________________________________
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".

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

[Pierre-Anthony Lemieux]

Since we are protecting against the algorithm within
get_next_track_with_minimum_timestamp() return NULL, should this patch
also include immediately returning from imf_read_packet() if
get_next_track_with_minimum_timestamp() returns NULL?

On Thu, Aug 25, 2022 at 7:53 PM Steven Liu <lq@chinaffmpeg.org> wrote:
>
> fix CID: 1512414
>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/imfdec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
> index 5bbe7a53f8..a97dcc3b8b 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;
>  }
>
> --
> 2.25.0
>
> _______________________________________________
> 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".

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

[Steven Liu]

Pierre-Anthony Lemieux <pal@sandflow.com> 于2022年8月26日周五 11:52写道：
>
> Since we are protecting against the algorithm within
> get_next_track_with_minimum_timestamp() return NULL, should this patch
> also include immediately returning from imf_read_packet() if
> get_next_track_with_minimum_timestamp() returns NULL?
What about return AVERROR_INVALIDDATA in imf_read_packet?
>
> On Thu, Aug 25, 2022 at 7:53 PM Steven Liu <lq@chinaffmpeg.org> wrote:
> >
> > fix CID: 1512414
> >
> > Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> > ---
> >  libavformat/imfdec.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
> > index 5bbe7a53f8..a97dcc3b8b 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;
> >  }
> >
> > --
> > 2.25.0
> >
> > _______________________________________________
> > 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] [PATCH v2] avformat/imfdec: check track valid before use it

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

_______________________________________________
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".

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

[Andreas Rheinhardt]

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)

_______________________________________________
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".

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

[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?

> (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".
_______________________________________________
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".

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

[Andreas Rheinhardt]

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".

_______________________________________________
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".

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

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".

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

[Steven Liu]

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".
_______________________________________________
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".

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

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".

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

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".