[FFmpeg-devel] [PATCH 2/2] avcodec/siren: Check available bits at frame start

[Paul B Mahol]

On Tue, Sep 28, 2021 at 8:09 PM James Almer <jamrial at 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 <michael at 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 at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>