[FFmpeg-devel] [PATCH] libavutil: AVEncodeInfo data structures

[James Almer]

On 8/13/2019 7:29 PM, Nicolas George wrote:
> Juan De León (12019-08-13):
>> The array is there so that the structure isn't opaque, it should be
>> accessed with the function.
> 
> I realize you need it, but not for the reason you say. It is needed for
> alignment: if blocks needs more alignment than info, info+sizeof(info)
> is not a valid pointer for blocks.
> 
>>>> +    if (!info || idx >= info->nb_blocks || idx < 0)
>>>> +        return NULL;
>>> How valid is it for applications to call with idx outside the range?
>> They shouldn't but I figure it's better to return NULL than to get
>> undefined behaviour.
> 
> In that case, I think is is better practice to document:
> 
> 	Index must be between 0 and nb_blocks
> 
> and check with an assert, forcing the application programmer fix their
> code immediately.
> 
> Most code will likely use idx from a loop counter, where it cannot be
> outside the range, and for the few other cases, the caller can do the
> check if necessary.
> 
> Also, make the fields that cannot be negative unsigned, and you can drop
> the <0 test.
> 
> Regards,

I'm fairly sure this was discussed before, but invalid arguments
shouldn't crash an user's application. They even have their own
standardized errno value for this purpose.
asserts() are to catch bugs in our code, not in theirs. Returning a NULL
pointer is the preferred behavior.