[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日 12:39
> 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日 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.
> 

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.