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

[Lynne]

May 16, 2019, 7:22 PM by jamrial at gmail.com:

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

I'm not, I still want the 2 checks.