[FFmpeg-devel,1/2] avcodec/movtextenc: Check for too many styles

The counter for the number of styles is written on two bytes, ergo
anything > UINT16_MAX is invalid. This also fixes a compiler warning
because of a tautologically true check on 64bit systems.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
A better solution would be to error out as soon as the byte length of a
subtitle exceeds UINT16_MAX; yet for this one would have to modify all
of ass_split to allow the callbacks to return errors.

 libavcodec/movtextenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年2月21日 9:41
> To: ffmpeg-devel@ffmpeg.org
> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too many
> styles
> 
> The counter for the number of styles is written on two bytes, ergo anything >
> UINT16_MAX is invalid. This also fixes a compiler warning because of a
> tautologically true check on 64bit systems.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> A better solution would be to error out as soon as the byte length of a subtitle
> exceeds UINT16_MAX; yet for this one would have to modify all of ass_split to
> allow the callbacks to return errors.
> 
>  libavcodec/movtextenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c index
> 1bef21e0b9..cf30adbd0a 100644
> --- a/libavcodec/movtextenc.c
> +++ b/libavcodec/movtextenc.c
> @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext *s)
>          StyleBox *tmp;
> 
>          // last style != defaults, end the style entry and start a new one
> -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) ||
> +        if (s->count + 1 > FFMIN(SIZE_MAX /
> + sizeof(*s->style_attributes), UINT16_MAX) ||

hi, logically, I think the result of FFMIN(SIZE_MAX / sizeof(*s->style_attributes), UINT16_MAX) is always UINT16_MAX, we may just use 's->count + 1 > UINT16_MAX'.


>              !(tmp = av_fast_realloc(s->style_attributes,
> 
> &s->style_attributes_bytes_allocated,
>                                      (s->count + 1) *
> sizeof(*s->style_attributes)))) {
> --
> 2.27.0
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".

Guo, Yejun:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: 2021年2月21日 9:41
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too many
>> styles
>>
>> The counter for the number of styles is written on two bytes, ergo anything >
>> UINT16_MAX is invalid. This also fixes a compiler warning because of a
>> tautologically true check on 64bit systems.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> A better solution would be to error out as soon as the byte length of a subtitle
>> exceeds UINT16_MAX; yet for this one would have to modify all of ass_split to
>> allow the callbacks to return errors.
>>
>>  libavcodec/movtextenc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c index
>> 1bef21e0b9..cf30adbd0a 100644
>> --- a/libavcodec/movtextenc.c
>> +++ b/libavcodec/movtextenc.c
>> @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext *s)
>>          StyleBox *tmp;
>>
>>          // last style != defaults, end the style entry and start a new one
>> -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) ||
>> +        if (s->count + 1 > FFMIN(SIZE_MAX /
>> + sizeof(*s->style_attributes), UINT16_MAX) ||
> 
> hi, logically, I think the result of FFMIN(SIZE_MAX / sizeof(*s->style_attributes), UINT16_MAX) is always UINT16_MAX, we may just use 's->count + 1 > UINT16_MAX'.
> 

And why?

> 
>>              !(tmp = av_fast_realloc(s->style_attributes,
>>
>> &s->style_attributes_bytes_allocated,
>>                                      (s->count + 1) *
>> sizeof(*s->style_attributes)))) {
>> --
>> 2.27.0
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
>> with subject "unsubscribe".
> _______________________________________________
> 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".
>

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年2月21日 12:12
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too
> many styles
> 
> Guo, Yejun:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: 2021年2月21日 9:41
> >> To: ffmpeg-devel@ffmpeg.org
> >> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too
> >> many styles
> >>
> >> The counter for the number of styles is written on two bytes, ergo
> >> anything > UINT16_MAX is invalid. This also fixes a compiler warning
> >> because of a tautologically true check on 64bit systems.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >> A better solution would be to error out as soon as the byte length of
> >> a subtitle exceeds UINT16_MAX; yet for this one would have to modify
> >> all of ass_split to allow the callbacks to return errors.
> >>
> >>  libavcodec/movtextenc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c index
> >> 1bef21e0b9..cf30adbd0a 100644
> >> --- a/libavcodec/movtextenc.c
> >> +++ b/libavcodec/movtextenc.c
> >> @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext *s)
> >>          StyleBox *tmp;
> >>
> >>          // last style != defaults, end the style entry and start a new one
> >> -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) ||
> >> +        if (s->count + 1 > FFMIN(SIZE_MAX /
> >> + sizeof(*s->style_attributes), UINT16_MAX) ||
> >
> > hi, logically, I think the result of FFMIN(SIZE_MAX /
> sizeof(*s->style_attributes), UINT16_MAX) is always UINT16_MAX, we may just
> use 's->count + 1 > UINT16_MAX'.
> >
> 
> And why?

For the original code below, the compiler reports that it is always false.
s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes)

Since s->count is unsigned int, imho, it means that SIZE_MAX / sizeof(*s->style_attributes)
is always larger than uint_max, and so of course larger than uint16_max.

another is that:
sizeof(*s->style_attributes) is 12,
SIZE_MAX: 18446744073709551615
UINT16_MAX: 65535

checked with code:
#include <stdio.h>
#include <stdint.h>
int main()
{
    printf("SIZE_MAX: %lu\nUINT16_MAX: %d\n", SIZE_MAX, UINT16_MAX);
}

Guo, Yejun:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: 2021年2月21日 12:12
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too
>> many styles
>>
>> Guo, Yejun:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Andreas Rheinhardt
>>>> Sent: 2021年2月21日 9:41
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too
>>>> many styles
>>>>
>>>> The counter for the number of styles is written on two bytes, ergo
>>>> anything > UINT16_MAX is invalid. This also fixes a compiler warning
>>>> because of a tautologically true check on 64bit systems.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> ---
>>>> A better solution would be to error out as soon as the byte length of
>>>> a subtitle exceeds UINT16_MAX; yet for this one would have to modify
>>>> all of ass_split to allow the callbacks to return errors.
>>>>
>>>>  libavcodec/movtextenc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c index
>>>> 1bef21e0b9..cf30adbd0a 100644
>>>> --- a/libavcodec/movtextenc.c
>>>> +++ b/libavcodec/movtextenc.c
>>>> @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext *s)
>>>>          StyleBox *tmp;
>>>>
>>>>          // last style != defaults, end the style entry and start a new one
>>>> -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) ||
>>>> +        if (s->count + 1 > FFMIN(SIZE_MAX /
>>>> + sizeof(*s->style_attributes), UINT16_MAX) ||
>>>
>>> hi, logically, I think the result of FFMIN(SIZE_MAX /
>> sizeof(*s->style_attributes), UINT16_MAX) is always UINT16_MAX, we may just
>> use 's->count + 1 > UINT16_MAX'.
>>>
>>
>> And why?
> 
> For the original code below, the compiler reports that it is always false.
> s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes)
> 
> Since s->count is unsigned int, imho, it means that SIZE_MAX / sizeof(*s->style_attributes)
> is always larger than uint_max, and so of course larger than uint16_max.
> 
> another is that:
> sizeof(*s->style_attributes) is 12,
> SIZE_MAX: 18446744073709551615
> UINT16_MAX: 65535
> 
> checked with code:
> #include <stdio.h>
> #include <stdint.h>
> int main()
> {
>     printf("SIZE_MAX: %lu\nUINT16_MAX: %d\n", SIZE_MAX, UINT16_MAX);
> }
> 
FYI: The C standard does not mandate the sizes for most types (the
uintx_t types are an exception). It also gives the compiler absolute
freedom on how much padding to add to a structure. A compiler could very
well add megabytes of padding to StyleBox. The numbers you provide only
pertain to one (or actually none, see below) implementation, not to all
of them. It is like someone claiming to prove a general theorem by only
checking it for one example.

Several of your statements are btw not only false for a hypothetical
system compliant with the C standard. They are false on common systems:
"SIZE_MAX / sizeof(*s->style_attributes) is always larger than uint_max"
is wrong on ordinary 32bit systems (where SIZE_MAX and UINT_MAX
typically coincide). And "sizeof(*s->style_attributes) is 12". Due to
padding it is normally 16; it could be reduced to 12 (for ordinary
systems) by rearranging its elements.

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年2月21日 12:39
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too
> many styles
> 
> Guo, Yejun:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: 2021年2月21日 12:12
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for
> >> too many styles
> >>
> >> Guo, Yejun:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>>> Andreas Rheinhardt
> >>>> Sent: 2021年2月21日 9:41
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>> Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for
> >>>> too many styles
> >>>>
> >>>> The counter for the number of styles is written on two bytes, ergo
> >>>> anything > UINT16_MAX is invalid. This also fixes a compiler
> >>>> warning because of a tautologically true check on 64bit systems.
> >>>>
> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>> ---
> >>>> A better solution would be to error out as soon as the byte length
> >>>> of a subtitle exceeds UINT16_MAX; yet for this one would have to
> >>>> modify all of ass_split to allow the callbacks to return errors.
> >>>>
> >>>>  libavcodec/movtextenc.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
> >>>> index 1bef21e0b9..cf30adbd0a 100644
> >>>> --- a/libavcodec/movtextenc.c
> >>>> +++ b/libavcodec/movtextenc.c
> >>>> @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext
> *s)
> >>>>          StyleBox *tmp;
> >>>>
> >>>>          // last style != defaults, end the style entry and start a new
> one
> >>>> -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) ||
> >>>> +        if (s->count + 1 > FFMIN(SIZE_MAX /
> >>>> + sizeof(*s->style_attributes), UINT16_MAX) ||
> >>>
> >>> hi, logically, I think the result of FFMIN(SIZE_MAX /
> >> sizeof(*s->style_attributes), UINT16_MAX) is always UINT16_MAX, we
> >> may just use 's->count + 1 > UINT16_MAX'.
> >>>
> >>
> >> And why?
> >
> > For the original code below, the compiler reports that it is always false.
> > s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes)
> >
> > Since s->count is unsigned int, imho, it means that SIZE_MAX /
> > sizeof(*s->style_attributes) is always larger than uint_max, and so of course
> larger than uint16_max.
> >
> > another is that:
> > sizeof(*s->style_attributes) is 12,
> > SIZE_MAX: 18446744073709551615
> > UINT16_MAX: 65535
> >
> > checked with code:
> > #include <stdio.h>
> > #include <stdint.h>
> > int main()
> > {
> >     printf("SIZE_MAX: %lu\nUINT16_MAX: %d\n", SIZE_MAX,
> UINT16_MAX); }
> >
> FYI: The C standard does not mandate the sizes for most types (the uintx_t
> types are an exception). It also gives the compiler absolute freedom on how
> much padding to add to a structure. A compiler could very well add megabytes
> of padding to StyleBox. The numbers you provide only pertain to one (or
> actually none, see below) implementation, not to all of them. It is like
> someone claiming to prove a general theorem by only checking it for one
> example.
> 
> Several of your statements are btw not only false for a hypothetical system
> compliant with the C standard. They are false on common systems:
> "SIZE_MAX / sizeof(*s->style_attributes) is always larger than uint_max"
> is wrong on ordinary 32bit systems (where SIZE_MAX and UINT_MAX typically
> coincide). And "sizeof(*s->style_attributes) is 12". Due to padding it is normally
> 16; it could be reduced to 12 (for ordinary
> systems) by rearranging its elements.
> 

thanks for the lucid explanation, yes, you're right. 
(and first to know that megabytes of padding is possible)

btw, just as a learning, is there possible a real system where uint_max < uint32_max?
If no, we might say that: size_max >= uint_max >= uint32_max > uint16_max.

Andreas Rheinhardt:
> The counter for the number of styles is written on two bytes, ergo
> anything > UINT16_MAX is invalid. This also fixes a compiler warning
> because of a tautologically true check on 64bit systems.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> A better solution would be to error out as soon as the byte length of a
> subtitle exceeds UINT16_MAX; yet for this one would have to modify all
> of ass_split to allow the callbacks to return errors.
> 
>  libavcodec/movtextenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
> index 1bef21e0b9..cf30adbd0a 100644
> --- a/libavcodec/movtextenc.c
> +++ b/libavcodec/movtextenc.c
> @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext *s)
>          StyleBox *tmp;
>  
>          // last style != defaults, end the style entry and start a new one
> -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) ||
> +        if (s->count + 1 > FFMIN(SIZE_MAX / sizeof(*s->style_attributes), UINT16_MAX) ||
>              !(tmp = av_fast_realloc(s->style_attributes,
>                                      &s->style_attributes_bytes_allocated,
>                                      (s->count + 1) * sizeof(*s->style_attributes)))) {
> 
Will apply this patchset tomorrow unless there are objections.

- Andreas

Quoting Guo, Yejun (2021-02-21 06:31:37)
> 
> 
> > >
> > FYI: The C standard does not mandate the sizes for most types (the uintx_t
> > types are an exception). It also gives the compiler absolute freedom on how
> > much padding to add to a structure. A compiler could very well add megabytes
> > of padding to StyleBox. The numbers you provide only pertain to one (or
> > actually none, see below) implementation, not to all of them. It is like
> > someone claiming to prove a general theorem by only checking it for one
> > example.
> > 
> > Several of your statements are btw not only false for a hypothetical system
> > compliant with the C standard. They are false on common systems:
> > "SIZE_MAX / sizeof(*s->style_attributes) is always larger than uint_max"
> > is wrong on ordinary 32bit systems (where SIZE_MAX and UINT_MAX typically
> > coincide). And "sizeof(*s->style_attributes) is 12". Due to padding it is normally
> > 16; it could be reduced to 12 (for ordinary
> > systems) by rearranging its elements.
> > 
> 
> thanks for the lucid explanation, yes, you're right. 
> (and first to know that megabytes of padding is possible)
> 
> btw, just as a learning, is there possible a real system where uint_max < uint32_max?
> If no, we might say that: size_max >= uint_max >= uint32_max > uint16_max.

We assume int (and hence also unsigned int) is at least 32bit.
But AFAIK the only limit on size_t is that it's at least 16bit,
otherwise it can be smaller than int.

On Tue, 23 Feb 2021 12:16:53 +0100
Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:

> Andreas Rheinhardt:
> > The counter for the number of styles is written on two bytes, ergo
> > anything > UINT16_MAX is invalid. This also fixes a compiler warning
> > because of a tautologically true check on 64bit systems.
> > 
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> > ---
> > A better solution would be to error out as soon as the byte length
> > of a subtitle exceeds UINT16_MAX; yet for this one would have to
> > modify all of ass_split to allow the callbacks to return errors.
> > 
> >  libavcodec/movtextenc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
> > index 1bef21e0b9..cf30adbd0a 100644
> > --- a/libavcodec/movtextenc.c
> > +++ b/libavcodec/movtextenc.c
> > @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext
> > *s) StyleBox *tmp;
> >  
> >          // last style != defaults, end the style entry and start a
> > new one
> > -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes)
> > ||
> > +        if (s->count + 1 > FFMIN(SIZE_MAX /
> > sizeof(*s->style_attributes), UINT16_MAX) || !(tmp =
> > av_fast_realloc(s->style_attributes,
> > &s->style_attributes_bytes_allocated, (s->count + 1) *
> > sizeof(*s->style_attributes)))) { 
> Will apply this patchset tomorrow unless there are objections.
> 

Looks fine. Thanks!

--phil

Anton Khirnov:
> Quoting Guo, Yejun (2021-02-21 06:31:37)
>>
>>
>>>>
>>> FYI: The C standard does not mandate the sizes for most types (the uintx_t
>>> types are an exception). It also gives the compiler absolute freedom on how
>>> much padding to add to a structure. A compiler could very well add megabytes
>>> of padding to StyleBox. The numbers you provide only pertain to one (or
>>> actually none, see below) implementation, not to all of them. It is like
>>> someone claiming to prove a general theorem by only checking it for one
>>> example.
>>>
>>> Several of your statements are btw not only false for a hypothetical system
>>> compliant with the C standard. They are false on common systems:
>>> "SIZE_MAX / sizeof(*s->style_attributes) is always larger than uint_max"
>>> is wrong on ordinary 32bit systems (where SIZE_MAX and UINT_MAX typically
>>> coincide). And "sizeof(*s->style_attributes) is 12". Due to padding it is normally
>>> 16; it could be reduced to 12 (for ordinary
>>> systems) by rearranging its elements.
>>>
>>
>> thanks for the lucid explanation, yes, you're right. 
>> (and first to know that megabytes of padding is possible)
>>
>> btw, just as a learning, is there possible a real system where uint_max < uint32_max?
>> If no, we might say that: size_max >= uint_max >= uint32_max > uint16_max.
> 
> We assume int (and hence also unsigned int) is at least 32bit.
> But AFAIK the only limit on size_t is that it's at least 16bit,
> otherwise it can be smaller than int.
> 
While it is true that the smallest permissible value of SIZE_MAX is
65535, we de facto require it to be much greater: We never check for
truncation when using any allocation function with an unsigned
parameter, ergo we rely on SIZE_MAX >= UINT_MAX. And some of our
structures don't fit into 65535B (e.g. the context of the DVB subtitles
decoder contains an "int clut_count2[257][256]") as do other objects (we
seem to have at least 40 objects with a size >= 65536; the biggest is
the VLC table for atrac3plus); H.2645 packets are even padded with
256KiB (see MAX_MBPAIR_SIZE). The latter would lead to nonsense
behaviour on a system with SIZE_MAX == 65535, while the other two would
lead to compilation errors.

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2021年2月24日 3:53
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: Check for too
> many styles
> 
> Anton Khirnov:
> > Quoting Guo, Yejun (2021-02-21 06:31:37)
> >>
> >>
> >>>>
> >>> FYI: The C standard does not mandate the sizes for most types (the uintx_t
> >>> types are an exception). It also gives the compiler absolute freedom on
> how
> >>> much padding to add to a structure. A compiler could very well add
> megabytes
> >>> of padding to StyleBox. The numbers you provide only pertain to one (or
> >>> actually none, see below) implementation, not to all of them. It is like
> >>> someone claiming to prove a general theorem by only checking it for one
> >>> example.
> >>>
> >>> Several of your statements are btw not only false for a hypothetical system
> >>> compliant with the C standard. They are false on common systems:
> >>> "SIZE_MAX / sizeof(*s->style_attributes) is always larger than uint_max"
> >>> is wrong on ordinary 32bit systems (where SIZE_MAX and UINT_MAX
> typically
> >>> coincide). And "sizeof(*s->style_attributes) is 12". Due to padding it is
> normally
> >>> 16; it could be reduced to 12 (for ordinary
> >>> systems) by rearranging its elements.
> >>>
> >>
> >> thanks for the lucid explanation, yes, you're right.
> >> (and first to know that megabytes of padding is possible)
> >>
> >> btw, just as a learning, is there possible a real system where uint_max <
> uint32_max?
> >> If no, we might say that: size_max >= uint_max >= uint32_max >
> uint16_max.
> >
> > We assume int (and hence also unsigned int) is at least 32bit.
> > But AFAIK the only limit on size_t is that it's at least 16bit,
> > otherwise it can be smaller than int.
> >
> While it is true that the smallest permissible value of SIZE_MAX is
> 65535, we de facto require it to be much greater: We never check for
> truncation when using any allocation function with an unsigned
> parameter, ergo we rely on SIZE_MAX >= UINT_MAX. And some of our
> structures don't fit into 65535B (e.g. the context of the DVB subtitles
> decoder contains an "int clut_count2[257][256]") as do other objects (we
> seem to have at least 40 objects with a size >= 65536; the biggest is
> the VLC table for atrac3plus); H.2645 packets are even padded with
> 256KiB (see MAX_MBPAIR_SIZE). The latter would lead to nonsense
> behaviour on a system with SIZE_MAX == 65535, while the other two would
> lead to compilation errors.

got it, thanks Andreas and Anton.