[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 at 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 at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".