| Message ID | 20240807134246.1559-1-kasper93@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/vpk: fix divide by zero | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| yinshiyou/make_loongarch64 | warning | New warnings during build |
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@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 ? thx [...]
On Fri, 9 Aug 2024 at 22:51, Michael Niedermayer <michael@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@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. 4. as we can see there were errors, but it still returns success, so we proceed. 5. on the next av_seek_frame() which goes to vpk_read_packet() it will fail because nb_channels is 0 at this point. Sorry for only a high level overview, but at this moment, I'm not sure how exactly it is supposed to work. I thought it might be intended to override headers parameters later if we know better later on, that's why my initial patch only tackled this case. - Kacper
Kacper Michajlow: > On Fri, 9 Aug 2024 at 22:51, Michael Niedermayer <michael@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@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? > > 4. as we can see there were errors, but it still returns success, so we proceed. > > 5. on the next av_seek_frame() which goes to vpk_read_packet() it will > fail because nb_channels is 0 at this point. > > Sorry for only a high level overview, but at this moment, I'm not sure > how exactly it is supposed to work. I thought it might be intended to > override headers parameters later if we know better later on, that's > why my initial patch only tackled this case. >
On Sat, 10 Aug 2024 at 11:25, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Kacper Michajlow: > > On Fri, 9 Aug 2024 at 22:51, Michael Niedermayer <michael@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@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 > > > > 4. as we can see there were errors, but it still returns success, so we proceed. > > > > 5. on the next av_seek_frame() which goes to vpk_read_packet() it will > > fail because nb_channels is 0 at this point. > > > > Sorry for only a high level overview, but at this moment, I'm not sure > > how exactly it is supposed to work. I thought it might be intended to > > override headers parameters later if we know better later on, that's > > why my initial patch only tackled this case. > > > > _______________________________________________ > 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".
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);
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@gmail.com> --- libavformat/vpk.c | 2 ++ 1 file changed, 2 insertions(+)