[FFmpeg-devel] [PATCH V2] avutil/tx: add check against (*ctx)

[James Almer]

On 5/16/2019 3:11 PM, Nicolas George wrote:
> James Almer (12019-05-16):
>> There are two precedents in the codebase, one checking for both the
>> passed argument and then the struct pointer pointed by it (av_bsf_free
>> and av_buffer_unref as i mentioned above), and one checking only the
>> struct pointer (av_bsf_list_free as you mentioned in another email).
> 
> There are a few other precedent. I think the majority dereference the
> pointer and only accept indirect NULL. This is not surprising: freeing
> something unconditionally is very useful when uniniting after a failure
> case or different branches of programming. On the other hand, freeing
> nothing is not a useful pattern.
> 
>> The current code is wrong as already established. This patch applying
>> (!ctx || !*ctx), and the one sent last night applying (!*ctx) are both
>> correct, each of them implementing one of the two aforementioned
> 
> Ok, I had missed there was another patch.
> 
>> precedents. The former just takes extra precautions against user error.
> 
> Not user: application programmer. This is a very important difference:
> users are supposed clueless, application programmers are supposed
> competent, even when they are not.

I meant it as library user, which is the application programmer and not
the consumer/end user, but yes, you're right they should know better.

> 
> That kind of mistake in using the API is probably a simple but grave
> mistake in the code, like writing av_foo_free(foo) instead of
> av_foo_free(&foo) (and ignoring the resulting warning). If ignored, it
> will lead to invalid frees or memory leaks in the application. This is
> not a service for the programmer: better have the program crash hard
> (either with an assert or a NULL deref) immediately, so they can fix it.
> 
> Regards,
In that case I'm fine with using the single (!*ctx) check from the first
patch.