| Message ID | 20190924163144.26443-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | e045be92cdf5a2851900e8e85b815c29ae6f100a |
| Headers | show |
On 24-09-2019 10:01 PM, Andreas Rheinhardt wrote: > There is currently an ordinary check for this (which would lead to a > memleak), but given that no demuxer should ever return a packet with an > invalid stream_index it is more appropriate for this to be an assert. > > FATE passes with this change. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/utils.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 3983a3f4ce..d8ef5fe54c 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -884,10 +884,8 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > continue; > } > > - if (pkt->stream_index >= (unsigned)s->nb_streams) { > - av_log(s, AV_LOG_ERROR, "Invalid stream index %d\n", pkt->stream_index); > - continue; > - } > + av_assert0(pkt->stream_index < (unsigned)s->nb_streams && > + "Invalid stream index.\n"); The mpegts demuxer can send packets from new streams which appear mid-way. See the check in ffmpeg.c process_input() which discards these packets. Whereas the patch will lead to an abort in fftools and other user apps transcoding a live feed. Gyan
On Tue, 24 Sep 2019, Gyan wrote: > > > On 24-09-2019 10:01 PM, Andreas Rheinhardt wrote: >> There is currently an ordinary check for this (which would lead to a >> memleak), but given that no demuxer should ever return a packet with an >> invalid stream_index it is more appropriate for this to be an assert. >> >> FATE passes with this change. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/utils.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index 3983a3f4ce..d8ef5fe54c 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -884,10 +884,8 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) >> continue; >> } >> >> - if (pkt->stream_index >= (unsigned)s->nb_streams) { >> - av_log(s, AV_LOG_ERROR, "Invalid stream index %d\n", > pkt->stream_index); >> - continue; >> - } >> + av_assert0(pkt->stream_index < (unsigned)s->nb_streams && >> + "Invalid stream index.\n"); > > The mpegts demuxer can send packets from new streams which appear > mid-way. See the check in ffmpeg.c process_input() which discards these > packets. That checks InputFile->nb_streams not AVFormatContext->nb_streams as far as I see. > Whereas the patch will lead to an abort in fftools and other user apps > transcoding a live feed. Are you sure? I think the demuxer always creates a new stream before returning a packet, so AVFormatContext->nb_streams should always be up-to-date when this code runs. Regards, Marton
On 24-09-2019 11:38 PM, Marton Balint wrote: > > > On Tue, 24 Sep 2019, Gyan wrote: > >> >> >> On 24-09-2019 10:01 PM, Andreas Rheinhardt wrote: >>> There is currently an ordinary check for this (which would lead to a >>> memleak), but given that no demuxer should ever return a packet with an >>> invalid stream_index it is more appropriate for this to be an assert. >>> >>> FATE passes with this change. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavformat/utils.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>> index 3983a3f4ce..d8ef5fe54c 100644 >>> --- a/libavformat/utils.c >>> +++ b/libavformat/utils.c >>> @@ -884,10 +884,8 @@ int ff_read_packet(AVFormatContext *s, AVPacket >>> *pkt) >>> continue; >>> } >>> >>> - if (pkt->stream_index >= (unsigned)s->nb_streams) { >>> - av_log(s, AV_LOG_ERROR, "Invalid stream index %d\n", >> pkt->stream_index); >>> - continue; >>> - } >>> + av_assert0(pkt->stream_index < (unsigned)s->nb_streams && >>> + "Invalid stream index.\n"); >> >> The mpegts demuxer can send packets from new streams which appear >> mid-way. See the check in ffmpeg.c process_input() which discards >> these packets. > > That checks InputFile->nb_streams not AVFormatContext->nb_streams as > far as I see. > >> Whereas the patch will lead to an abort in fftools and other user >> apps transcoding a live feed. > > Are you sure? I think the demuxer always creates a new stream before > returning a packet, so AVFormatContext->nb_streams should always be > up-to-date when this code runs. No. I'll have to craft an input and check. Gyan
On 24-09-2019 11:39 PM, Gyan wrote: > > > On 24-09-2019 11:38 PM, Marton Balint wrote: >> >> >> On Tue, 24 Sep 2019, Gyan wrote: >> >>> >>> >>> On 24-09-2019 10:01 PM, Andreas Rheinhardt wrote: >>>> There is currently an ordinary check for this (which would lead to a >>>> memleak), but given that no demuxer should ever return a packet >>>> with an >>>> invalid stream_index it is more appropriate for this to be an assert. >>>> >>>> FATE passes with this change. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> libavformat/utils.c | 6 ++---- >>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>>> index 3983a3f4ce..d8ef5fe54c 100644 >>>> --- a/libavformat/utils.c >>>> +++ b/libavformat/utils.c >>>> @@ -884,10 +884,8 @@ int ff_read_packet(AVFormatContext *s, >>>> AVPacket *pkt) >>>> continue; >>>> } >>>> >>>> - if (pkt->stream_index >= (unsigned)s->nb_streams) { >>>> - av_log(s, AV_LOG_ERROR, "Invalid stream index %d\n", >>> pkt->stream_index); >>>> - continue; >>>> - } >>>> + av_assert0(pkt->stream_index < (unsigned)s->nb_streams && >>>> + "Invalid stream index.\n"); >>> >>> The mpegts demuxer can send packets from new streams which appear >>> mid-way. See the check in ffmpeg.c process_input() which discards >>> these packets. >> >> That checks InputFile->nb_streams not AVFormatContext->nb_streams as >> far as I see. >> >>> Whereas the patch will lead to an abort in fftools and other user >>> apps transcoding a live feed. >> >> Are you sure? I think the demuxer always creates a new stream before >> returning a packet, so AVFormatContext->nb_streams should always be >> up-to-date when this code runs. > > No. I'll have to craft an input and check. Tested. Yes, the format context is updated, but InputFiles isn't. Comment withdrawn. Sorry. Gyan
diff --git a/libavformat/utils.c b/libavformat/utils.c index 3983a3f4ce..d8ef5fe54c 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -884,10 +884,8 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) continue; } - if (pkt->stream_index >= (unsigned)s->nb_streams) { - av_log(s, AV_LOG_ERROR, "Invalid stream index %d\n", pkt->stream_index); - continue; - } + av_assert0(pkt->stream_index < (unsigned)s->nb_streams && + "Invalid stream index.\n"); st = s->streams[pkt->stream_index];
There is currently an ordinary check for this (which would lead to a memleak), but given that no demuxer should ever return a packet with an invalid stream_index it is more appropriate for this to be an assert. FATE passes with this change. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/utils.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)