Re: [FFmpeg-devel] [PATCH 2/4] avformat/iamf_reader: return REDO on failure to read

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Wed, Mar 20, 2024 at 11:22:11PM -0300, James Almer wrote:
> On 3/20/2024 10:15 PM, Michael Niedermayer wrote:
> > Fixes: null pointer derference
> > Fixes: 67007/clusterfuzz-testcase-minimized-ffmpeg_dem_IAMF_fuzzer-6522819204677632
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/iamf_reader.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/iamf_reader.c b/libavformat/iamf_reader.c
> > index 42d20f1ae6..a06aa98cdb 100644
> > --- a/libavformat/iamf_reader.c
> > +++ b/libavformat/iamf_reader.c
> > @@ -26,6 +26,7 @@
> >   #include "libavcodec/packet.h"
> >   #include "avformat.h"
> >   #include "avio_internal.h"
> > +#include "demux.h"
> >   #include "iamf.h"
> >   #include "iamf_parse.h"
> >   #include "iamf_reader.h"
> > @@ -322,7 +323,7 @@ int ff_iamf_read_packet(AVFormatContext *s, IAMFDemuxContext *c,
> >               break;
> >       }
> > -    return read;
> > +    return FFERROR_REDO;
> 
> Where is the null pointer dereference happening? I don't particularly like
> this approach because ff_iamf_read_packet() is also called by the mov
> demuxer.

==8458==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000703a8b bp 0x7ffc691161f0 sp 0x7ffc69116000 T0)
==8458==The signal is caused by a READ memory access.
==8458==Hint: address points to the zero page.
    #0 0x703a8a in ff_read_packet ffmpeg/libavformat/demux.c:671:15
    #1 0x7074cc in read_frame_internal ffmpeg/libavformat/demux.c:1346:15
    #2 0x704a07 in av_read_frame ffmpeg/libavformat/demux.c:1550:17
    #3 0x4c844f in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:211:15


	SCARINESS: 10 (null-deref)
    #0 0x60b7cc in ff_read_packet /src/ffmpeg/libavformat/demux.c:683:15
    #1 0x60cbcb in read_frame_internal /src/ffmpeg/libavformat/demux.c:1358:15
    #2 0x60c59b in av_read_frame /src/ffmpeg/libavformat/demux.c:1569:17
    #3 0x57808a in LLVMFuzzerTestOneInput /src/ffmpeg/tools/target_dem_fuzzer.c:211:15
    #4 0x449183 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #5 0x4348e2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
    #6 0x43a18c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:860:9
    #7 0x4636c2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #8 0x7854642cc082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
    #9 0x42aaad in _start

Either way the iamf demuxer returns no packet but the caller believes it
returns a packet. WHen that gets used things crash


> 
> >   }
> >   void ff_iamf_read_deinit(IAMFDemuxContext *c)
> 
> Does the following also help?
> 
> > diff --git a/libavformat/iamf_reader.c b/libavformat/iamf_reader.c
> > index 42d20f1ae6..4e79691a03 100644
> > --- a/libavformat/iamf_reader.c
> > +++ b/libavformat/iamf_reader.c
> > @@ -311,8 +311,7 @@ int ff_iamf_read_packet(AVFormatContext *s, IAMFDemuxContext *c,
> >          } else {
> >              int64_t offset = avio_skip(pb, obu_size);
> >              if (offset < 0) {
> > -                ret = offset;
> > -                break;
> > +                return offset;
> >              }
> >          }
> >          max_size -= len;

yes, this seems to fix it too and is better

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