On Sat, Nov 02, 2024 at 07:22:46AM +0100, Kacper Michajlow wrote:
> On Sat, 2 Nov 2024 at 01:54, Michael Niedermayer <mich...@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 <kaspe...@gmail.com> wrote:
> > > >
> > > > On Sat, 10 Aug 2024 at 11:25, Andreas Rheinhardt
> > > > <andreas.rheinha...@outlook.com> wrote:
> > > > >
> > > > > Kacper Michajlow:
> > > > > > On Fri, 9 Aug 2024 at 22:51, Michael Niedermayer
> > > > > > <mich...@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 <kaspe...@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.Here we have AV_CODEC_ID_ADPCM_PSX which has no parser and no need_parsing set and its values are set by the demuxers header reading please correct me if iam wrong but So we seem to have a case where the only thing that ever sets the parameters is the demuxers header reading. (there are many other cases where this is true) its not the decoder setting this so avformat_find_stream_info() would not help in setting its values. But it resets them that reset seems wrong as it corrupts demuxer set values that are not ever going to be set from somewhere else and the demuxer correctly doesnt expect its non zero value to become zero I dont think avformat_find_stream_info() should be allowed to corrupt demuxer set values when it fails opening a codec that isnt even able to set these values. This differs from cases like mp3 where we expect the codec / parser to set values above is my feeling about this, i didnt have enough time ATM to really look into this again thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
