[FFmpeg-devel] [PATCH v6] avcodec: add farbfeld encoder

[Stefano Sabatini]

On date Wednesday 2024-06-05 12:02:08 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
> > On date Tuesday 2024-06-04 17:28:35 -0500, Marcus B Spencer wrote:
[...]
> >> +#define HEADER_SIZE 16
> >> +
> >> +static int farbfeld_encode_frame(AVCodecContext *ctx, AVPacket *pkt,
> >> +                                 const AVFrame *p, int *got_packet)
> >> +{
> > 
> >> +    int raw_img_size = av_image_get_buffer_size(
> >> +        p->format,
> >> +        p->width,
> >> +        p->height,
> >> +        1
> >> +    );
> > 
> >> +    int64_t buf_size = (int64_t)raw_img_size + HEADER_SIZE;
> > 
> > not yet, this might change the sign for a negative raw_img_size, you
> > need two separate checks (one is not enough), as in the following:
> > 
> > int raw_img_size = av_image_get_buffer_size(p->format, p->width,p->height, 1);
> > 
> > if (raw_image_size < 0)
> >     return raw_image_size;
> >      
> > int buf_size = raw_img_size + HEADER_SIZE;
> > if (buf_size < 0)
> >     return AVERROR(EINVAL);
> 

> This is absolutely wrong: raw_img_size is nonnegative here as is
> HEADER_SIZE and the addition will be performed as an int, so that
> overflow would be UB which implies that the compiler can optimize this
> check away.

Correct, the following should avoid the UB if I'm not mistaken again:

if (HEADER_SIZE > (INT_MAX - raw_img_size))
     return AVERROR(EINVAL);
int buf_size = raw_img_size + HEADER_SIZE;
...

> One does not need two checks as long as int is 32 bits (because then one
> can just perform the addition in 64bits).

sizeof(int) is not defined by the C standard, so you cannot assume it
is 32 bits (even if on most platforms/compilers it will be)

> Just use the following (#if
> has been used because compilers have a tendency to emit warnings if a
> particular check is tautologically false):
> 
> #if INT_MAX > INT64_MAX - HEADER_SIZE
>     if (raw_img_size > INT64_MAX - HEADER_SIZE)
>         return AVERROR(ERANGE);
> #endif