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

[Andreas Rheinhardt]

Guo, Yejun:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: 2021年2月21日 12:12
>> To: ffmpeg-devel at 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 at ffmpeg.org> On Behalf Of
>>>> Andreas Rheinhardt
>>>> Sent: 2021年2月21日 9:41
>>>> To: ffmpeg-devel at ffmpeg.org
>>>> Cc: Andreas Rheinhardt <andreas.rheinhardt at 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 at 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