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

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

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.

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

Setting ret there and breaking the loop was wrong, as the scope of ret 
doesn't reach outside loop.

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

[...]