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

[Andreas Rheinhardt]

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)
> 

Did you even read the following? It handles the case where simply using
64bits is not enough.

>> 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