[FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.

[Mark Thompson]

On 05/06/18 17:30, Jacob Trimble wrote:
> Just because I can't check whether my food has salmonella doesn't mean
> I shouldn't check the temperature when I cook it.  Adding a NULL check
> is trivial and will catch the most common error case.  We also can't
> check whether malloc() allocates enough memory, so should we then not
> check for NULL?  NULL is used as an error signal, so if the caller
> didn't include a NULL check, they will pass it here.  Rather than
> crashing the program (hopefully it will crash, it is undefined
> behavior, so anything could happen), we should be nice and validate
> the input and error out.  Just because it is impossible to check other
> error cases doesn't mean we should ignore all error checks.

(My opinion, others may disagree.)

Please consider what is actually useful to an API user here.

The check you are suggesting will cause the function to, when passed entirely invalid arguments, silently return having done nothing.  Is this better than the almost-guaranteed segfault you will get instead?  Well, no.  There is much more scope for the error to go unnoticed and cause other hard-to-debug issues later, where it could have been caught immediately.

If there is a concern that a function like this could be misused then (since this is certainly undefined behaviour in any case) turning it into an abort() is the best case so that the program will definitely fail and any errors can be diagnosed immediately.  As such, I think argument checks for nonsensical invalid input like this should be done either with av_assert or not at all.

- Mark