Check NULL pointers or not...

[Tomas Mraz]

On Thu, 2019-11-28 at 11:19 -0800, Benjamin Kaduk wrote:
> 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.

I'd the option 2 or the middle-ground option. However if we choose the
middle-ground will we try to apply it to old code or will we just add
it to the new API calls or when the code of an existing API call is
being significantly reworked anyway?

Also there might be exceptional cases where taking the option 1 is
actually a better choice, but I do not think this should be the norm.

Also in some cases where the API call is called very frequently in
performance sensitive code it might be good idea to choose option 2
even in case we would decide to use the middle-ground otherwise.
-- 
Tomáš Mráz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
[You'll know whether the road is wrong if you carefully listen to your
conscience.]