| Message ID | 20180417001346.8174-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 2324ef1ff32e5effd6f295bca80580ae4816be0b |
| Headers | show |
tis 2018-04-17 klockan 02:13 +0200 skrev Michael Niedermayer: > Speeds up decoding from 8 to 3 seconds for 6302/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5626371985375232 > Fixes: Timeout > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cinepak.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > index 89e940ae0d..ba0589582f 100644 > --- a/libavcodec/cinepak.c > +++ b/libavcodec/cinepak.c > @@ -315,14 +315,11 @@ static int cinepak_decode_strip (CinepakContext *s, > return AVERROR_INVALIDDATA; > } > > -static int cinepak_decode (CinepakContext *s) > +static int cinepak_predecode_check (CinepakContext *s) > { > - const uint8_t *eod = (s->data + s->size); > - int i, result, strip_size, frame_flags, num_strips; > - int y0 = 0; > + int num_strips; > int encoded_buf_size; > > - frame_flags = s->data[0]; > num_strips = AV_RB16 (&s->data[8]); > encoded_buf_size = AV_RB24(&s->data[1]); > > @@ -353,6 +350,21 @@ static int cinepak_decode (CinepakContext *s) > s->sega_film_skip_bytes = 0; > } > > + if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > + return AVERROR_INVALIDDATA; Looks like an extra check, not just moving existing checks as the commit message implies /Tomas
On Tue, Apr 17, 2018 at 10:32:16AM +0200, Tomas Härdin wrote: > tis 2018-04-17 klockan 02:13 +0200 skrev Michael Niedermayer: > > Speeds up decoding from 8 to 3 seconds for 6302/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5626371985375232 > > Fixes: Timeout > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/cinepak.c | 27 ++++++++++++++++++++++----- > > 1 file changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > index 89e940ae0d..ba0589582f 100644 > > --- a/libavcodec/cinepak.c > > +++ b/libavcodec/cinepak.c > > @@ -315,14 +315,11 @@ static int cinepak_decode_strip (CinepakContext *s, > > return AVERROR_INVALIDDATA; > > } > > > > -static int cinepak_decode (CinepakContext *s) > > +static int cinepak_predecode_check (CinepakContext *s) > > { > > - const uint8_t *eod = (s->data + s->size); > > - int i, result, strip_size, frame_flags, num_strips; > > - int y0 = 0; > > + int num_strips; > > int encoded_buf_size; > > > > - frame_flags = s->data[0]; > > num_strips = AV_RB16 (&s->data[8]); > > encoded_buf_size = AV_RB24(&s->data[1]); > > > > @@ -353,6 +350,21 @@ static int cinepak_decode (CinepakContext *s) > > s->sega_film_skip_bytes = 0; > > } > > > > + if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > > + return AVERROR_INVALIDDATA; > > Looks like an extra check, not just moving existing checks as the > commit message implies its from this: for (i=0; i < num_strips; i++) { if ((s->data + 12) > eod) return AVERROR_INVALIDDATA; ... But i can split this in a separte patch if you prefer ? [...]
tis 2018-04-17 klockan 14:24 +0200 skrev Michael Niedermayer: > On Tue, Apr 17, 2018 at 10:32:16AM +0200, Tomas Härdin wrote: > > tis 2018-04-17 klockan 02:13 +0200 skrev Michael Niedermayer: > > > Speeds up decoding from 8 to 3 seconds for 6302/clusterfuzz- > > > testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer- > > > 5626371985375232 > > > Fixes: Timeout > > > > > > Found-by: continuous fuzzing process https://github.com/google/os > > > s-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > --- > > > libavcodec/cinepak.c | 27 ++++++++++++++++++++++----- > > > 1 file changed, 22 insertions(+), 5 deletions(-) > > > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > > index 89e940ae0d..ba0589582f 100644 > > > --- a/libavcodec/cinepak.c > > > +++ b/libavcodec/cinepak.c > > > @@ -315,14 +315,11 @@ static int cinepak_decode_strip > > > (CinepakContext *s, > > > return AVERROR_INVALIDDATA; > > > } > > > > > > -static int cinepak_decode (CinepakContext *s) > > > +static int cinepak_predecode_check (CinepakContext *s) > > > { > > > - const uint8_t *eod = (s->data + s->size); > > > - int i, result, strip_size, frame_flags, > > > num_strips; > > > - int y0 = 0; > > > + int num_strips; > > > int encoded_buf_size; > > > > > > - frame_flags = s->data[0]; > > > num_strips = AV_RB16 (&s->data[8]); > > > encoded_buf_size = AV_RB24(&s->data[1]); > > > > > > @@ -353,6 +350,21 @@ static int cinepak_decode (CinepakContext > > > *s) > > > s->sega_film_skip_bytes = 0; > > > } > > > > > > + if (s->size < 10 + s->sega_film_skip_bytes + num_strips * > > > 12) > > > + return AVERROR_INVALIDDATA; > > > > Looks like an extra check, not just moving existing checks as the > > commit message implies > > its from this: > for (i=0; i < num_strips; i++) { > if ((s->data + 12) > eod) > return AVERROR_INVALIDDATA; > ... > > But i can split this in a separte patch if you prefer ? Nah, I think this discussion being archived should be enough /Tomas
On Wed, Apr 18, 2018 at 09:07:20PM +0200, Tomas Härdin wrote: > tis 2018-04-17 klockan 14:24 +0200 skrev Michael Niedermayer: > > On Tue, Apr 17, 2018 at 10:32:16AM +0200, Tomas Härdin wrote: > > > tis 2018-04-17 klockan 02:13 +0200 skrev Michael Niedermayer: > > > > Speeds up decoding from 8 to 3 seconds for 6302/clusterfuzz- > > > > testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer- > > > > 5626371985375232 > > > > Fixes: Timeout > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/os > > > > s-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > > --- > > > > libavcodec/cinepak.c | 27 ++++++++++++++++++++++----- > > > > 1 file changed, 22 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > > > index 89e940ae0d..ba0589582f 100644 > > > > --- a/libavcodec/cinepak.c > > > > +++ b/libavcodec/cinepak.c > > > > @@ -315,14 +315,11 @@ static int cinepak_decode_strip > > > > (CinepakContext *s, > > > > return AVERROR_INVALIDDATA; > > > > } > > > > > > > > -static int cinepak_decode (CinepakContext *s) > > > > +static int cinepak_predecode_check (CinepakContext *s) > > > > { > > > > - const uint8_t *eod = (s->data + s->size); > > > > - int i, result, strip_size, frame_flags, > > > > num_strips; > > > > - int y0 = 0; > > > > + int num_strips; > > > > int encoded_buf_size; > > > > > > > > - frame_flags = s->data[0]; > > > > num_strips = AV_RB16 (&s->data[8]); > > > > encoded_buf_size = AV_RB24(&s->data[1]); > > > > > > > > @@ -353,6 +350,21 @@ static int cinepak_decode (CinepakContext > > > > *s) > > > > s->sega_film_skip_bytes = 0; > > > > } > > > > > > > > + if (s->size < 10 + s->sega_film_skip_bytes + num_strips * > > > > 12) > > > > + return AVERROR_INVALIDDATA; > > > > > > Looks like an extra check, not just moving existing checks as the > > > commit message implies > > > > its from this: > > for (i=0; i < num_strips; i++) { > > if ((s->data + 12) > eod) > > return AVERROR_INVALIDDATA; > > ... > > > > But i can split this in a separte patch if you prefer ? > > Nah, I think this discussion being archived should be enough will apply thx [...]
diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c index 89e940ae0d..ba0589582f 100644 --- a/libavcodec/cinepak.c +++ b/libavcodec/cinepak.c @@ -315,14 +315,11 @@ static int cinepak_decode_strip (CinepakContext *s, return AVERROR_INVALIDDATA; } -static int cinepak_decode (CinepakContext *s) +static int cinepak_predecode_check (CinepakContext *s) { - const uint8_t *eod = (s->data + s->size); - int i, result, strip_size, frame_flags, num_strips; - int y0 = 0; + int num_strips; int encoded_buf_size; - frame_flags = s->data[0]; num_strips = AV_RB16 (&s->data[8]); encoded_buf_size = AV_RB24(&s->data[1]); @@ -353,6 +350,21 @@ static int cinepak_decode (CinepakContext *s) s->sega_film_skip_bytes = 0; } + if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) + return AVERROR_INVALIDDATA; + + return 0; +} + +static int cinepak_decode (CinepakContext *s) +{ + const uint8_t *eod = (s->data + s->size); + int i, result, strip_size, frame_flags, num_strips; + int y0 = 0; + + frame_flags = s->data[0]; + num_strips = AV_RB16 (&s->data[8]); + s->data += 10 + s->sega_film_skip_bytes; num_strips = FFMIN(num_strips, MAX_STRIPS); @@ -439,6 +451,11 @@ static int cinepak_decode_frame(AVCodecContext *avctx, if (s->size < 10) return AVERROR_INVALIDDATA; + if ((ret = cinepak_predecode_check(s)) < 0) { + av_log(avctx, AV_LOG_ERROR, "cinepak_predecode_check failed\n"); + return ret; + } + if ((ret = ff_reget_buffer(avctx, s->frame)) < 0) return ret;
Speeds up decoding from 8 to 3 seconds for 6302/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5626371985375232 Fixes: Timeout Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cinepak.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)