diff mbox series

[FFmpeg-devel,2/2] avformat/subtitles: Check for NOPTS in ff_subtitles_queue_finalize()

Message ID 20200623001458.15855-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avformat/mvdec: Fix integer overflow with billions of channels
Related show

Checks

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

Commit Message

Michael Niedermayer June 23, 2020, 12:14 a.m. UTC
Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/subtitles.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas George June 23, 2020, 12:08 p.m. UTC | #1
Michael Niedermayer (12020-06-23):
> Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
> Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/subtitles.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
> index ad7f68938e..ccab80391f 100644
> --- a/libavformat/subtitles.c
> +++ b/libavformat/subtitles.c
> @@ -202,7 +202,7 @@ void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
>            q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
>                                       : cmp_pkt_sub_pos_ts);
>      for (i = 0; i < q->nb_subs; i++)
> -        if (q->subs[i].duration < 0 && i < q->nb_subs - 1)
> +        if (q->subs[i].duration < 0 && i < q->nb_subs - 1 && q->subs[i].pts != AV_NOPTS_VALUE)
>              q->subs[i].duration = q->subs[i + 1].pts - q->subs[i].pts;
>  
>      if (!q->keep_duplicates)

Having no PTS at this point makes no sense. We should examine why it
arrived there. Can you share the test case?

Regards,
Michael Niedermayer June 23, 2020, 11:37 p.m. UTC | #2
On Tue, Jun 23, 2020 at 02:08:44PM +0200, Nicolas George wrote:
> Michael Niedermayer (12020-06-23):
> > Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
> > Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/subtitles.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
> > index ad7f68938e..ccab80391f 100644
> > --- a/libavformat/subtitles.c
> > +++ b/libavformat/subtitles.c
> > @@ -202,7 +202,7 @@ void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
> >            q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
> >                                       : cmp_pkt_sub_pos_ts);
> >      for (i = 0; i < q->nb_subs; i++)
> > -        if (q->subs[i].duration < 0 && i < q->nb_subs - 1)
> > +        if (q->subs[i].duration < 0 && i < q->nb_subs - 1 && q->subs[i].pts != AV_NOPTS_VALUE)
> >              q->subs[i].duration = q->subs[i + 1].pts - q->subs[i].pts;
> >  
> >      if (!q->keep_duplicates)
> 
> Having no PTS at this point makes no sense. We should examine why it
> arrived there. 

it comes from microdvddec in this case, it can be fixed for example
with the following: (i do not know if these are all cases which can
generate this)

commit 06eff32c6e4100b70a3a215a0756580a6fa124b5 (HEAD -> master)
Author: Michael Niedermayer <michael@niedermayer.cc>
Date:   Tue Jun 23 01:43:14 2020 +0200

    iavformat/microdvddec: Skip NOPTS values
    
    Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
    Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424
    
    Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
    Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

diff --git a/libavformat/microdvddec.c b/libavformat/microdvddec.c
index 8759200f88..1f871b2518 100644
--- a/libavformat/microdvddec.c
+++ b/libavformat/microdvddec.c
@@ -94,6 +94,7 @@ static int microdvd_read_header(AVFormatContext *s)
         int64_t pos = avio_tell(s->pb);
         int len = ff_get_line(s->pb, line_buf, sizeof(line_buf));
         char *line = line_buf;
+        int64_t pts;
 
         if (!strncmp(line, bom, 3))
             line += 3;
@@ -137,13 +138,16 @@ static int microdvd_read_header(AVFormatContext *s)
         SKIP_FRAME_ID;
         if (!*p)
             continue;
+        pts = get_pts(line);
+        if (pts == AV_NOPTS_VALUE)
+            continue;
         sub = ff_subtitles_queue_insert(&microdvd->q, p, strlen(p), 0);
         if (!sub) {
             ret = AVERROR(ENOMEM);
             goto fail;
         }
         sub->pos = pos;
-        sub->pts = get_pts(line);
+        sub->pts = pts;
         sub->duration = get_duration(line);
     }
     ff_subtitles_queue_finalize(s, &microdvd->q);





> Can you share the test case?

Ill send you the file privatly

thx

[...]
Nicolas George June 24, 2020, 12:10 p.m. UTC | #3
Michael Niedermayer (12020-06-24):
> On Tue, Jun 23, 2020 at 02:08:44PM +0200, Nicolas George wrote:
> > Michael Niedermayer (12020-06-23):
> > > Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
> > > Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/subtitles.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
> > > index ad7f68938e..ccab80391f 100644
> > > --- a/libavformat/subtitles.c
> > > +++ b/libavformat/subtitles.c
> > > @@ -202,7 +202,7 @@ void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
> > >            q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
> > >                                       : cmp_pkt_sub_pos_ts);
> > >      for (i = 0; i < q->nb_subs; i++)
> > > -        if (q->subs[i].duration < 0 && i < q->nb_subs - 1)
> > > +        if (q->subs[i].duration < 0 && i < q->nb_subs - 1 && q->subs[i].pts != AV_NOPTS_VALUE)
> > >              q->subs[i].duration = q->subs[i + 1].pts - q->subs[i].pts;
> > >  
> > >      if (!q->keep_duplicates)
> > 
> > Having no PTS at this point makes no sense. We should examine why it
> > arrived there. 
> 
> it comes from microdvddec in this case, it can be fixed for example
> with the following: (i do not know if these are all cases which can
> generate this)

Thanks. It seems like a much more correct fix. We can see in
$FATE/sub/MicroDVD_capability_tester.sub that all lines have a number in
the first pair of braces except DEFAULT, which is parsed separately.

> commit 06eff32c6e4100b70a3a215a0756580a6fa124b5 (HEAD -> master)
> Author: Michael Niedermayer <michael@niedermayer.cc>
> Date:   Tue Jun 23 01:43:14 2020 +0200
> 

>     iavformat/microdvddec: Skip NOPTS values

skip malformed lines without frame number.

>     
>     Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'

Colon / semicolon.

>     Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424
>     
>     Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>     Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> 
> diff --git a/libavformat/microdvddec.c b/libavformat/microdvddec.c
> index 8759200f88..1f871b2518 100644
> --- a/libavformat/microdvddec.c
> +++ b/libavformat/microdvddec.c
> @@ -94,6 +94,7 @@ static int microdvd_read_header(AVFormatContext *s)
>          int64_t pos = avio_tell(s->pb);
>          int len = ff_get_line(s->pb, line_buf, sizeof(line_buf));
>          char *line = line_buf;
> +        int64_t pts;
>  
>          if (!strncmp(line, bom, 3))
>              line += 3;
> @@ -137,13 +138,16 @@ static int microdvd_read_header(AVFormatContext *s)
>          SKIP_FRAME_ID;
>          if (!*p)
>              continue;

> +        pts = get_pts(line);
> +        if (pts == AV_NOPTS_VALUE)
> +            continue;
>          sub = ff_subtitles_queue_insert(&microdvd->q, p, strlen(p), 0);
>          if (!sub) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
>          }
>          sub->pos = pos;
> -        sub->pts = get_pts(line);
> +        sub->pts = pts;
>          sub->duration = get_duration(line);
>      }
>      ff_subtitles_queue_finalize(s, &microdvd->q);
> 
> 
> 
> 
> 
> > Can you share the test case?
> 
> Ill send you the file privatly

I really do not see the point. I only understood what I was looking at
after reading your public mail and the source code, which means anybody
could get the same information. And it is not as if it was a real bug,
even less a security issue, just a case of garbage in garbage out.

Regards,
Michael Niedermayer June 24, 2020, 11:33 p.m. UTC | #4
On Wed, Jun 24, 2020 at 02:10:09PM +0200, Nicolas George wrote:
> Michael Niedermayer (12020-06-24):
> > On Tue, Jun 23, 2020 at 02:08:44PM +0200, Nicolas George wrote:
> > > Michael Niedermayer (12020-06-23):
> > > > Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
> > > > Fixes: 23490/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5133490093031424
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/subtitles.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
> > > > index ad7f68938e..ccab80391f 100644
> > > > --- a/libavformat/subtitles.c
> > > > +++ b/libavformat/subtitles.c
> > > > @@ -202,7 +202,7 @@ void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
> > > >            q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
> > > >                                       : cmp_pkt_sub_pos_ts);
> > > >      for (i = 0; i < q->nb_subs; i++)
> > > > -        if (q->subs[i].duration < 0 && i < q->nb_subs - 1)
> > > > +        if (q->subs[i].duration < 0 && i < q->nb_subs - 1 && q->subs[i].pts != AV_NOPTS_VALUE)
> > > >              q->subs[i].duration = q->subs[i + 1].pts - q->subs[i].pts;
> > > >  
> > > >      if (!q->keep_duplicates)
> > > 
> > > Having no PTS at this point makes no sense. We should examine why it
> > > arrived there. 
> > 
> > it comes from microdvddec in this case, it can be fixed for example
> > with the following: (i do not know if these are all cases which can
> > generate this)
> 
> Thanks. It seems like a much more correct fix. We can see in
> $FATE/sub/MicroDVD_capability_tester.sub that all lines have a number in
> the first pair of braces except DEFAULT, which is parsed separately.
> 
> > commit 06eff32c6e4100b70a3a215a0756580a6fa124b5 (HEAD -> master)
> > Author: Michael Niedermayer <michael@niedermayer.cc>
> > Date:   Tue Jun 23 01:43:14 2020 +0200
> > 
> 
> >     iavformat/microdvddec: Skip NOPTS values
> 
> skip malformed lines without frame number.
> 
> >     
> >     Fixes; signed integer overflow: 1 - -9223372036854775808 cannot be represented in type 'long'
> 
> Colon / semicolon.

will apply with these changes

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
index ad7f68938e..ccab80391f 100644
--- a/libavformat/subtitles.c
+++ b/libavformat/subtitles.c
@@ -202,7 +202,7 @@  void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
           q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
                                      : cmp_pkt_sub_pos_ts);
     for (i = 0; i < q->nb_subs; i++)
-        if (q->subs[i].duration < 0 && i < q->nb_subs - 1)
+        if (q->subs[i].duration < 0 && i < q->nb_subs - 1 && q->subs[i].pts != AV_NOPTS_VALUE)
             q->subs[i].duration = q->subs[i + 1].pts - q->subs[i].pts;
 
     if (!q->keep_duplicates)