| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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,
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(µdvd->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, µdvd->q); > Can you share the test case? Ill send you the file privatly thx [...]
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(µdvd->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, µdvd->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,
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 --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)
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(-)