[FFmpeg-devel] [PATCH] avformat/vpk: fix divide by zero

Sat Nov 2 08:22:46 EET 2024

On Sat, 2 Nov 2024 at 01:54, Michael Niedermayer <michael at niedermayer.cc> wrote:
>
> Hi
>
> On Fri, Nov 01, 2024 at 02:20:33PM +0100, Kacper Michajlow wrote:
> > On Sat, 10 Aug 2024 at 18:49, Kacper Michajlow <kasper93 at gmail.com> wrote:
> > >
> > > On Sat, 10 Aug 2024 at 11:25, Andreas Rheinhardt
> > > <andreas.rheinhardt at outlook.com> wrote:
> > > >
> > > > Kacper Michajlow:
> > > > > On Fri, 9 Aug 2024 at 22:51, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > >>
> > > > >> On Wed, Aug 07, 2024 at 03:42:46PM +0200, Kacper Michajłow wrote:
> > > > >>> Can happen after calling avformat_find_stream_info() when the codec
> > > > >>> fails to open, but return value is 0 and subsequent uses of this context
> > > > >>> have zero value in channel number.
> > > > >>>
> > > > >>> Found by OSS-Fuzz.
> > > > >>>
> > > > >>> Signed-off-by: Kacper Michajłow <kasper93 at gmail.com>
> > > > >>> ---
> > > > >>>  libavformat/vpk.c | 2 ++
> > > > >>>  1 file changed, 2 insertions(+)
> > > > >>>
> > > > >>> diff --git a/libavformat/vpk.c b/libavformat/vpk.c
> > > > >>> index 001ad33555..aa98ef2dd4 100644
> > > > >>> --- a/libavformat/vpk.c
> > > > >>> +++ b/libavformat/vpk.c
> > > > >>> @@ -86,6 +86,8 @@ static int vpk_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > >>>
> > > > >>>      vpk->current_block++;
> > > > >>>      if (vpk->current_block == vpk->block_count) {
> > > > >>> +        if (par->ch_layout.nb_channels <= 0)
> > > > >>> +            return AVERROR_INVALIDDATA;
> > > > >>>          unsigned size = vpk->last_block_size / par->ch_layout.nb_channels;
> > > > >>>          unsigned skip = (par->block_align - vpk->last_block_size) / par->ch_layout.nb_channels;
> > > > >>>          uint64_t pos = avio_tell(s->pb);
> > > > >>
> > > > >> iam not sure if a parser or other should replace a valid set of
> > > > >> parameters by an invalid
> > > > >> (this patch implies that such a action occured)
> > > > >>
> > > > >> can you explain more detailedly by what and why channels is set to 0 ?
> > > > >>
> > > > >
> > > > > You are right, it might be better to improve this to not override the
> > > > > params. Let me explain what happens, I didn't read through the whole
> > > > > avformat_find_stream_info() to know what would be the best approach
> > > > > yet. I will try to look at it, but if you have immediate ideas, that
> > > > > would be nice.
> > > > >
> > > > > 1. avformat_open_input() sets nb_channels to 108
> > > > >
> > > > > 2. Just after that we call avformat_find_stream_info(avfc, NULL); this
> > > > > returns 0 (success), but as a result it overrides params already
> > > > > present in the context.
> > > > > log for reference, during the find stream info call
> > > > > [ffmpeg/demuxer] vpk: Before avformat_find_stream_info() pos:
> > > > > 538976288 bytes read:21 seeks:1 nb_streams:1
> > > > > [ffmpeg/demuxer] vpk: Failed to open codec in avformat_find_stream_info
> > > > > [lavf] mp_seek(0x512000018090, 0, size)
> > > > > [lavf] 0=mp_read(0x512000018090, 0x7fe4c7ce8800, 50000000), pos:
> > > > > 538976288, eof:1
> > > > > [lavf] 0=mp_read(0x512000018090, 0x52d00000a400, 32768), pos: 538976288, eof:1
> > > > > [ffmpeg/audio] adpcm_psx: Decoder requires channel layout to be set
> > > > > [ffmpeg/demuxer] vpk: Failed to open codec in avformat_find_stream_info
> > > > > [lavf] mp_seek(0x512000018090, 0, size)
> > > > > [lavf] mp_seek(0x512000018090, 0, size)
> > > > > [lavf] mp_seek(0x512000018090, 0, size)
> > > > > [ffmpeg/demuxer] vpk: stream 0: start_time: NOPTS duration: 0.069852
> > > > > [ffmpeg/demuxer] vpk: format: start_time: NOPTS duration: 0.069852
> > > > > (estimate from stream) bitrate=2 kb/s
> > > > > [ffmpeg/demuxer] vpk: Could not find codec parameters for stream 0
> > > > > (Audio: adpcm_psx, 538976288 Hz, 0 channels): unspecified sample
> > > > > format
> > > > > [ffmpeg/demuxer] Consider increasing the value for the
> > > > > 'analyzeduration' (0) and 'probesize' (5000000) options
> > > > > [ffmpeg/demuxer] vpk: After avformat_find_stream_info() pos: 538976288
> > > > > bytes read:21 seeks:1 frames:0
> > > > >
> > > > > 3. the nb_channels value is cleared in avformat_find_stream_info() ->
> > > > > avcodec_parameters_from_context() -> codec_parameters_reset() and
> > > > > remains 0.
> > > >
> > > > This seems like the error: Why is AVCodecParameters being set from an
> > > > AVCodecContext if the codec could not be successfully opened?
> > >
> > > avcodec_open2() is only emitting a warning, no other action taken.
> > > https://github.com/FFmpeg/FFmpeg/blob/1b8d95da3a4a5c9441238928a36b653da693c286/libavformat/demux.c#L2603-L2605
> > >
> > > later "some" things happen, but I think we could check if we have
> > > codec params before assigning them
> > >
> > > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > > index dc65f9ad91..e8785304ca 100644
> > > --- a/libavformat/demux.c
> > > +++ b/libavformat/demux.c
> > > @@ -3038,7 +3038,7 @@ int avformat_find_stream_info(AVFormatContext
> > > *ic, AVDictionary **options)
> > > AVStream *const st = ic->streams[i];
> > > FFStream *const sti = ffstream(st);
> > >
> > > - if (sti->avctx_inited) {
> > > + if (sti->avctx_inited && has_codec_parameters(sti, NULL)) {
> > > ret = avcodec_parameters_from_context(st->codecpar, sti->avctx);
> > > if (ret < 0)
> > > goto find_stream_info_err;
> > >
> > > But the question is if we want to preserve old values in case of
> > > failed open or clear those values to indicate that really there are no
> > > valid parameters, because we failed to open the codec with current
> > > ones.
> > >
> > > - Kacper
> [...]
> > FYI, this bug has been disclosed after reaching deadline
> > https://issues.oss-fuzz.com/issues/42536474
>
> does this replicate with ffmpeg ?
> It seems not with this:
> -i clusterfuzz-testcase-minimized-fuzzer_loadfile-5130628287234048  -f null -

Not directly. Like mentioned in the previous emails. You have to call
avformat_find_stream_info() to trigger this bug. Apply
avformat_find_stream_info.diff and it should reproduce with ffmpeg.c
too.

Like said before, I don't know what is expected behaviour of
`avformat_find_stream_info()`, currently it seems to override codec
params, settings number of channels to 0, which later causes issues.
While it doesn't return an error. Note that the error itself is not in
`avformat_find_stream_info()`, but later on. As a workaround, see
has_codec_parameters.diff this prevents overriding codec params in
context.

Let me know if you need more info.

Thanks,
Kacper
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avformat_find_stream_info.diff
Type: application/octet-stream
Size: 635 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241102/e54ad11c/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: has_codec_parameters.diff
Type: application/octet-stream
Size: 585 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241102/e54ad11c/attachment-0001.obj>