On Tue, Sep 28, 2021 at 8:09 PM James Almer <jamr...@gmail.com> wrote:
> On 9/28/2021 3:03 PM, Michael Niedermayer wrote: > > On Tue, Sep 28, 2021 at 09:44:01PM +1000, Peter Ross wrote: > >> On Tue, Sep 28, 2021 at 12:07:49AM -0300, James Almer wrote: > >>> On 9/27/2021 7:37 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 > >>>> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> > >>>> --- > >>>> libavcodec/siren.c | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/libavcodec/siren.c b/libavcodec/siren.c > >>>> index 7f2b4678608..708990d6654 100644 > >>>> --- a/libavcodec/siren.c > >>>> +++ b/libavcodec/siren.c > >>>> @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, > void *data, > >>>> if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) > >>>> return ret; > >>>> + if (s->sample_rate_bits + 4 > get_bits_left(gb)) > >>>> + return AVERROR_INVALIDDATA; > >>> > >>> This is not enough. You'll inevitably get another timeout in the future > >>> unless you add a get_bits_left(gb) > 0 condition to the loop in > >>> decode_envelope(). > >>> And there should be at least four bits left after decode_envelope() > returns, > >>> so check for that too. > > > > I tend not to add more checks than needed for timeouts because > > it increases the risk the patch gets rejected because it breaks > something and > > then the reporter refuses to share any details. Or someone is offended > because > > too many ugly/hacky timeout checks are added. So my patches are a bit > > minimalistic (less work and less friction). > > But that now offends others who want more complete fixes :( > > > > added a check in the loop, ill post the new patch > > Nothing you did offended me. I'm just saying that your patch fixes the > timeout for that specific bitstream created by the fuzzer, but the core > issue is in that loop, and it'd be better to fix it there instead. > > If you don't want to add the check after decode_envelope() then that's ok. > I prefer James solution, instead of heuristic nonsense. > > > > > > >> > >> suggest increasing the threshold to include: > >> > > > >> s->sample_rate_bits + s->number_of_regions + 4 + s->checksum_bits > > get_bits_left(gb) > > > > changed > > > > > >> > >> > >> hey michael are you only fuzzing AV_CODEC_ID_MSNSIREN? > >> i would expect this issue to occur for for the vanilla SIREN codec too? > > > > The fuzzer is supposed to fuzz all decoders, > > sometimes it has difficulty with some decoders, when for example theres > no > > seed file or something else special is needed for the codec. For example > > a codec may need a specific codec_tag or extradata set ... > > > > 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-requ...@ffmpeg.org with subject "unsubscribe". > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
