| Message ID | 20191022131645.8394-5-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Oct 22, 2019 at 3:17 PM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote: > vobsub_read_packet() didn't check whether an index in array of AVPackets > was valid and therefore used uninitialized values. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Actually I only wanted to use Valgrind to check for memleaks... > > libavformat/mpeg.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c > index 73ade71d95..474afd06b9 100644 > --- a/libavformat/mpeg.c > +++ b/libavformat/mpeg.c > @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, > AVPacket *pkt) > FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; > int64_t ts; > av_assert0(tmpq->nb_subs); > + > + if (tmpq->current_sub_idx >= tmpq->nb_subs) > + continue; > + > ts = tmpq->subs[tmpq->current_sub_idx].pts; > if (ts < min_ts) { > min_ts = ts; > -- > 2.20.1 > > Ping. - Andreas
On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: > vobsub_read_packet() didn't check whether an index in array of AVPackets > was valid and therefore used uninitialized values. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Actually I only wanted to use Valgrind to check for memleaks... > > libavformat/mpeg.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c > index 73ade71d95..474afd06b9 100644 > --- a/libavformat/mpeg.c > +++ b/libavformat/mpeg.c > @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) > FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; > int64_t ts; > av_assert0(tmpq->nb_subs); > + > + if (tmpq->current_sub_idx >= tmpq->nb_subs) > + continue; How can this issue be reproduced ? thx [...]
Michael Niedermayer: > On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: >> vobsub_read_packet() didn't check whether an index in array of AVPackets >> was valid and therefore used uninitialized values. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> Actually I only wanted to use Valgrind to check for memleaks... >> >> libavformat/mpeg.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >> index 73ade71d95..474afd06b9 100644 >> --- a/libavformat/mpeg.c >> +++ b/libavformat/mpeg.c >> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; >> int64_t ts; >> av_assert0(tmpq->nb_subs); >> + >> + if (tmpq->current_sub_idx >= tmpq->nb_subs) >> + continue; > > How can this issue be reproduced ? > > thx > > [...] Read a VobSub subtitle till the end: ffmpeg -i <idx input file> -c copy -f null - - Andreas
On Sun, Jan 19, 2020 at 3:44 PM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote: > Michael Niedermayer: > > On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: > >> vobsub_read_packet() didn't check whether an index in array of AVPackets > >> was valid and therefore used uninitialized values. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> Actually I only wanted to use Valgrind to check for memleaks... > >> > >> libavformat/mpeg.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c > >> index 73ade71d95..474afd06b9 100644 > >> --- a/libavformat/mpeg.c > >> +++ b/libavformat/mpeg.c > >> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, > AVPacket *pkt) > >> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; > >> int64_t ts; > >> av_assert0(tmpq->nb_subs); > >> + > >> + if (tmpq->current_sub_idx >= tmpq->nb_subs) > >> + continue; > > > > How can this issue be reproduced ? > > > > thx > > > > [...] > > Read a VobSub subtitle till the end: > ffmpeg -i <idx input file> -c copy -f null - > > - Andreas > Ping. - Andreas
Andreas Rheinhardt: > On Sun, Jan 19, 2020 at 3:44 PM Andreas Rheinhardt < > andreas.rheinhardt@gmail.com> wrote: > >> Michael Niedermayer: >>> On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: >>>> vobsub_read_packet() didn't check whether an index in array of AVPackets >>>> was valid and therefore used uninitialized values. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> Actually I only wanted to use Valgrind to check for memleaks... >>>> >>>> libavformat/mpeg.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >>>> index 73ade71d95..474afd06b9 100644 >>>> --- a/libavformat/mpeg.c >>>> +++ b/libavformat/mpeg.c >>>> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, >> AVPacket *pkt) >>>> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; >>>> int64_t ts; >>>> av_assert0(tmpq->nb_subs); >>>> + >>>> + if (tmpq->current_sub_idx >= tmpq->nb_subs) >>>> + continue; >>> >>> How can this issue be reproduced ? >>> >>> thx >>> >>> [...] >> >> Read a VobSub subtitle till the end: >> ffmpeg -i <idx input file> -c copy -f null - >> >> - Andreas >> > > Ping. > > - Andreas > Ping. - Andreas
On Sun, Jan 19, 2020 at 02:43:00PM +0000, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: > >> vobsub_read_packet() didn't check whether an index in array of AVPackets > >> was valid and therefore used uninitialized values. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> Actually I only wanted to use Valgrind to check for memleaks... > >> > >> libavformat/mpeg.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c > >> index 73ade71d95..474afd06b9 100644 > >> --- a/libavformat/mpeg.c > >> +++ b/libavformat/mpeg.c > >> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) > >> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; > >> int64_t ts; > >> av_assert0(tmpq->nb_subs); > >> + > >> + if (tmpq->current_sub_idx >= tmpq->nb_subs) > >> + continue; > > > > How can this issue be reproduced ? > > > > thx > > > > [...] > > Read a VobSub subtitle till the end: > ffmpeg -i <idx input file> -c copy -f null - does not reproduce ./ffmpeg -i fate/sub/vobsub.idx -c copy -f null - Output file #0 does not contain any stream code does not execute ./ffmpeg -i fate/sub/vobsub.idx -c copy -map s -f null - the if does not execute iam obviously doing something wrong, can you provide a unambigous testcase? Thanks [...]
Michael Niedermayer: > On Sun, Jan 19, 2020 at 02:43:00PM +0000, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: >>>> vobsub_read_packet() didn't check whether an index in array of AVPackets >>>> was valid and therefore used uninitialized values. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> Actually I only wanted to use Valgrind to check for memleaks... >>>> >>>> libavformat/mpeg.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >>>> index 73ade71d95..474afd06b9 100644 >>>> --- a/libavformat/mpeg.c >>>> +++ b/libavformat/mpeg.c >>>> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; >>>> int64_t ts; >>>> av_assert0(tmpq->nb_subs); >>>> + >>>> + if (tmpq->current_sub_idx >= tmpq->nb_subs) >>>> + continue; >>> >>> How can this issue be reproduced ? >>> >>> thx >>> >>> [...] >> >> Read a VobSub subtitle till the end: >> ffmpeg -i <idx input file> -c copy -f null - > > does not reproduce > > ./ffmpeg -i fate/sub/vobsub.idx -c copy -f null - > Output file #0 does not contain any stream > code does not execute Sorry, I forgot that subtitle streams are not enabled by default. > > ./ffmpeg -i fate/sub/vobsub.idx -c copy -map s -f null - > the if does not execute > > iam obviously doing something wrong, can you provide a unambigous testcase? > You are not doing anything wrong; it's just that the sample is truncated: The idx file claims to contain 47 packets, yet the sub file contains only 46. As a result, trying to read the 47. packet (the last packet that exists in the FFDemuxSubtitlesQueue) fails and no attempt to read another packet (which would trigger what I told you) is attempted. You can reproduce this by deleting the last line in vobsub.idx. - Andreas
Andreas Rheinhardt: > Michael Niedermayer: >> On Sun, Jan 19, 2020 at 02:43:00PM +0000, Andreas Rheinhardt wrote: >>> Michael Niedermayer: >>>> On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: >>>>> vobsub_read_packet() didn't check whether an index in array of AVPackets >>>>> was valid and therefore used uninitialized values. >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>>> --- >>>>> Actually I only wanted to use Valgrind to check for memleaks... >>>>> >>>>> libavformat/mpeg.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >>>>> index 73ade71d95..474afd06b9 100644 >>>>> --- a/libavformat/mpeg.c >>>>> +++ b/libavformat/mpeg.c >>>>> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>>> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; >>>>> int64_t ts; >>>>> av_assert0(tmpq->nb_subs); >>>>> + >>>>> + if (tmpq->current_sub_idx >= tmpq->nb_subs) >>>>> + continue; >>>> >>>> How can this issue be reproduced ? >>>> >>>> thx >>>> >>>> [...] >>> >>> Read a VobSub subtitle till the end: >>> ffmpeg -i <idx input file> -c copy -f null - >> >> does not reproduce >> >> ./ffmpeg -i fate/sub/vobsub.idx -c copy -f null - >> Output file #0 does not contain any stream >> code does not execute > > Sorry, I forgot that subtitle streams are not enabled by default. >> >> ./ffmpeg -i fate/sub/vobsub.idx -c copy -map s -f null - >> the if does not execute >> >> iam obviously doing something wrong, can you provide a unambigous testcase? >> > You are not doing anything wrong; it's just that the sample is > truncated: The idx file claims to contain 47 packets, yet the sub file > contains only 46. As a result, trying to read the 47. packet (the last > packet that exists in the FFDemuxSubtitlesQueue) fails and no attempt to > read another packet (which would trigger what I told you) is attempted. > > You can reproduce this by deleting the last line in vobsub.idx. > > - Andreas > Ping. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Michael Niedermayer: >>> On Sun, Jan 19, 2020 at 02:43:00PM +0000, Andreas Rheinhardt wrote: >>>> Michael Niedermayer: >>>>> On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: >>>>>> vobsub_read_packet() didn't check whether an index in array of AVPackets >>>>>> was valid and therefore used uninitialized values. >>>>>> >>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>>>> --- >>>>>> Actually I only wanted to use Valgrind to check for memleaks... >>>>>> >>>>>> libavformat/mpeg.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >>>>>> index 73ade71d95..474afd06b9 100644 >>>>>> --- a/libavformat/mpeg.c >>>>>> +++ b/libavformat/mpeg.c >>>>>> @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>>>> FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; >>>>>> int64_t ts; >>>>>> av_assert0(tmpq->nb_subs); >>>>>> + >>>>>> + if (tmpq->current_sub_idx >= tmpq->nb_subs) >>>>>> + continue; >>>>> >>>>> How can this issue be reproduced ? >>>>> >>>>> thx >>>>> >>>>> [...] >>>> >>>> Read a VobSub subtitle till the end: >>>> ffmpeg -i <idx input file> -c copy -f null - >>> >>> does not reproduce >>> >>> ./ffmpeg -i fate/sub/vobsub.idx -c copy -f null - >>> Output file #0 does not contain any stream >>> code does not execute >> >> Sorry, I forgot that subtitle streams are not enabled by default. >>> >>> ./ffmpeg -i fate/sub/vobsub.idx -c copy -map s -f null - >>> the if does not execute >>> >>> iam obviously doing something wrong, can you provide a unambigous testcase? >>> >> You are not doing anything wrong; it's just that the sample is >> truncated: The idx file claims to contain 47 packets, yet the sub file >> contains only 46. As a result, trying to read the 47. packet (the last >> packet that exists in the FFDemuxSubtitlesQueue) fails and no attempt to >> read another packet (which would trigger what I told you) is attempted. >> >> You can reproduce this by deleting the last line in vobsub.idx. >> >> - Andreas >> > Ping. > > - Andreas > Ping. - Andreas
On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: > vobsub_read_packet() didn't check whether an index in array of AVPackets > was valid and therefore used uninitialized values. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Actually I only wanted to use Valgrind to check for memleaks... > > libavformat/mpeg.c | 4 ++++ > 1 file changed, 4 insertions(+) LGTM thx [...]
Michael Niedermayer: > On Tue, Oct 22, 2019 at 03:16:45PM +0200, Andreas Rheinhardt wrote: >> vobsub_read_packet() didn't check whether an index in array of AVPackets >> was valid and therefore used uninitialized values. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> Actually I only wanted to use Valgrind to check for memleaks... >> >> libavformat/mpeg.c | 4 ++++ >> 1 file changed, 4 insertions(+) > > LGTM > > thx > Applied, thanks. - Andreas
diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c index 73ade71d95..474afd06b9 100644 --- a/libavformat/mpeg.c +++ b/libavformat/mpeg.c @@ -930,6 +930,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) FFDemuxSubtitlesQueue *tmpq = &vobsub->q[i]; int64_t ts; av_assert0(tmpq->nb_subs); + + if (tmpq->current_sub_idx >= tmpq->nb_subs) + continue; + ts = tmpq->subs[tmpq->current_sub_idx].pts; if (ts < min_ts) { min_ts = ts;
vobsub_read_packet() didn't check whether an index in array of AVPackets was valid and therefore used uninitialized values. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Actually I only wanted to use Valgrind to check for memleaks... libavformat/mpeg.c | 4 ++++ 1 file changed, 4 insertions(+)