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

[Jacob Trimble]

On Tue, Jun 5, 2018 at 1:06 PM Mark Thompson <sw at jkqxz.net> wrote:
>
> 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.

It returns an error, which should be checked by the caller.  We can't
do anything to change the caller's code, but we can make our code not
crash their program.

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

What about invalid MP4 files?  Should we convert those to abort() too?
 When parsing MP4 files, we check that it is valid and return an error
code if it is not.  There is no difference between a method to parse
an MP4 file and these, so these should validate the inputs as best it
can and return errors if we find them.

>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

When I design an API, I try to make it hard to misuse, being as
helpful to the caller as possible.  Having the function intentionally
crash on certain input when a simple if could solve it is not a good
idea.  I think the memcpy API is an example of a bad api, it assumes
valid input.  Are we going to stop checking that MP4 files are valid
too?  If we only accept valid MP4 files it seems pointless to check it
and return errors.  These functions return NULL on errors, it is an
error to pass NULL, therefore we should check and return NULL.  It is
impossible to check some things, but if it were possible, I would
suggest checking that too.

One other problem is that NULL is sometimes a valid value to pass to a
function and sometimes not.  NULL is used as a signal value whereas an
invalid pointer is never valid.  From the compiler's point of view,
there is no difference between a function that accepts NULL as valid
and one that does not.  So the only thing we can do is write a comment
saying "Does not accept NULL", but this is fragile as the compiler
can't check this (without static analysis).  By having us return an
error for invalid input, it makes the API more stable and easier to
use.  We can't stop the caller from ignoring the return value, but we
can at least do our best to validate the input and avoid crashing the
program.

But if you aren't going to accept a patch to increase code heath,
there is nothing I can do.  I'll remove the NULL checks from my other
patch so it can be submitted.