| Message ID | 20210624205724.18711-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/matroskadec: Reset state also on failure in matroska_reset_status() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 6/24/2021 5:57 PM, Michael Niedermayer wrote: > Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long' > Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/sbgdec.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c > index dafdc4a1cc..0a6e927e57 100644 > --- a/libavformat/sbgdec.c > +++ b/libavformat/sbgdec.c > @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s) > } > if (s->start_ts == AV_NOPTS_VALUE) > s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now; > + if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration) Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration) check? Both s->start_ts and s->opt_duration are apparently guaranteed to be positive. > + return AVERROR_INVALIDDATA; > + > s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration : > AV_NOPTS_VALUE; /* may be overridden later by -E option */ > cur_ts = now; >
On Fri, Jun 25, 2021 at 09:22:03AM -0300, James Almer wrote: > On 6/24/2021 5:57 PM, Michael Niedermayer wrote: > > Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long' > > Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/sbgdec.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c > > index dafdc4a1cc..0a6e927e57 100644 > > --- a/libavformat/sbgdec.c > > +++ b/libavformat/sbgdec.c > > @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s) > > } > > if (s->start_ts == AV_NOPTS_VALUE) > > s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now; > > + if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration) > > Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration) > check? Both s->start_ts and s->opt_duration are apparently guaranteed to be > positive. The variables are read by str_to_time() which looks like it can read negative numbers. But maybe iam missing something thx [...]
On 6/25/2021 12:58 PM, Michael Niedermayer wrote: > On Fri, Jun 25, 2021 at 09:22:03AM -0300, James Almer wrote: >> On 6/24/2021 5:57 PM, Michael Niedermayer wrote: >>> Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long' >>> Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/sbgdec.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c >>> index dafdc4a1cc..0a6e927e57 100644 >>> --- a/libavformat/sbgdec.c >>> +++ b/libavformat/sbgdec.c >>> @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s) >>> } >>> if (s->start_ts == AV_NOPTS_VALUE) >>> s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now; >>> + if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration) >> >> Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration) >> check? Both s->start_ts and s->opt_duration are apparently guaranteed to be >> positive. > > The variables are read by str_to_time() which looks like it can read negative > numbers. Afaics, it checks the very first character to be < '0' || > '9' for both hours and minutes, so strtol() is not going to see a '-'. Is there supposed to be one for seconds in valid files? If not, the same check could be done and ensure no negative value is parsed. > But maybe iam missing something > > thx > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
James Almer (12021-06-25): > Afaics, it checks the very first character to be < '0' || > '9' for both > hours and minutes, so strtol() is not going to see a '-'. > Is there supposed to be one for seconds in valid files? If not, the same > check could be done and ensure no negative value is parsed. No, valid files cannot have negative times. Also, I think with the time that adding this format was a misguided idea. I suspect nobody uses it. We could just remove it. Regards,
On Sun, Jun 27, 2021 at 08:22:12PM +0200, Nicolas George wrote: > James Almer (12021-06-25): > > Afaics, it checks the very first character to be < '0' || > '9' for both > > hours and minutes, so strtol() is not going to see a '-'. > > Is there supposed to be one for seconds in valid files? If not, the same > > check could be done and ensure no negative value is parsed. > > No, valid files cannot have negative times. > > Also, I think with the time that adding this format was a misguided > idea. I suspect nobody uses it. We could just remove it. maybe you could go over the parsing/reading of values in it and add appropriate checks for all. This would likely cut down on the amount of integer issues, and someone like you who knows the format well can likely do this faster than others like me fixing one issue found by the fuzzers at a time Thanks [...]
On Fri, Jun 25, 2021 at 09:22:03AM -0300, James Almer wrote: > On 6/24/2021 5:57 PM, Michael Niedermayer wrote: > > Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long' > > Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/sbgdec.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c > > index dafdc4a1cc..0a6e927e57 100644 > > --- a/libavformat/sbgdec.c > > +++ b/libavformat/sbgdec.c > > @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s) > > } > > if (s->start_ts == AV_NOPTS_VALUE) > > s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now; > > + if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration) > > Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration) > check? Both s->start_ts and s->opt_duration are apparently guaranteed to be > positive. will apply with this variant thx [...]
diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c index dafdc4a1cc..0a6e927e57 100644 --- a/libavformat/sbgdec.c +++ b/libavformat/sbgdec.c @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s) } if (s->start_ts == AV_NOPTS_VALUE) s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now; + if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration) + return AVERROR_INVALIDDATA; + s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration : AV_NOPTS_VALUE; /* may be overridden later by -E option */ cur_ts = now;
Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long' Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/sbgdec.c | 3 +++ 1 file changed, 3 insertions(+)