diff mbox series

[FFmpeg-devel] avformat/mux: Check pkt->stream_index before using it

Message ID 20200507225500.26860-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 39195896f31aef7e96f1e4748e569d930df9d0c9
Headers show
Series [FFmpeg-devel] avformat/mux: Check pkt->stream_index before using it
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 7, 2020, 10:55 p.m. UTC
This commit fixes two recent regressions both of which are about using
pkt->stream_index as index in an AVFormatContext's streams array before
actually comparing the value with the count of streams in said array.
96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
likewise in write_packets_common().

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
The same error in the same file applied on the same day by two different
people. How unlikely.

 libavformat/mux.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Anton Khirnov May 10, 2020, 5:36 p.m. UTC | #1
Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
> This commit fixes two recent regressions both of which are about using
> pkt->stream_index as index in an AVFormatContext's streams array before
> actually comparing the value with the count of streams in said array.
> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
> likewise in write_packets_common().
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> The same error in the same file applied on the same day by two different
> people. How unlikely.

How is it a regression? Isn't it rather invalid API use?

Not that I object to having a check. But then why is check_packet()
called so deep and not immediately on entry to the muxer?
Marton Balint May 10, 2020, 5:45 p.m. UTC | #2
On Sun, 10 May 2020, Anton Khirnov wrote:

> Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
>> This commit fixes two recent regressions both of which are about using
>> pkt->stream_index as index in an AVFormatContext's streams array before
>> actually comparing the value with the count of streams in said array.
>> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
>> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
>> likewise in write_packets_common().
>> 
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> The same error in the same file applied on the same day by two different
>> people. How unlikely.
>
> How is it a regression? Isn't it rather invalid API use?

Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6

Yes, it is kind of invalid API use, but since the check is already there, 
we should make it actually worthwile.

>
> Not that I object to having a check. But then why is check_packet()
> called so deep and not immediately on entry to the muxer?

I guess it is not that deep, but recent factorization efforts hidden it a 
bit.

Regards,
Marton
Anton Khirnov May 10, 2020, 6:07 p.m. UTC | #3
Quoting Marton Balint (2020-05-10 19:45:04)
> 
> 
> On Sun, 10 May 2020, Anton Khirnov wrote:
> 
> > Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
> >> This commit fixes two recent regressions both of which are about using
> >> pkt->stream_index as index in an AVFormatContext's streams array before
> >> actually comparing the value with the count of streams in said array.
> >> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
> >> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
> >> likewise in write_packets_common().
> >> 
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >> The same error in the same file applied on the same day by two different
> >> people. How unlikely.
> >
> > How is it a regression? Isn't it rather invalid API use?
> 
> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6
> 
> Yes, it is kind of invalid API use, but since the check is already there, 
> we should make it actually worthwile.

lol

I agree that checking for it is a good idea, obviously, but I wouldn't
call it a regression.

> 
> >
> > Not that I object to having a check. But then why is check_packet()
> > called so deep and not immediately on entry to the muxer?
> 
> I guess it is not that deep, but recent factorization efforts hidden it a 
> bit.

You can see in my original commit it is the very first thing done after
entering the muxer. Right now it's several function calls deep.
Andreas Rheinhardt May 10, 2020, 7:35 p.m. UTC | #4
Anton Khirnov:
> Quoting Marton Balint (2020-05-10 19:45:04)
>>
>>
>> On Sun, 10 May 2020, Anton Khirnov wrote:
>>
>>> Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
>>>> This commit fixes two recent regressions both of which are about using
>>>> pkt->stream_index as index in an AVFormatContext's streams array before
>>>> actually comparing the value with the count of streams in said array.
>>>> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
>>>> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
>>>> likewise in write_packets_common().
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> ---
>>>> The same error in the same file applied on the same day by two different
>>>> people. How unlikely.
>>>
>>> How is it a regression? Isn't it rather invalid API use?
>>
>> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6
>>
>> Yes, it is kind of invalid API use, but since the check is already there, 
>> we should make it actually worthwile.
> 
> lol
> 
> I agree that checking for it is a good idea, obviously, but I wouldn't
> call it a regression.
> 
How about rephrasing the first sentence to: "This commit stops using
pkt->stream_index as index in an AVFormatContext's streams array before
actually comparing the value with the count of streams in said array."
>>
>>>
>>> Not that I object to having a check. But then why is check_packet()
>>> called so deep and not immediately on entry to the muxer?
>>
>> I guess it is not that deep, but recent factorization efforts hidden it a 
>> bit.
> 
> You can see in my original commit it is the very first thing done after
> entering the muxer. Right now it's several function calls deep.
> 
I could make it the very first thing called in write_packets_common().

- Andreas
Anton Khirnov May 11, 2020, 2:56 p.m. UTC | #5
Quoting Andreas Rheinhardt (2020-05-10 21:35:54)
> Anton Khirnov:
> > Quoting Marton Balint (2020-05-10 19:45:04)
> >>
> >>
> >> On Sun, 10 May 2020, Anton Khirnov wrote:
> >>
> >>> Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
> >>>> This commit fixes two recent regressions both of which are about using
> >>>> pkt->stream_index as index in an AVFormatContext's streams array before
> >>>> actually comparing the value with the count of streams in said array.
> >>>> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
> >>>> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
> >>>> likewise in write_packets_common().
> >>>>
> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>> ---
> >>>> The same error in the same file applied on the same day by two different
> >>>> people. How unlikely.
> >>>
> >>> How is it a regression? Isn't it rather invalid API use?
> >>
> >> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6
> >>
> >> Yes, it is kind of invalid API use, but since the check is already there, 
> >> we should make it actually worthwile.
> > 
> > lol
> > 
> > I agree that checking for it is a good idea, obviously, but I wouldn't
> > call it a regression.
> > 
> How about rephrasing the first sentence to: "This commit stops using
> pkt->stream_index as index in an AVFormatContext's streams array before
> actually comparing the value with the count of streams in said array."

Sure, sounds good.

> >>
> >>>
> >>> Not that I object to having a check. But then why is check_packet()
> >>> called so deep and not immediately on entry to the muxer?
> >>
> >> I guess it is not that deep, but recent factorization efforts hidden it a 
> >> bit.
> > 
> > You can see in my original commit it is the very first thing done after
> > entering the muxer. Right now it's several function calls deep.
> > 
> I could make it the very first thing called in write_packets_common().

Why not move the check_packet() call out of prepare_packet() into
av_[interleaved_]write_frame() instead?
Andreas Rheinhardt May 11, 2020, 9:58 p.m. UTC | #6
Anton Khirnov:
> Quoting Andreas Rheinhardt (2020-05-10 21:35:54)
>> Anton Khirnov:
>>> Quoting Marton Balint (2020-05-10 19:45:04)
>>>>
>>>>
>>>> On Sun, 10 May 2020, Anton Khirnov wrote:
>>>>
>>>>> Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
>>>>>> This commit fixes two recent regressions both of which are about using
>>>>>> pkt->stream_index as index in an AVFormatContext's streams array before
>>>>>> actually comparing the value with the count of streams in said array.
>>>>>> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
>>>>>> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
>>>>>> likewise in write_packets_common().
>>>>>>
>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>>>> ---
>>>>>> The same error in the same file applied on the same day by two different
>>>>>> people. How unlikely.
>>>>>
>>>>> How is it a regression? Isn't it rather invalid API use?
>>>>
>>>> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6
>>>>
>>>> Yes, it is kind of invalid API use, but since the check is already there, 
>>>> we should make it actually worthwile.
>>>
>>> lol
>>>
>>> I agree that checking for it is a good idea, obviously, but I wouldn't
>>> call it a regression.
>>>
>> How about rephrasing the first sentence to: "This commit stops using
>> pkt->stream_index as index in an AVFormatContext's streams array before
>> actually comparing the value with the count of streams in said array."
> 
> Sure, sounds good.
> 
>>>>
>>>>>
>>>>> Not that I object to having a check. But then why is check_packet()
>>>>> called so deep and not immediately on entry to the muxer?
>>>>
>>>> I guess it is not that deep, but recent factorization efforts hidden it a 
>>>> bit.
>>>
>>> You can see in my original commit it is the very first thing done after
>>> entering the muxer. Right now it's several function calls deep.
>>>
>> I could make it the very first thing called in write_packets_common().
> 
> Why not move the check_packet() call out of prepare_packet() into
> av_[interleaved_]write_frame() instead?
> 
This would add code duplication and IMO it is nicer to only check a
packet for validity if there is actually a packet to check.

- Andreas
Anton Khirnov May 12, 2020, 7:48 a.m. UTC | #7
Quoting Andreas Rheinhardt (2020-05-11 23:58:47)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2020-05-10 21:35:54)
> >> Anton Khirnov:
> >>> Quoting Marton Balint (2020-05-10 19:45:04)
> >>>>
> >>>>
> >>>> On Sun, 10 May 2020, Anton Khirnov wrote:
> >>>>
> >>>>> Quoting Andreas Rheinhardt (2020-05-08 00:55:00)
> >>>>>> This commit fixes two recent regressions both of which are about using
> >>>>>> pkt->stream_index as index in an AVFormatContext's streams array before
> >>>>>> actually comparing the value with the count of streams in said array.
> >>>>>> 96e5e6abb9851d7a26ba21703955d5826ac857c0 did this in
> >>>>>> prepare_input_packet() and 64063512227c4c87a7d16a1076481dc6baf19841 did
> >>>>>> likewise in write_packets_common().
> >>>>>>
> >>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>>>> ---
> >>>>>> The same error in the same file applied on the same day by two different
> >>>>>> people. How unlikely.
> >>>>>
> >>>>> How is it a regression? Isn't it rather invalid API use?
> >>>>
> >>>> Fun fact: 7b03b65bf0d02519c86750d2da33f413e11cf0c6
> >>>>
> >>>> Yes, it is kind of invalid API use, but since the check is already there, 
> >>>> we should make it actually worthwile.
> >>>
> >>> lol
> >>>
> >>> I agree that checking for it is a good idea, obviously, but I wouldn't
> >>> call it a regression.
> >>>
> >> How about rephrasing the first sentence to: "This commit stops using
> >> pkt->stream_index as index in an AVFormatContext's streams array before
> >> actually comparing the value with the count of streams in said array."
> > 
> > Sure, sounds good.
> > 
> >>>>
> >>>>>
> >>>>> Not that I object to having a check. But then why is check_packet()
> >>>>> called so deep and not immediately on entry to the muxer?
> >>>>
> >>>> I guess it is not that deep, but recent factorization efforts hidden it a 
> >>>> bit.
> >>>
> >>> You can see in my original commit it is the very first thing done after
> >>> entering the muxer. Right now it's several function calls deep.
> >>>
> >> I could make it the very first thing called in write_packets_common().
> > 
> > Why not move the check_packet() call out of prepare_packet() into
> > av_[interleaved_]write_frame() instead?
> > 
> This would add code duplication and IMO it is nicer to only check a
> packet for validity if there is actually a packet to check.

Okay. I don't entirely agree, but this is not important enough to waste
time arguing about. Feel free to push.
diff mbox series

Patch

diff --git a/libavformat/mux.c b/libavformat/mux.c
index 41659b19c9..f2de73af9b 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -761,12 +761,13 @@  static int check_packet(AVFormatContext *s, AVPacket *pkt)
 
 static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt)
 {
+    AVStream *st;
     int ret;
-    AVStream *st = s->streams[pkt->stream_index];
 
     ret = check_packet(s, pkt);
     if (ret < 0)
         return ret;
+    st = s->streams[pkt->stream_index];
 
 #if !FF_API_COMPUTE_PKT_FIELDS2 || !FF_API_LAVF_AVCTX
     /* sanitize the timestamps */
@@ -1176,10 +1177,11 @@  static int write_packets_from_bsfs(AVFormatContext *s, AVStream *st, AVPacket *p
 
 static int write_packets_common(AVFormatContext *s, AVPacket *pkt, int interleaved)
 {
-    AVStream *st = s->streams[pkt->stream_index];
+    AVStream *st;
     int ret = prepare_input_packet(s, pkt);
     if (ret < 0)
         return ret;
+    st = s->streams[pkt->stream_index];
 
     ret = check_bitstream(s, st, pkt);
     if (ret < 0)