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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 6064 bytes --]

On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
> 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@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.

This makes the testcase which is 108 bytes long take a bit more than 7 seconds
which is below the threshold for the timeout, but i would guess 30x on the channel
number would bring it well above

The next location it gets stuck on if the timeout threshold is reduced:

    #0 0x4a5b41 in __sanitizer_print_stack_trace /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3
    #1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback() Fuzzer/build/../FuzzerLoop.cpp:248:7
    #2 0x7fbe0815e41f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f)
    #3 0x7fbe07c73e08  (/lib/x86_64-linux-gnu/libc.so.6+0xbbe08)
    #4 0x49c747 in __asan_memcpy /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3
    #5 0x2873f59 in av_channel_layout_copy ffmpeg/libavutil/channel_layout.c:452:9
    #6 0x1895763 in avcodec_parameters_to_context ffmpeg/libavcodec/codec_par.c:235:15
    #7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:2579:15
    #8 0x4cd17b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:207:11
    #9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
    #10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
    #11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
    #12 0x2a263d0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
    #13 0x7fbe07bdc082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
    #14 0x42529d in _start (ffmpeg/tools/target_dem_caf_fuzzer+0x42529d)


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

that is strictly true

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Any man who breaks a law that conscience tells him is unjust and willingly 
accepts the penalty by staying in jail in order to arouse the conscience of 
the community on the injustice of the law is at that moment expressing the 
very highest respect for law. - Martin Luther King Jr

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".