[FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps

[FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps

[Michael Niedermayer]

Fixes: Timeout
Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/cafdec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
index 426c56b9bd..334077efb5 100644
--- a/libavformat/cafdec.c
+++ b/libavformat/cafdec.c
@@ -33,6 +33,7 @@
 #include "isom.h"
 #include "mov_chan.h"
 #include "libavcodec/flac.h"
+#include "libavcodec/internal.h"
 #include "libavutil/intreadwrite.h"
 #include "libavutil/intfloat.h"
 #include "libavutil/dict.h"
@@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
     st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
     st->codecpar->bits_per_coded_sample = avio_rb32(pb);
 
+    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
+        st->codecpar->bits_per_coded_sample > 64)
+        return AVERROR_INVALIDDATA;
+
     if (caf->bytes_per_packet < 0 || caf->frames_per_packet < 0 || st->codecpar->ch_layout.nb_channels < 0)
         return AVERROR_INVALIDDATA;
 
-- 
2.17.1

_______________________________________________
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/mov: Do not deallocate heif_item in a input dependant way

[Michael Niedermayer]

Fixes: out of array access
Fixes: 67070/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5685384082161664

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mov.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index f954b924a0..a87ce5cefe 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -8077,7 +8077,7 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     }
     item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
 
-    heif_item = av_realloc_array(c->heif_item, item_count, sizeof(*c->heif_item));
+    heif_item = av_realloc_array(c->heif_item, FFMAX(item_count, c->nb_heif_item), sizeof(*c->heif_item));
     if (!heif_item)
         return AVERROR(ENOMEM);
     c->heif_item = heif_item;
@@ -8202,7 +8202,7 @@ static int mov_read_iinf(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     avio_rb24(pb);  // flags.
     entry_count = version ? avio_rb32(pb) : avio_rb16(pb);
 
-    heif_item = av_realloc_array(c->heif_item, entry_count, sizeof(*c->heif_item));
+    heif_item = av_realloc_array(c->heif_item, FFMAX(entry_count, c->nb_heif_item), sizeof(*c->heif_item));
     if (!heif_item)
         return AVERROR(ENOMEM);
     c->heif_item = heif_item;
-- 
2.17.1

_______________________________________________
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/mov: Do not deallocate heif_item in a input dependant way

[James Almer]

On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> Fixes: out of array access
> Fixes: 67070/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5685384082161664
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/mov.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index f954b924a0..a87ce5cefe 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -8077,7 +8077,7 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>       }
>       item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
>   
> -    heif_item = av_realloc_array(c->heif_item, item_count, sizeof(*c->heif_item));
> +    heif_item = av_realloc_array(c->heif_item, FFMAX(item_count, c->nb_heif_item), sizeof(*c->heif_item));
>       if (!heif_item)
>           return AVERROR(ENOMEM);
>       c->heif_item = heif_item;
> @@ -8202,7 +8202,7 @@ static int mov_read_iinf(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>       avio_rb24(pb);  // flags.
>       entry_count = version ? avio_rb32(pb) : avio_rb16(pb);
>   
> -    heif_item = av_realloc_array(c->heif_item, entry_count, sizeof(*c->heif_item));
> +    heif_item = av_realloc_array(c->heif_item, FFMAX(entry_count, c->nb_heif_item), sizeof(*c->heif_item));
>       if (!heif_item)
>           return AVERROR(ENOMEM);
>       c->heif_item = heif_item;

LGTM.
_______________________________________________
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/mov: Do not deallocate heif_item in a input dependant way

[Michael Niedermayer]

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

On Mon, Mar 25, 2024 at 10:22:19PM -0300, James Almer wrote:
> On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > Fixes: out of array access
> > Fixes: 67070/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5685384082161664
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/mov.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index f954b924a0..a87ce5cefe 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -8077,7 +8077,7 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >       }
> >       item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
> > -    heif_item = av_realloc_array(c->heif_item, item_count, sizeof(*c->heif_item));
> > +    heif_item = av_realloc_array(c->heif_item, FFMAX(item_count, c->nb_heif_item), sizeof(*c->heif_item));
> >       if (!heif_item)
> >           return AVERROR(ENOMEM);
> >       c->heif_item = heif_item;
> > @@ -8202,7 +8202,7 @@ static int mov_read_iinf(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >       avio_rb24(pb);  // flags.
> >       entry_count = version ? avio_rb32(pb) : avio_rb16(pb);
> > -    heif_item = av_realloc_array(c->heif_item, entry_count, sizeof(*c->heif_item));
> > +    heif_item = av_realloc_array(c->heif_item, FFMAX(entry_count, c->nb_heif_item), sizeof(*c->heif_item));
> >       if (!heif_item)
> >           return AVERROR(ENOMEM);
> >       c->heif_item = heif_item;
> 
> LGTM.

will apply

thx

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

[-- 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] avformat/aiffdec: Check for previously set channels

[Michael Niedermayer]

Fixes: out of array access (av_channel_layout_copy())
Fixes: 67087/clusterfuzz-testcase-minimized-ffmpeg_dem_AIFF_fuzzer-4920720268263424

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/aiffdec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
index 9318943f96..fc01ffcbf1 100644
--- a/libavformat/aiffdec.c
+++ b/libavformat/aiffdec.c
@@ -106,6 +106,8 @@ static int get_aiff_header(AVFormatContext *s, int64_t size,
         size++;
     par->codec_type = AVMEDIA_TYPE_AUDIO;
     channels = avio_rb16(pb);
+    if (par->ch_layout.nb_channels && par->ch_layout.nb_channels != channels)
+        return AVERROR_INVALIDDATA;
     par->ch_layout.nb_channels = channels;
     num_frames = avio_rb32(pb);
     par->bits_per_coded_sample = avio_rb16(pb);
-- 
2.17.1

_______________________________________________
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] avformat/aiffdec: Check for previously set channels

[Michael Niedermayer]

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

On Sat, Mar 23, 2024 at 12:08:18AM +0100, Michael Niedermayer wrote:
> Fixes: out of array access (av_channel_layout_copy())
> Fixes: 67087/clusterfuzz-testcase-minimized-ffmpeg_dem_AIFF_fuzzer-4920720268263424
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/aiffdec.c | 2 ++
>  1 file changed, 2 insertions(+)

will apply

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

No great genius has ever existed without some touch of madness. -- Aristotle

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[Anton Khirnov]

Quoting Michael Niedermayer (2024-03-23 00:08:16)
> Fixes: Timeout
> Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/cafdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> index 426c56b9bd..334077efb5 100644
> --- a/libavformat/cafdec.c
> +++ b/libavformat/cafdec.c
> @@ -33,6 +33,7 @@
>  #include "isom.h"
>  #include "mov_chan.h"
>  #include "libavcodec/flac.h"
> +#include "libavcodec/internal.h"
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/intfloat.h"
>  #include "libavutil/dict.h"
> @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
>      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
>      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
>  
> +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||

I dislike this.

-- 
Anton Khirnov
_______________________________________________
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/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > Fixes: Timeout
> > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/cafdec.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > index 426c56b9bd..334077efb5 100644
> > --- a/libavformat/cafdec.c
> > +++ b/libavformat/cafdec.c
> > @@ -33,6 +33,7 @@
> >  #include "isom.h"
> >  #include "mov_chan.h"
> >  #include "libavcodec/flac.h"
> > +#include "libavcodec/internal.h"
> >  #include "libavutil/intreadwrite.h"
> >  #include "libavutil/intfloat.h"
> >  #include "libavutil/dict.h"
> > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> >  
> > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> 
> I dislike this.

I dislike it too

thx

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > Fixes: Timeout
> > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/cafdec.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > index 426c56b9bd..334077efb5 100644
> > > --- a/libavformat/cafdec.c
> > > +++ b/libavformat/cafdec.c
> > > @@ -33,6 +33,7 @@
> > >  #include "isom.h"
> > >  #include "mov_chan.h"
> > >  #include "libavcodec/flac.h"
> > > +#include "libavcodec/internal.h"
> > >  #include "libavutil/intreadwrite.h"
> > >  #include "libavutil/intfloat.h"
> > >  #include "libavutil/dict.h"
> > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > >  
> > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > 
> > I dislike this.
> 
> I dislike it too

so what do we do about this ?
any objections to apply this ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[Anton Khirnov]

Quoting Michael Niedermayer (2024-06-25 21:25:46)
> On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > Fixes: Timeout
> > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/cafdec.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > index 426c56b9bd..334077efb5 100644
> > > > --- a/libavformat/cafdec.c
> > > > +++ b/libavformat/cafdec.c
> > > > @@ -33,6 +33,7 @@
> > > >  #include "isom.h"
> > > >  #include "mov_chan.h"
> > > >  #include "libavcodec/flac.h"
> > > > +#include "libavcodec/internal.h"
> > > >  #include "libavutil/intreadwrite.h"
> > > >  #include "libavutil/intfloat.h"
> > > >  #include "libavutil/dict.h"
> > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > >  
> > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > 
> > > I dislike this.
> > 
> > I dislike it too
> 
> so what do we do about this ?

About what? What is the actual problem that needs addressed?

> any objections to apply this ?

yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread

-- 
Anton Khirnov
_______________________________________________
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/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-06-25 21:25:46)
> > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > > Fixes: Timeout
> > > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > 
> > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >  libavformat/cafdec.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > index 426c56b9bd..334077efb5 100644
> > > > > --- a/libavformat/cafdec.c
> > > > > +++ b/libavformat/cafdec.c
> > > > > @@ -33,6 +33,7 @@
> > > > >  #include "isom.h"
> > > > >  #include "mov_chan.h"
> > > > >  #include "libavcodec/flac.h"
> > > > > +#include "libavcodec/internal.h"
> > > > >  #include "libavutil/intreadwrite.h"
> > > > >  #include "libavutil/intfloat.h"
> > > > >  #include "libavutil/dict.h"
> > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > >  
> > > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > 
> > > > I dislike this.
> > > 
> > > I dislike it too
> > 
> > so what do we do about this ?
> 
> About what? What is the actual problem that needs addressed?

67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328


> 
> > any objections to apply this ?
> 
> yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread

a maximum number for each theoretically unlimited parameter is desirable
This can be a user setable value or a compile time value when such is preferred.

We can take this to the TC if you want.

The same way as the number of files or number of bytes used by some cache
needs limits, so do channels, and number of pixels.

One can remove them but users and companies concious about security and
efficiency with untrusted input in an (semi-) automated environment will likely
choose the codebase providing such features.

thx

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

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[Paul B Mahol]

On Thu, Jun 27, 2024 at 1:50 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-06-25 21:25:46)
> > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > > > Fixes: Timeout
> > > > > > Fixes:
> 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > >
> > > > > > Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > >  libavformat/cafdec.c | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > --- a/libavformat/cafdec.c
> > > > > > +++ b/libavformat/cafdec.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >  #include "isom.h"
> > > > > >  #include "mov_chan.h"
> > > > > >  #include "libavcodec/flac.h"
> > > > > > +#include "libavcodec/internal.h"
> > > > > >  #include "libavutil/intreadwrite.h"
> > > > > >  #include "libavutil/intfloat.h"
> > > > > >  #include "libavutil/dict.h"
> > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > >
> > > > > > +    if (st->codecpar->ch_layout.nb_channels >
> FF_SANE_NB_CHANNELS ||
> > > > >
> > > > > I dislike this.
> > > >
> > > > I dislike it too
> > >
> > > so what do we do about this ?
> >
> > About what? What is the actual problem that needs addressed?
>
> 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
>
>
> >
> > > any objections to apply this ?
> >
> > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread
>
> a maximum number for each theoretically unlimited parameter is desirable
> This can be a user setable value or a compile time value when such is
> preferred.
>

Cant you check if so many channels are actually available in stream?
Or there is some silly loop that goes unchecked up to INT32_MAX ?


>
> We can take this to the TC if you want.
>
> The same way as the number of files or number of bytes used by some cache
> needs limits, so do channels, and number of pixels.
>
> One can remove them but users and companies concious about security and
> efficiency with untrusted input in an (semi-) automated environment will
> likely
> choose the codebase providing such features.
>

"Sure."


>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The day soldiers stop bringing you their problems is the day you have
> stopped
> leading them. They have either lost confidence that you can help or
> concluded
> you do not care. Either case is a failure of leadership. - Colin Powell
> _______________________________________________
> 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 1/3] avformat/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Thu, Jun 27, 2024 at 08:40:00AM +0200, Paul B Mahol wrote:
> On Thu, Jun 27, 2024 at 1:50 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2024-06-25 21:25:46)
> > > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > > > > Fixes: Timeout
> > > > > > > Fixes:
> > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > > >
> > > > > > > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > > ---
> > > > > > >  libavformat/cafdec.c | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > > --- a/libavformat/cafdec.c
> > > > > > > +++ b/libavformat/cafdec.c
> > > > > > > @@ -33,6 +33,7 @@
> > > > > > >  #include "isom.h"
> > > > > > >  #include "mov_chan.h"
> > > > > > >  #include "libavcodec/flac.h"
> > > > > > > +#include "libavcodec/internal.h"
> > > > > > >  #include "libavutil/intreadwrite.h"
> > > > > > >  #include "libavutil/intfloat.h"
> > > > > > >  #include "libavutil/dict.h"
> > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > > >
> > > > > > > +    if (st->codecpar->ch_layout.nb_channels >
> > FF_SANE_NB_CHANNELS ||
> > > > > >
> > > > > > I dislike this.
> > > > >
> > > > > I dislike it too
> > > >
> > > > so what do we do about this ?
> > >
> > > About what? What is the actual problem that needs addressed?
> >
> > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> >
> >
> > >
> > > > any objections to apply this ?
> > >
> > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread
> >
> > a maximum number for each theoretically unlimited parameter is desirable
> > This can be a user setable value or a compile time value when such is
> > preferred.
> >
> 
> Cant you check if so many channels are actually available in stream?

Iam not sure how. Maybe there is a way ...


> Or there is some silly loop that goes unchecked up to INT32_MAX ?

there are multiple loops
one is in
av_channel_layout_custom_init()

another is in av_channel_layout_describe_bprint()

thx

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[Anton Khirnov]

Quoting Michael Niedermayer (2024-06-27 01:50:12)
> On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-06-25 21:25:46)
> > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > > > Fixes: Timeout
> > > > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > > 
> > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > >  libavformat/cafdec.c | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > --- a/libavformat/cafdec.c
> > > > > > +++ b/libavformat/cafdec.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >  #include "isom.h"
> > > > > >  #include "mov_chan.h"
> > > > > >  #include "libavcodec/flac.h"
> > > > > > +#include "libavcodec/internal.h"
> > > > > >  #include "libavutil/intreadwrite.h"
> > > > > >  #include "libavutil/intfloat.h"
> > > > > >  #include "libavutil/dict.h"
> > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > >  
> > > > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > > 
> > > > > I dislike this.
> > > > 
> > > > I dislike it too
> > > 
> > > so what do we do about this ?
> > 
> > About what? What is the actual problem that needs addressed?
> 
> 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> 
> 
> > 
> > > any objections to apply this ?
> > 
> > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread
> 
> a maximum number for each theoretically unlimited parameter is desirable

I disagree.

> This can be a user setable value or a compile time value when such is preferred.
> 
> We can take this to the TC if you want.
> 
> The same way as the number of files or number of bytes used by some cache
> needs limits,

Those limits are implemented at the OS level, not by individual
programs. And they are runtime-configurable.

> so do channels, and number of pixels.

Does not follow.

> One can remove them but users and companies concious about security and
> efficiency with untrusted input in an (semi-) automated environment will likely
> choose the codebase providing such features.

[citation needed]

My main objection to this approach is that you're addressing a symptom
(fuzzer timeout) rather than the actual problem (some code scaling
inappropriately with input size), and it's completely unclear where that
actual problem even is, as cafdec does not seem to be doing anything
with the channel count. That suggests the problem is really in some
other code and you're just papering over it.

-- 
Anton Khirnov
_______________________________________________
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/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Thu, Jun 27, 2024 at 08:53:09AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-06-27 01:50:12)
> > On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2024-06-25 21:25:46)
> > > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > > > > Fixes: Timeout
> > > > > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > > > 
> > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > > ---
> > > > > > >  libavformat/cafdec.c | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > > --- a/libavformat/cafdec.c
> > > > > > > +++ b/libavformat/cafdec.c
> > > > > > > @@ -33,6 +33,7 @@
> > > > > > >  #include "isom.h"
> > > > > > >  #include "mov_chan.h"
> > > > > > >  #include "libavcodec/flac.h"
> > > > > > > +#include "libavcodec/internal.h"
> > > > > > >  #include "libavutil/intreadwrite.h"
> > > > > > >  #include "libavutil/intfloat.h"
> > > > > > >  #include "libavutil/dict.h"
> > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > > >  
> > > > > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > > > 
> > > > > > I dislike this.
> > > > > 
> > > > > I dislike it too
> > > > 
> > > > so what do we do about this ?
> > > 
> > > About what? What is the actual problem that needs addressed?
> > 
> > 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > 
> > 
> > > 
> > > > any objections to apply this ?
> > > 
> > > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread
> > 
> > a maximum number for each theoretically unlimited parameter is desirable
> 
> I disagree.
> 

> > This can be a user setable value or a compile time value when such is preferred.
> > 
> > We can take this to the TC if you want.
> > 
> > The same way as the number of files or number of bytes used by some cache
> > needs limits,
> 
> Those limits are implemented at the OS level, not by individual
> programs.

random pick, man ccache --max-files=N --max-size=SIZE


> And they are runtime-configurable.

I would favor runtime-configurable as well


> 
> > so do channels, and number of pixels.
> 
> Does not follow.

For a file with length n "bits" it is understood and expected that there will be O(n)
processing needed. having O(2^n) is not "expected"
if you have a loop going over channels, pixels, or another "unlimited" property
you can create very small files that require the maximum memory and processing
allowed by the OS for the process

Thats a DOS attack


> 
> > One can remove them but users and companies concious about security and
> > efficiency with untrusted input in an (semi-) automated environment will likely
> > choose the codebase providing such features.
> 
> [citation needed]

> 
> My main objection to this approach is that you're addressing a symptom
> (fuzzer timeout) rather than the actual problem (some code scaling
> inappropriately with input size), and it's completely unclear where that
> actual problem even is, as cafdec does not seem to be doing anything
> with the channel count. That suggests the problem is really in some
> other code and you're just papering over it.

ive rerun the testcase and depending on settings it either hits
OOM in av_channel_layout_custom_init() or it will timeout in

av_channel_layout_describe_bprint libavutil/channel_layout.c:627:17

The file has 33 million channels.
Now if for some reason none of the above would hit a timeout and
instead the decoder would decode 33 million channels of silence
maybe because it had a 1 byte packet and error concealment produced
a silent packet to fill the space. Still the same issue exists
only a limit on the channel number avoids this.

Surely improving all the code that is slow with millions of channels
is a good idea but even the most efficient code to produce a frame with
arbitrary # million channels in it will lead to a DOS attack opertunity

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 1/3] avformat/cafdec: sanity check channels and bps

[James Almer]

On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> Fixes: Timeout
> Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/cafdec.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> index 426c56b9bd..334077efb5 100644
> --- a/libavformat/cafdec.c
> +++ b/libavformat/cafdec.c
> @@ -33,6 +33,7 @@
>   #include "isom.h"
>   #include "mov_chan.h"
>   #include "libavcodec/flac.h"
> +#include "libavcodec/internal.h"
>   #include "libavutil/intreadwrite.h"
>   #include "libavutil/intfloat.h"
>   #include "libavutil/dict.h"
> @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
>       st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
>       st->codecpar->bits_per_coded_sample = avio_rb32(pb);
>   
> +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> +        st->codecpar->bits_per_coded_sample > 64)

Where does the process take so long that oss-fuzz gets a timeout if 
these are unreasonably high? I don't see nb_channels used anywhere in 
here where that matters.
Is it in the PCM decoder? Because that decoder is meant to handle any 
arbitrary amount of channels, so limiting it to whatever 
FF_SANE_NB_CHANNELS is set to is not ok.

And is the bits_per_coded_sample > 64 check to prevent codec_id being 
AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
AV_CODEC_ID_NONE for that matter could happen for valid files with a 
codec we don't currently support.

> +        return AVERROR_INVALIDDATA;
> +
>       if (caf->bytes_per_packet < 0 || caf->frames_per_packet < 0 || st->codecpar->ch_layout.nb_channels < 0)
>           return AVERROR_INVALIDDATA;
>   
_______________________________________________
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/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
> On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > Fixes: Timeout
> > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/cafdec.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > index 426c56b9bd..334077efb5 100644
> > --- a/libavformat/cafdec.c
> > +++ b/libavformat/cafdec.c
> > @@ -33,6 +33,7 @@
> >   #include "isom.h"
> >   #include "mov_chan.h"
> >   #include "libavcodec/flac.h"
> > +#include "libavcodec/internal.h"
> >   #include "libavutil/intreadwrite.h"
> >   #include "libavutil/intfloat.h"
> >   #include "libavutil/dict.h"
> > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> >       st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> >       st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > +        st->codecpar->bits_per_coded_sample > 64)
> 
> Where does the process take so long that oss-fuzz gets a timeout if these
> are unreasonably high? I don't see nb_channels used anywhere in here where
> that matters.
> Is it in the PCM decoder? Because that decoder is meant to handle any
> arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
> is set to is not ok.

libavutil/channel_layout.c:627:17
or it will OOM before depending on how much memory is available


> 
> And is the bits_per_coded_sample > 64 check to prevent codec_id being
> AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
> AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
> we don't currently support.

We generally check read values for validity at the earliest,
bits_per_coded_sample of more than 64 seem not valid to me.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[James Almer]

On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
>> On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
>>> Fixes: Timeout
>>> Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavformat/cafdec.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
>>> index 426c56b9bd..334077efb5 100644
>>> --- a/libavformat/cafdec.c
>>> +++ b/libavformat/cafdec.c
>>> @@ -33,6 +33,7 @@
>>>    #include "isom.h"
>>>    #include "mov_chan.h"
>>>    #include "libavcodec/flac.h"
>>> +#include "libavcodec/internal.h"
>>>    #include "libavutil/intreadwrite.h"
>>>    #include "libavutil/intfloat.h"
>>>    #include "libavutil/dict.h"
>>> @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
>>>        st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
>>>        st->codecpar->bits_per_coded_sample = avio_rb32(pb);
>>> +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
>>> +        st->codecpar->bits_per_coded_sample > 64)
>>
>> Where does the process take so long that oss-fuzz gets a timeout if these
>> are unreasonably high? I don't see nb_channels used anywhere in here where
>> that matters.
>> Is it in the PCM decoder? Because that decoder is meant to handle any
>> arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
>> is set to is not ok.
> 
> libavutil/channel_layout.c:627:17
> or it will OOM before depending on how much memory is available

Does this fix the timeout?

> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> index 2d6963b6df..a4fcd199e1 100644
> --- a/libavutil/channel_layout.c
> +++ b/libavutil/channel_layout.c
> @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
>          for (i = 0; i < channel_layout->nb_channels; i++) {
>              enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
> 
> +            if (!av_bprint_is_complete(bp))
> +                break;
>              if (i)
>                  av_bprintf(bp, "+");
>              av_channel_name_bprint(bp, ch);

But this is not ok as it will affect the buffer len value 
av_channel_layout_describe() returns on success when truncation took 
place, so something else would have to be done.

> 
> 
>>
>> And is the bits_per_coded_sample > 64 check to prevent codec_id being
>> AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
>> AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
>> we don't currently support.
> 
> We generally check read values for validity at the earliest,
> bits_per_coded_sample of more than 64 seem not valid to me.

I agree the check is fine, but my point is that, assuming this is 
affecting demuxing time, this check as is may be hiding an issue related 
to codec_id being set to AV_CODEC_ID_NONE here (the result of 
ff_get_pcm_codec_id() with an unsupported bits_per_coded_sample value), 
so it should be looked at more closely because said codec_id could 
happen with valid files using codecs the demuxer does not know about.

If it does not affect demuxing time and is a "just in case" check, then 
it doesn't belong in a patch that says "Fixes: Timeout" and mentions an 
ossfuzz issue.

> 
> 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 1/3] avformat/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
> On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> > On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
> > > On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > > > Fixes: Timeout
> > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >    libavformat/cafdec.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > index 426c56b9bd..334077efb5 100644
> > > > --- a/libavformat/cafdec.c
> > > > +++ b/libavformat/cafdec.c
> > > > @@ -33,6 +33,7 @@
> > > >    #include "isom.h"
> > > >    #include "mov_chan.h"
> > > >    #include "libavcodec/flac.h"
> > > > +#include "libavcodec/internal.h"
> > > >    #include "libavutil/intreadwrite.h"
> > > >    #include "libavutil/intfloat.h"
> > > >    #include "libavutil/dict.h"
> > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > >        st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > >        st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > +        st->codecpar->bits_per_coded_sample > 64)
> > > 
> > > Where does the process take so long that oss-fuzz gets a timeout if these
> > > are unreasonably high? I don't see nb_channels used anywhere in here where
> > > that matters.
> > > Is it in the PCM decoder? Because that decoder is meant to handle any
> > > arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
> > > is set to is not ok.
> > 
> > libavutil/channel_layout.c:627:17
> > or it will OOM before depending on how much memory is available
> 
> Does this fix the timeout?
> 
> > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> > index 2d6963b6df..a4fcd199e1 100644
> > --- a/libavutil/channel_layout.c
> > +++ b/libavutil/channel_layout.c
> > @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
> >          for (i = 0; i < channel_layout->nb_channels; i++) {
> >              enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
> > 
> > +            if (!av_bprint_is_complete(bp))
> > +                break;
> >              if (i)
> >                  av_bprintf(bp, "+");
> >              av_channel_name_bprint(bp, ch);
> 
> But this is not ok as it will affect the buffer len value
> av_channel_layout_describe() returns on success when truncation took place,
> so something else would have to be done.

This makes the testcase which is 108 bytes long take a bit more than 7 seconds
which is below the threshold for the timeout, but i would guess 30x on the channel
number would bring it well above

The next location it gets stuck on if the timeout threshold is reduced:

    #0 0x4a5b41 in __sanitizer_print_stack_trace /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3
    #1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback() Fuzzer/build/../FuzzerLoop.cpp:248:7
    #2 0x7fbe0815e41f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f)
    #3 0x7fbe07c73e08  (/lib/x86_64-linux-gnu/libc.so.6+0xbbe08)
    #4 0x49c747 in __asan_memcpy /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3
    #5 0x2873f59 in av_channel_layout_copy ffmpeg/libavutil/channel_layout.c:452:9
    #6 0x1895763 in avcodec_parameters_to_context ffmpeg/libavcodec/codec_par.c:235:15
    #7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:2579:15
    #8 0x4cd17b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:207:11
    #9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
    #10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
    #11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
    #12 0x2a263d0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
    #13 0x7fbe07bdc082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
    #14 0x42529d in _start (ffmpeg/tools/target_dem_caf_fuzzer+0x42529d)


> 
> > 
> > 
> > > 
> > > And is the bits_per_coded_sample > 64 check to prevent codec_id being
> > > AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
> > > AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
> > > we don't currently support.
> > 
> > We generally check read values for validity at the earliest,
> > bits_per_coded_sample of more than 64 seem not valid to me.
> 
> I agree the check is fine, but my point is that, assuming this is affecting
> demuxing time, this check as is may be hiding an issue related to codec_id
> being set to AV_CODEC_ID_NONE here (the result of ff_get_pcm_codec_id() with
> an unsupported bits_per_coded_sample value), so it should be looked at more
> closely because said codec_id could happen with valid files using codecs the
> demuxer does not know about.
> 

> If it does not affect demuxing time and is a "just in case" check, then it
> doesn't belong in a patch that says "Fixes: Timeout" and mentions an ossfuzz
> issue.

that is strictly true

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Any man who breaks a law that conscience tells him is unjust and willingly 
accepts the penalty by staying in jail in order to arouse the conscience of 
the community on the injustice of the law is at that moment expressing the 
very highest respect for law. - Martin Luther King Jr

[-- 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 1/3] avformat/cafdec: sanity check channels and bps

[James Almer]

On 7/1/2024 8:42 PM, Michael Niedermayer wrote:
> On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
>> On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
>>> On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
>>>> On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
>>>>> Fixes: Timeout
>>>>> Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>     libavformat/cafdec.c | 5 +++++
>>>>>     1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
>>>>> index 426c56b9bd..334077efb5 100644
>>>>> --- a/libavformat/cafdec.c
>>>>> +++ b/libavformat/cafdec.c
>>>>> @@ -33,6 +33,7 @@
>>>>>     #include "isom.h"
>>>>>     #include "mov_chan.h"
>>>>>     #include "libavcodec/flac.h"
>>>>> +#include "libavcodec/internal.h"
>>>>>     #include "libavutil/intreadwrite.h"
>>>>>     #include "libavutil/intfloat.h"
>>>>>     #include "libavutil/dict.h"
>>>>> @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
>>>>>         st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
>>>>>         st->codecpar->bits_per_coded_sample = avio_rb32(pb);
>>>>> +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
>>>>> +        st->codecpar->bits_per_coded_sample > 64)
>>>>
>>>> Where does the process take so long that oss-fuzz gets a timeout if these
>>>> are unreasonably high? I don't see nb_channels used anywhere in here where
>>>> that matters.
>>>> Is it in the PCM decoder? Because that decoder is meant to handle any
>>>> arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
>>>> is set to is not ok.
>>>
>>> libavutil/channel_layout.c:627:17
>>> or it will OOM before depending on how much memory is available
>>
>> Does this fix the timeout?
>>
>>> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
>>> index 2d6963b6df..a4fcd199e1 100644
>>> --- a/libavutil/channel_layout.c
>>> +++ b/libavutil/channel_layout.c
>>> @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
>>>           for (i = 0; i < channel_layout->nb_channels; i++) {
>>>               enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
>>>
>>> +            if (!av_bprint_is_complete(bp))
>>> +                break;
>>>               if (i)
>>>                   av_bprintf(bp, "+");
>>>               av_channel_name_bprint(bp, ch);
>>
>> But this is not ok as it will affect the buffer len value
>> av_channel_layout_describe() returns on success when truncation took place,
>> so something else would have to be done.
> 
> This makes the testcase which is 108 bytes long take a bit more than 7 seconds
> which is below the threshold for the timeout, but i would guess 30x on the channel
> number would bring it well above

If you try to process that file without the fuzzer, does it also take 7 
seconds before it stops? If not, then the fuzzer is having Valgrind 
levels of penalty hit, and i don't think adding dubious checks in our 
codebase just to appease it is correct.

> 
> The next location it gets stuck on if the timeout threshold is reduced:
> 
>      #0 0x4a5b41 in __sanitizer_print_stack_trace /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3
>      #1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback() Fuzzer/build/../FuzzerLoop.cpp:248:7
>      #2 0x7fbe0815e41f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f)
>      #3 0x7fbe07c73e08  (/lib/x86_64-linux-gnu/libc.so.6+0xbbe08)
>      #4 0x49c747 in __asan_memcpy /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3
>      #5 0x2873f59 in av_channel_layout_copy ffmpeg/libavutil/channel_layout.c:452:9
>      #6 0x1895763 in avcodec_parameters_to_context ffmpeg/libavcodec/codec_par.c:235:15
>      #7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:2579:15
>      #8 0x4cd17b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:207:11
>      #9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
>      #10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
>      #11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
>      #12 0x2a263d0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
>      #13 0x7fbe07bdc082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
>      #14 0x42529d in _start (ffmpeg/tools/target_dem_caf_fuzzer+0x42529d)
> 
> 
>>
>>>
>>>
>>>>
>>>> And is the bits_per_coded_sample > 64 check to prevent codec_id being
>>>> AV_CODEC_ID_NONE? if so, how does that affect demuxing time?
>>>> AV_CODEC_ID_NONE for that matter could happen for valid files with a codec
>>>> we don't currently support.
>>>
>>> We generally check read values for validity at the earliest,
>>> bits_per_coded_sample of more than 64 seem not valid to me.
>>
>> I agree the check is fine, but my point is that, assuming this is affecting
>> demuxing time, this check as is may be hiding an issue related to codec_id
>> being set to AV_CODEC_ID_NONE here (the result of ff_get_pcm_codec_id() with
>> an unsupported bits_per_coded_sample value), so it should be looked at more
>> closely because said codec_id could happen with valid files using codecs the
>> demuxer does not know about.
>>
> 
>> If it does not affect demuxing time and is a "just in case" check, then it
>> doesn't belong in a patch that says "Fixes: Timeout" and mentions an ossfuzz
>> issue.
> 
> that is strictly true
> 
> 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 1/3] avformat/cafdec: sanity check channels and bps

[Michael Niedermayer]

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

On Mon, Jul 01, 2024 at 09:01:28PM -0300, James Almer wrote:
> On 7/1/2024 8:42 PM, Michael Niedermayer wrote:
> > On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
> > > On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> > > > On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
> > > > > On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > > > > > Fixes: Timeout
> > > > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > > 
> > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > >     libavformat/cafdec.c | 5 +++++
> > > > > >     1 file changed, 5 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > --- a/libavformat/cafdec.c
> > > > > > +++ b/libavformat/cafdec.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >     #include "isom.h"
> > > > > >     #include "mov_chan.h"
> > > > > >     #include "libavcodec/flac.h"
> > > > > > +#include "libavcodec/internal.h"
> > > > > >     #include "libavutil/intreadwrite.h"
> > > > > >     #include "libavutil/intfloat.h"
> > > > > >     #include "libavutil/dict.h"
> > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > >         st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > >         st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > > > +        st->codecpar->bits_per_coded_sample > 64)
> > > > > 
> > > > > Where does the process take so long that oss-fuzz gets a timeout if these
> > > > > are unreasonably high? I don't see nb_channels used anywhere in here where
> > > > > that matters.
> > > > > Is it in the PCM decoder? Because that decoder is meant to handle any
> > > > > arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
> > > > > is set to is not ok.
> > > > 
> > > > libavutil/channel_layout.c:627:17
> > > > or it will OOM before depending on how much memory is available
> > > 
> > > Does this fix the timeout?
> > > 
> > > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> > > > index 2d6963b6df..a4fcd199e1 100644
> > > > --- a/libavutil/channel_layout.c
> > > > +++ b/libavutil/channel_layout.c
> > > > @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
> > > >           for (i = 0; i < channel_layout->nb_channels; i++) {
> > > >               enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
> > > > 
> > > > +            if (!av_bprint_is_complete(bp))
> > > > +                break;
> > > >               if (i)
> > > >                   av_bprintf(bp, "+");
> > > >               av_channel_name_bprint(bp, ch);
> > > 
> > > But this is not ok as it will affect the buffer len value
> > > av_channel_layout_describe() returns on success when truncation took place,
> > > so something else would have to be done.
> > 
> > This makes the testcase which is 108 bytes long take a bit more than 7 seconds
> > which is below the threshold for the timeout, but i would guess 30x on the channel
> > number would bring it well above
> 
> If you try to process that file without the fuzzer, does it also take 7
> seconds before it stops?

real	0m3.232s
user	0m1.064s
sys	0m2.156s

for a 108 byte file with your speed up patch, without that patch i didnt try as i have not
enough patience ATM

with 66 instead of 33 million channels

real	0m6.181s
user	0m1.774s
sys	0m4.385s


> If not, then the fuzzer is having Valgrind levels
> of penalty hit, and i don't think adding dubious checks in our codebase just
> to appease it is correct.

Please think again

The fuzzer is slower than non instrumented code, but its purpose is to detect issues

First i dont see how that leads to "dubious checks" or "appease" these are loaded
words not appropriate for a technical discusison

Second, if the fuzzer spends the majority of its time in a busy loop copying millions
of channels. Thats time it will not find any issues.
So both the fuzzing process itself as well as DOS of users are affected

Third if it goes over its threshold, for its purpose it found an issue.
If thats not the case the fuzzers configuration does not match the definition of a
DOS issue. Thats a problem in itself. (this isnt applying as the 7 sec is below)

Fourth, we should try to investigate and fix issues (truth we often dont have the
resources to fully investigate everything). Here the issue is clearly 33 million
channels being worked on as valid data. It would take time, alot of time to process each
sample times 33 million. The user does not have 33 million speakers and its not
reasonable for her computer to have to process files just to later reject them.
She should have a way to reject this earlier without the processing. This sample
also is no worst case at all. We can apply your speedup and wait for the fuzzer
to find a worse one, this may take time or might never happen. This doesnt
mean the worst case is 6 seconds for a 108 byte file. That said a web browser
opening 100 media files each 108 bytes and consuming 6 seconds cpu time
is already quite bad, also the memory requirement is bad too. This is a random
example maybe its not possible exactly like that, its more to show this issue
100 108 byte files is 10kb 10minutes of cpu time is alot for that, maybe a real
exploit would use a differnt demuxer or whatever. The issue iam having is that
Adding a check on channels is blocked on arguments that a specific testcase isnt
bad enough NOT on arguments that the WORST case would not be bad enough.
Thats like saying going 1 byte over the array but still being in an allocated
struct is not so bad disregarding what the maximum would be.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope

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