[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);
}