[FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps

[James Almer]

On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
>> On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
>>> Fixes: Timeout
>>> Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>    libavformat/cafdec.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
>>> index 426c56b9bd..334077efb5 100644
>>> --- a/libavformat/cafdec.c
>>> +++ b/libavformat/cafdec.c
>>> @@ -33,6 +33,7 @@
>>>    #include "isom.h"
>>>    #include "mov_chan.h"
>>>    #include "libavcodec/flac.h"
>>> +#include "libavcodec/internal.h"
>>>    #include "libavutil/intreadwrite.h"
>>>    #include "libavutil/intfloat.h"
>>>    #include "libavutil/dict.h"
>>> @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
>>>        st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
>>>        st->codecpar->bits_per_coded_sample = avio_rb32(pb);
>>> +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
>>> +        st->codecpar->bits_per_coded_sample > 64)
>>
>> Where does the process take so long that oss-fuzz gets a timeout if these
>> are unreasonably high? I don't see nb_channels used anywhere in here where
>> that matters.
>> Is it in the PCM decoder? Because that decoder is meant to handle any
>> arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
>> is set to is not ok.
> 
> libavutil/channel_layout.c:627:17
> or it will OOM before depending on how much memory is available

Does this fix the timeout?

> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> index 2d6963b6df..a4fcd199e1 100644
> --- a/libavutil/channel_layout.c
> +++ b/libavutil/channel_layout.c
> @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
>          for (i = 0; i < channel_layout->nb_channels; i++) {
>              enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
> 
> +            if (!av_bprint_is_complete(bp))
> +                break;
>              if (i)
>                  av_bprintf(bp, "+");
>              av_channel_name_bprint(bp, ch);

But this is not ok as it will affect the buffer len value 
av_channel_layout_describe() returns on success when truncation took 
place, so something else would have to be done.

> 
> 
>>
>> And is the bits_per_coded_sample > 64 check to prevent codec_id being
>> AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
>> AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
>> we don't currently support.
> 
> We generally check read values for validity at the earliest,
> bits_per_coded_sample of more than 64 seem not valid to me.

I agree the check is fine, but my point is that, assuming this is 
affecting demuxing time, this check as is may be hiding an issue related 
to codec_id being set to AV_CODEC_ID_NONE here (the result of 
ff_get_pcm_codec_id() with an unsupported bits_per_coded_sample value), 
so it should be looked at more closely because said codec_id could 
happen with valid files using codecs the demuxer does not know about.

If it does not affect demuxing time and is a "just in case" check, then 
it doesn't belong in a patch that says "Fixes: Timeout" and mentions an 
ossfuzz issue.

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