[FFmpeg-devel] [PATCH 1/3] avformat/kvag: Check sample_rate

[FFmpeg-devel] [PATCH 1/3] avformat/kvag: Check sample_rate

[Michael Niedermayer]

Fixes: Division by 0
Fixes: -copyts -start_at_zero -itsoffset 00:00:01 -itsscale 1 -ss 00:00:02  -i zgclab/ffmpeg_crash/poc1 output.mp4

Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/kvag.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/kvag.c b/libavformat/kvag.c
index 1d0aee09942..b55aa893ec2 100644
--- a/libavformat/kvag.c
+++ b/libavformat/kvag.c
@@ -38,7 +38,7 @@
 typedef struct KVAGHeader {
     uint32_t    magic;
     uint32_t    data_size;
-    uint32_t    sample_rate;
+    int    sample_rate;
     uint16_t    stereo;
 } KVAGHeader;
 
@@ -70,6 +70,9 @@ static int kvag_read_header(AVFormatContext *s)
     hdr.sample_rate             = AV_RL32(buf +  8);
     hdr.stereo                  = AV_RL16(buf + 12);
 
+    if (hdr.sample_rate <= 0)
+        return AVERROR_INVALIDDATA;
+
     par                         = st->codecpar;
     par->codec_type             = AVMEDIA_TYPE_AUDIO;
     par->codec_id               = AV_CODEC_ID_ADPCM_IMA_SSI;
-- 
2.43.2

_______________________________________________
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] [PATCH 2/3] avformat/concatdec: Check file

[Michael Niedermayer]

Fixes: null pointer dereference
Fixes: -stream_loop 1 -ss 00:00:05 -i zgclab/ffmpeg_crash/poc2 -codec:v copy -codec:a aac -y output.mp4

Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/concatdec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 493659649cf..fe65d0c7688 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -639,6 +639,11 @@ static int concat_parse_script(AVFormatContext *avf)
         }
     }
 
+    if (!file) {
+        ret = AVERROR_INVALIDDATA;
+        goto fail;
+    }
+
     if (file->inpoint != AV_NOPTS_VALUE && file->outpoint != AV_NOPTS_VALUE) {
         if (file->inpoint  > file->outpoint ||
             file->outpoint - (uint64_t)file->inpoint > INT64_MAX)
-- 
2.43.2

_______________________________________________
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 2/3] avformat/concatdec: Check file

[Michael Niedermayer]

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

On Tue, Apr 30, 2024 at 02:48:53AM +0200, Michael Niedermayer wrote:
> Fixes: null pointer dereference
> Fixes: -stream_loop 1 -ss 00:00:05 -i zgclab/ffmpeg_crash/poc2 -codec:v copy -codec:a aac -y output.mp4
> 
> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/concatdec.c | 5 +++++
>  1 file changed, 5 insertions(+)

will apply

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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

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

[FFmpeg-devel] [PATCH 3/3] avutil/opt: Preserve nb_channels in opt_free

[Michael Niedermayer]

Fixes: division by 0
Fixes: decoder modifying demuxer channels on failure
Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3

Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavutil/opt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libavutil/opt.c b/libavutil/opt.c
index ecbf7efe5fb..24c08e4bc06 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr)
         av_dict_free((AVDictionary **)ptr);
         break;
 
-    case AV_OPT_TYPE_CHLAYOUT:
+    case AV_OPT_TYPE_CHLAYOUT: {
+        int nb_channels = ((AVChannelLayout *)ptr)->nb_channels;
         av_channel_layout_uninit((AVChannelLayout *)ptr);
-        break;
+        ((AVChannelLayout *)ptr)->nb_channels = nb_channels;
+        break;}
 
     default:
         break;
-- 
2.43.2

_______________________________________________
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 3/3] avutil/opt: Preserve nb_channels in opt_free

[James Almer]

On 4/29/2024 9:48 PM, Michael Niedermayer wrote:
> Fixes: division by 0
> Fixes: decoder modifying demuxer channels on failure
> Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3
> 
> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavutil/opt.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index ecbf7efe5fb..24c08e4bc06 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr)
>           av_dict_free((AVDictionary **)ptr);
>           break;
>   
> -    case AV_OPT_TYPE_CHLAYOUT:
> +    case AV_OPT_TYPE_CHLAYOUT: {
> +        int nb_channels = ((AVChannelLayout *)ptr)->nb_channels;
>           av_channel_layout_uninit((AVChannelLayout *)ptr);
> -        break;
> +        ((AVChannelLayout *)ptr)->nb_channels = nb_channels;
> +        break;}
>   
>       default:
>           break;

A little bit of context would be helpful here. What's using nb_channels 
after av_opt_free was called and where?
_______________________________________________
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 3/3] avutil/opt: Preserve nb_channels in opt_free

[Michael Niedermayer]

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

On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote:
> On 4/29/2024 9:48 PM, Michael Niedermayer wrote:
> > Fixes: division by 0
> > Fixes: decoder modifying demuxer channels on failure
> > Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3
> > 
> > Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavutil/opt.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavutil/opt.c b/libavutil/opt.c
> > index ecbf7efe5fb..24c08e4bc06 100644
> > --- a/libavutil/opt.c
> > +++ b/libavutil/opt.c
> > @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr)
> >           av_dict_free((AVDictionary **)ptr);
> >           break;
> > -    case AV_OPT_TYPE_CHLAYOUT:
> > +    case AV_OPT_TYPE_CHLAYOUT: {
> > +        int nb_channels = ((AVChannelLayout *)ptr)->nb_channels;
> >           av_channel_layout_uninit((AVChannelLayout *)ptr);
> > -        break;
> > +        ((AVChannelLayout *)ptr)->nb_channels = nb_channels;
> > +        break;}
> >       default:
> >           break;
> 
> A little bit of context would be helpful here. What's using nb_channels
> after av_opt_free was called and where?

demuxer sets nb_channels
find stream info copies codec params to context
find stream info tries opening decoder
decoder, refuses, and opt_free_elem() is called on cleanup
context now has 0 channels
context gets copied into params of demuxer
demuxer goes like i have set the channels to a non zero value let me devide by them
and oops

there is more than one position in this chain of events this can be fixed

thx

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

Observe your enemies, for they first find out your faults. -- Antisthenes

[-- 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 3/3] avutil/opt: Preserve nb_channels in opt_free

[James Almer]

On 4/30/2024 9:38 PM, Michael Niedermayer wrote:
> On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote:
>> On 4/29/2024 9:48 PM, Michael Niedermayer wrote:
>>> Fixes: division by 0
>>> Fixes: decoder modifying demuxer channels on failure
>>> Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3
>>>
>>> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavutil/opt.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index ecbf7efe5fb..24c08e4bc06 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr)
>>>            av_dict_free((AVDictionary **)ptr);
>>>            break;
>>> -    case AV_OPT_TYPE_CHLAYOUT:
>>> +    case AV_OPT_TYPE_CHLAYOUT: {
>>> +        int nb_channels = ((AVChannelLayout *)ptr)->nb_channels;
>>>            av_channel_layout_uninit((AVChannelLayout *)ptr);
>>> -        break;
>>> +        ((AVChannelLayout *)ptr)->nb_channels = nb_channels;
>>> +        break;}
>>>        default:
>>>            break;
>>
>> A little bit of context would be helpful here. What's using nb_channels
>> after av_opt_free was called and where?
> 
> demuxer sets nb_channels
> find stream info copies codec params to context
> find stream info tries opening decoder
> decoder, refuses, and opt_free_elem() is called on cleanup
> context now has 0 channels
> context gets copied into params of demuxer

Should this happen if the internal decoder failed to generate a frame? 
Can it be avoided?

> demuxer goes like i have set the channels to a non zero value let me devide by them
> and oops
> 
> there is more than one position in this chain of events this can be fixed
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 3/3] avutil/opt: Preserve nb_channels in opt_free

[James Almer]

On 4/30/2024 9:38 PM, Michael Niedermayer wrote:
> On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote:
>> On 4/29/2024 9:48 PM, Michael Niedermayer wrote:
>>> Fixes: division by 0
>>> Fixes: decoder modifying demuxer channels on failure
>>> Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3
>>>
>>> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavutil/opt.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index ecbf7efe5fb..24c08e4bc06 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr)
>>>            av_dict_free((AVDictionary **)ptr);
>>>            break;
>>> -    case AV_OPT_TYPE_CHLAYOUT:
>>> +    case AV_OPT_TYPE_CHLAYOUT: {
>>> +        int nb_channels = ((AVChannelLayout *)ptr)->nb_channels;
>>>            av_channel_layout_uninit((AVChannelLayout *)ptr);
>>> -        break;
>>> +        ((AVChannelLayout *)ptr)->nb_channels = nb_channels;
>>> +        break;}
>>>        default:
>>>            break;
>>
>> A little bit of context would be helpful here. What's using nb_channels
>> after av_opt_free was called and where?
> 
> demuxer sets nb_channels
> find stream info copies codec params to context
> find stream info tries opening decoder
> decoder, refuses, and opt_free_elem() is called on cleanup
> context now has 0 channels
> context gets copied into params of demuxer
> demuxer goes like i have set the channels to a non zero value let me devide by them
> and oops
> 
> there is more than one position in this chain of events this can be fixed
> 
> thx

I think we should prevent avcodec_open2() from clearing a user-set 
parameter when closing, rather than change how av_opt_free works.

Like so:

> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index 888dd76228..fc8a40e4db 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub)
> 
>  av_cold void ff_codec_close(AVCodecContext *avctx)
>  {
> +    AVChannelLayout ch_layout;
>      int i;
> 
>      if (!avctx)
> @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
> 
>      if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
>          av_opt_free(avctx->priv_data);
> +
> +    // Work around av_opt_free() unsetting ch_layout
> +    ch_layout = avctx->ch_layout;
> +    memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout));
>      av_opt_free(avctx);
> +    avctx->ch_layout = ch_layout;
> +
>      av_freep(&avctx->priv_data);
>      if (av_codec_is_encoder(avctx->codec)) {
>          av_freep(&avctx->extradata);
_______________________________________________
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 1/3] avformat/kvag: Check sample_rate

[Michael Niedermayer]

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

On Tue, Apr 30, 2024 at 02:48:52AM +0200, Michael Niedermayer wrote:
> Fixes: Division by 0
> Fixes: -copyts -start_at_zero -itsoffset 00:00:01 -itsscale 1 -ss 00:00:02  -i zgclab/ffmpeg_crash/poc1 output.mp4
> 
> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/kvag.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

I will apply this one
sample rate is signed everywhere
so accepting things outside the signed int range is not
going to work


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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato

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