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

[James Almer]

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.

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