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

[Nicolas George]

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.

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,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190516/b53406f3/attachment.sig>