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

[Michael Niedermayer]

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


> 
> 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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210928/9573587f/attachment.sig>