Check NULL pointers or not...

[Benjamin Kaduk]

On Wed, Nov 27, 2019 at 10:38:41AM +0100, Richard Levitte wrote:
> Depending on who you're asking, you get different responses.
> 
> The topic is: should we check pointers coming from applications before
> using them or not?
> 
> There are two positions on this:
> 
> 1. Yes we should so we don't crash processes unnecessarily, and we
>    should return an error if pointers are NULL (unless that's valid
>    input, such as for the freeing functions).  There is a generic
>    error reason code for it, ERR_R_PASSED_NULL_PARAMETER.
> 
> 2. No we should not, as we don't cater for user programming errors.
>    Also, it allows us to catch our own errors early.
> 
> For a while, our pendulum was going with option 2, but lately, we seem
> to go back to option 1.
> 
> Both options have their pros and cons:
> 
> 1. cons: there's the very real possibility that users don't check for
>          errors as they should, and things go wrong further on,
>          sometimes in ways where the actual cause is very hard to
>          figure out.
>    pros: programs will not spuriously crash because we didn't catch an
>          internal corner case in our tests.
> 
> 2. cons: programs may crash, sometimes due to our own mistakes,
>          sometimes due to user errors.
>    pros: some very subtle bugs are found, either quicker or at all.
>          An actual case is PR 7630 [1], which was uncovered because
>          there was a crash due to a NULL being used (this was
>          ultimately due to users not checking errors).
> 
> As a middle ground, and perhaps a way to satify both camps, we could
> use ossl_assert() in our checks for input NULL, as follows:
> 
>     if (!ossl_assert(ptr != NULL)) {
>         ERR_raise(ERR_LIB_WHATEVER, ERR_R_PASSED_NULL_PARAMETER);
>         return 0;
>     }
> 
> This means that in a debug build, there may be crashes due to pointers
> being NULL, while in a release build, the caller gets an error.
> 
> Thoughts?

My recollection was that we had some reasonable support for this "middle
ground" approach.  I don't personally have strong enough feelings to
advocate a specific position, and will try to adhere to the group consensus
going forward.

-Ben