Re: Check NULL pointers or not...

2019-11-28 Thread 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.]




Re: Check NULL pointers or not...

2019-11-28 Thread 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


Re: AW: Check NULL pointers or not...

2019-11-28 Thread Richard Levitte
On Wed, 27 Nov 2019 11:53:01 +0100,
Dr. Matthias St. Pierre wrote:
> 
> FYI: there was a related discussion a year ago on GitHub about an 
> ossl_is_null() macro,
> 
> https://github.com/openssl/openssl/pull/7218
> 
> which ended up undecided.

Thank you.  I had totally forgotten that one, so reminder appreciated.

Looking at the diff shows me that there's a precedent for what I
proposed (using ossl_assert).

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/