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

Sat Nov 2 02:48:36 EET 2024

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 -

[vpk @ 0x11ac0680] Failed to open codec in avformat_find_stream_info
[adpcm_psx @ 0x11ad2ec0] Decoder requires channel layout to be set
[vpk @ 0x11ac0680] Failed to open codec in avformat_find_stream_info
[vpk @ 0x11ac0680] Could not find codec parameters for stream 0 (Audio: adpcm_psx, 538976288 Hz, 0 channels): unspecified sample format
Consider increasing the value for the 'analyzeduration' (0) and 'probesize' (5000000) options
Input #0, vpk, from 'clusterfuzz-testcase-minimized-fuzzer_loadfile-5130628287234048':
  Duration: 00:00:00.07, bitrate: 2 kb/s
  Stream #0:0: Audio: adpcm_psx, 538976288 Hz, 0 channels
Output #0, null, to 'pipe:':
[out#0/null @ 0x11ad5580] Output file does not contain any stream
Error opening output file -.
Error opening output files: Invalid argument

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241102/8903dcde/attachment.sig>