[FFmpeg-devel] [RFC] Channels

[FFmpeg-devel] [RFC] Channels

[Michael Niedermayer]

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

Hi all

we have code like
st->codecpar->ch_layout.nb_channels = avio_rb32(pb);

and then somewhere there is some code that uses this by first allocating
an array and that then hits OOM
(it was this here:
    map = av_calloc(nb_channels, sizeof(*channel_layout->u.map));)

is anyone against adding a max_channels field to AVFormatContext  or something
like that ?

alternative is "wont fix" for all such cases, or maybe someone sees another way ?

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.

[-- 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] [RFC] Channels

[James Almer]

On 3/21/2024 11:25 PM, Michael Niedermayer wrote:
> Hi all
> 
> we have code like
> st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> 
> and then somewhere there is some code that uses this by first allocating
> an array and that then hits OOM
> (it was this here:
>      map = av_calloc(nb_channels, sizeof(*channel_layout->u.map));)
> 
> is anyone against adding a max_channels field to AVFormatContext  or something
> like that ?
> 
> alternative is "wont fix" for all such cases, or maybe someone sees another way ?
> 
> thx

We have FF_SANE_NB_CHANNELS, so maybe add a check for it to mxfdec.c 
(Where i assume this is happening) and mov_chan.c or mov.c before 
continuing with such a layout.

> 
> 
> _______________________________________________
> 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] [RFC] Channels

[Michael Niedermayer]

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

On Thu, Mar 21, 2024 at 11:59:17PM -0300, James Almer wrote:
> On 3/21/2024 11:25 PM, Michael Niedermayer wrote:
> > Hi all
> > 
> > we have code like
> > st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > 
> > and then somewhere there is some code that uses this by first allocating
> > an array and that then hits OOM
> > (it was this here:
> >      map = av_calloc(nb_channels, sizeof(*channel_layout->u.map));)
> > 
> > is anyone against adding a max_channels field to AVFormatContext  or something
> > like that ?
> > 
> > alternative is "wont fix" for all such cases, or maybe someone sees another way ?
> > 
> > thx
> 
> We have FF_SANE_NB_CHANNELS, so maybe add a check for it to mxfdec.c (Where
> i assume this is happening) and mov_chan.c or mov.c before continuing with
> such a layout.

wasnt mxf, ill send a patch

thanks!

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire

[-- 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] [RFC] Channels

[Anton Khirnov]

Quoting Michael Niedermayer (2024-03-22 03:25:25)
> Hi all
> 
> we have code like
> st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> 
> and then somewhere there is some code that uses this by first allocating
> an array and that then hits OOM
> (it was this here:
>     map = av_calloc(nb_channels, sizeof(*channel_layout->u.map));)
> 
> is anyone against adding a max_channels field to AVFormatContext  or something
> like that ?

I am.

> alternative is "wont fix" for all such cases,

IMO it's not, in general, a bug, so EWONTFIX is the appropriate
response. If the user does not want us to do arbitrarily large
allocation, they have the appropriate OS-level mechanisms (e.g. ulimit,
cgroups on Linux) or av_max_alloc().

-- 
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] [RFC] Channels

[Michael Niedermayer]

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

On Fri, Mar 22, 2024 at 11:29:31AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-03-22 03:25:25)
[...]
> > alternative is "wont fix" for all such cases,
> 
> IMO it's not, in general, a bug, so EWONTFIX is the appropriate
> response. If the user does not want us to do arbitrarily large
> allocation, they have the appropriate OS-level mechanisms (e.g. ulimit,
> cgroups on Linux) or av_max_alloc().

You misunderstand the issue.

the issue is coverage in the fuzzer

if your 32bit channel number is all allowed then in some decoders
and demuxers you will in 99.9% of the cases never go beyond the
channel processing code
because it will timeout or hit OOM

your suggestion of ulimits, cgroups and other limits dont help
We already have both time and space limits in the fuzzers

Below is simplifying things a bit

if 99.9% of the random 32bit channel numbers die in the channel
processing because of the current limit. Then making the limit
tighter will increase that percentage further.

If you want better coverage you need a channel limit that stops
us before a resource intensive channel processing loop

you can also write down a model of this problem in a more formal way
Ht as in time spend reading the header
Ct time spend processing each channel after the header
Cmax maximum number of channels that will continue execution after the header

you will see that a Cmax = 2^32 will never be able to do what s Cmax=512
can do no matter what external limits you apply

because if you set really high external limits than 99.9% of time will be
spend in the channel processing code because most of the time the channel
number will be very large and nothing will stop it so little time will be
spend for coverage afterwards

and OTOH if you set a medium outside memory/time limut then most channel
cases will hit that limit but run the full length of the time limut
here 99.9% of the cases will timeout and take ALOT of time leaving no
resources for coverage after the channel code

and if you set a realls small outside memory/time limit then maybe you
will quickly stop the channel code but now 99.999% of cases will timeout
in the channel loop and what remains will not have enough time left to
even execute all the code after the loop

So again if you want fuzzer coverage theres need for a channel limit of
some sort.

The alternative is to tell everyone that we will not fix this and then
have bad fuzzer coverage for some cases.

(what iam skiping here is that fuzzers and AI might be able to direct
 the fuzzing towards lower channel numbers. But ill leave that to you
 if you want to trust that)

thx

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

It is a danger to trust the dream we wish for rather than
the science we have, -- Dr. Kenneth Brown

[-- 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] [RFC] Channels

[Anton Khirnov]

Quoting Michael Niedermayer (2024-03-27 22:54:14)
> On Fri, Mar 22, 2024 at 11:29:31AM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-03-22 03:25:25)
> [...]
> > > alternative is "wont fix" for all such cases,
> > 
> > IMO it's not, in general, a bug, so EWONTFIX is the appropriate
> > response. If the user does not want us to do arbitrarily large
> > allocation, they have the appropriate OS-level mechanisms (e.g. ulimit,
> > cgroups on Linux) or av_max_alloc().
> 
> You misunderstand the issue.
> 
> the issue is coverage in the fuzzer
> 
> if your 32bit channel number is all allowed then in some decoders
> and demuxers you will in 99.9% of the cases never go beyond the
> channel processing code
> because it will timeout or hit OOM
> 
> your suggestion of ulimits, cgroups and other limits dont help
> We already have both time and space limits in the fuzzers
> 
> Below is simplifying things a bit
> 
> if 99.9% of the random 32bit channel numbers die in the channel
> processing because of the current limit. Then making the limit
> tighter will increase that percentage further.
> 
> If you want better coverage you need a channel limit that stops
> us before a resource intensive channel processing loop
> 
> you can also write down a model of this problem in a more formal way
> Ht as in time spend reading the header
> Ct time spend processing each channel after the header
> Cmax maximum number of channels that will continue execution after the header
> 
> you will see that a Cmax = 2^32 will never be able to do what s Cmax=512
> can do no matter what external limits you apply
> 
> because if you set really high external limits than 99.9% of time will be
> spend in the channel processing code because most of the time the channel
> number will be very large and nothing will stop it so little time will be
> spend for coverage afterwards
> 
> and OTOH if you set a medium outside memory/time limut then most channel
> cases will hit that limit but run the full length of the time limut
> here 99.9% of the cases will timeout and take ALOT of time leaving no
> resources for coverage after the channel code
> 
> and if you set a realls small outside memory/time limit then maybe you
> will quickly stop the channel code but now 99.999% of cases will timeout
> in the channel loop and what remains will not have enough time left to
> even execute all the code after the loop
> 
> So again if you want fuzzer coverage theres need for a channel limit of
> some sort.
> 
> The alternative is to tell everyone that we will not fix this and then
> have bad fuzzer coverage for some cases.

I understand that this is done for fuzzers, I just disagree that we
should introduce arbitrary limits to our code in order to appease them.
They should be tools for our benefit, not vice versa.

-- 
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] [RFC] Channels

[Tomas Härdin]

fre 2024-03-22 klockan 03:25 +0100 skrev Michael Niedermayer:
> Hi all
> 
> we have code like
> st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> 
> and then somewhere there is some code that uses this by first
> allocating
> an array and that then hits OOM
> (it was this here:
>     map = av_calloc(nb_channels, sizeof(*channel_layout->u.map));)
> 
> is anyone against adding a max_channels field to AVFormatContext  or
> something
> like that ?

Sounds reasonable, but also we have FF_SANE_NB_CHANNELS as James said.
But a more proper solution is to use formal methods rather than
fuzzers. Proofs beat fuzzing every day

A more practical reason to limit channels is that there is without a
doubt oodles of overflow bugs that trigger with channels >= INT32_MAX
that don't trigger with channels == FF_SANE_NB_CHANNELS. Formal
verification would discovered these of course, but we have nowhere near
enough labour power to do that across the entire codebase. So limiting
channels is a practical way to ensure channels*bytes_per_sample and so
on can't overflow.

A second question is: which users would need two billion channels?

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