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

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

[Kacper Michajłow]

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);
-- 
2.43.0

_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1546 bytes --]

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

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

Whats the most studid thing your enemy could do ? Blow himself up
Whats the most studid thing you could do ? Give up your rights and
freedom because your enemy blew himself up.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[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.

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
_______________________________________________
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".

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

[Andreas Rheinhardt]

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.
> 

_______________________________________________
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".

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

[Kacper Michajlow]

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".
_______________________________________________
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".