| Message ID | 20210928222835.29462-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 109113b54f6fa7b880f66c797f7ab49d65dc7167 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/siren: Check available bits at frame start | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 9/28/2021 7:28 PM, Michael Niedermayer wrote: > Fixes: Timeout > Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Suggested-by: James Almer <jamrial@gmail.com> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/siren.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/siren.c b/libavcodec/siren.c > index 7f084172a4c..8c9628e2d8e 100644 > --- a/libavcodec/siren.c > +++ b/libavcodec/siren.c > @@ -447,6 +447,8 @@ static int decode_envelope(SirenContext *s, GetBitContext *gb, > int index = 0; > > do { > + if (get_bits_left(gb) < 4) If the idea is that there will be enough bits to read rate_control after this function returns, then this should also take into account the get_bits1() below. > + return AVERROR_INVALIDDATA; > index = differential_decoder_tree[i - 1][index][get_bits1(gb)]; > } while (index > 0); > > @@ -727,9 +729,11 @@ static int siren_decode(AVCodecContext *avctx, void *data, > > skip_bits(gb, s->sample_rate_bits); > > - decode_envelope(s, gb, s->number_of_regions, > + ret = decode_envelope(s, gb, s->number_of_regions, > s->decoder_standard_deviation, > s->absolute_region_power_index, s->esf_adjustment); > + if (ret < 0) > + return ret; > > rate_control = get_bits(gb, 4); > >
On Tue, Sep 28, 2021 at 10:19:48PM -0300, James Almer wrote: > On 9/28/2021 7:28 PM, Michael Niedermayer wrote: > > Fixes: Timeout > > Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Suggested-by: James Almer <jamrial@gmail.com> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/siren.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/siren.c b/libavcodec/siren.c > > index 7f084172a4c..8c9628e2d8e 100644 > > --- a/libavcodec/siren.c > > +++ b/libavcodec/siren.c > > @@ -447,6 +447,8 @@ static int decode_envelope(SirenContext *s, GetBitContext *gb, > > int index = 0; > > do { > > + if (get_bits_left(gb) < 4) > > If the idea is that there will be enough bits to read rate_control after > this function returns, then this should also take into account the > get_bits1() below. Do you suggest that this is merged with 3/3 or do you suggest something else ? thx [...]
On 9/29/2021 12:31 PM, Michael Niedermayer wrote: > On Tue, Sep 28, 2021 at 10:19:48PM -0300, James Almer wrote: >> On 9/28/2021 7:28 PM, Michael Niedermayer wrote: >>> Fixes: Timeout >>> Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Suggested-by: James Almer <jamrial@gmail.com> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/siren.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/siren.c b/libavcodec/siren.c >>> index 7f084172a4c..8c9628e2d8e 100644 >>> --- a/libavcodec/siren.c >>> +++ b/libavcodec/siren.c >>> @@ -447,6 +447,8 @@ static int decode_envelope(SirenContext *s, GetBitContext *gb, >>> int index = 0; >>> do { >>> + if (get_bits_left(gb) < 4) >> >> If the idea is that there will be enough bits to read rate_control after >> this function returns, then this should also take into account the >> get_bits1() below. > > Do you suggest that this is merged with 3/3 or do you suggest something else ? I was thinking making this a < 5 check, so it's guaranteed there's one for index inside this do while loop plus four for rate_control after returning from this function, but i hadn't looked at the 3/3 patch when i wrote that, and now i see it actually adds a consideration for that. LGTM then. And no opinion about squashing them. Do as you prefer. > > 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". >
On Wed, Sep 29, 2021 at 12:54:02PM -0300, James Almer wrote: > On 9/29/2021 12:31 PM, Michael Niedermayer wrote: > > On Tue, Sep 28, 2021 at 10:19:48PM -0300, James Almer wrote: > > > On 9/28/2021 7:28 PM, Michael Niedermayer wrote: > > > > Fixes: Timeout > > > > Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Suggested-by: James Almer <jamrial@gmail.com> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/siren.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/siren.c b/libavcodec/siren.c > > > > index 7f084172a4c..8c9628e2d8e 100644 > > > > --- a/libavcodec/siren.c > > > > +++ b/libavcodec/siren.c > > > > @@ -447,6 +447,8 @@ static int decode_envelope(SirenContext *s, GetBitContext *gb, > > > > int index = 0; > > > > do { > > > > + if (get_bits_left(gb) < 4) > > > > > > If the idea is that there will be enough bits to read rate_control after > > > this function returns, then this should also take into account the > > > get_bits1() below. > > > > Do you suggest that this is merged with 3/3 or do you suggest something else ? > > I was thinking making this a < 5 check, so it's guaranteed there's one for > index inside this do while loop plus four for rate_control after returning > from this function, but i hadn't looked at the 3/3 patch when i wrote that, > and now i see it actually adds a consideration for that. > > LGTM then. And no opinion about squashing them. Do as you prefer. will apply seperatly thx [...]
diff --git a/libavcodec/siren.c b/libavcodec/siren.c index 7f084172a4c..8c9628e2d8e 100644 --- a/libavcodec/siren.c +++ b/libavcodec/siren.c @@ -447,6 +447,8 @@ static int decode_envelope(SirenContext *s, GetBitContext *gb, int index = 0; do { + if (get_bits_left(gb) < 4) + return AVERROR_INVALIDDATA; index = differential_decoder_tree[i - 1][index][get_bits1(gb)]; } while (index > 0); @@ -727,9 +729,11 @@ static int siren_decode(AVCodecContext *avctx, void *data, skip_bits(gb, s->sample_rate_bits); - decode_envelope(s, gb, s->number_of_regions, + ret = decode_envelope(s, gb, s->number_of_regions, s->decoder_standard_deviation, s->absolute_region_power_index, s->esf_adjustment); + if (ret < 0) + return ret; rate_control = get_bits(gb, 4);
Fixes: Timeout Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Suggested-by: James Almer <jamrial@gmail.com> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/siren.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)