[openssl-project] Removal of NULL checks

[Richard Levitte]

In message <8ACBEBBF-F4A2-469F-BB5B-3564A24DC9B8 at dukhovni.org> on Thu, 9 Aug 2018 11:02:16 -0400, Viktor Dukhovni <openssl-users at dukhovni.org> said:

openssl-users> 
openssl-users> 
openssl-users> > On Aug 9, 2018, at 9:49 AM, Salz, Rich <rsalz at akamai.com> wrote:
openssl-users> > 
openssl-users> > This is another reason why I am opposed to NULL checks.
openssl-users> 
openssl-users> Whether one's for them, or against them, removing a check
openssl-users> from a function that would formerly return an error and
openssl-users> making it crash is a substantial API change.  We must
openssl-users> avoid API changes whenever we can.  We can introduce
openssl-users> new functions and gradually deprecate the old over
openssl-users> a number (at least 3 IMHO) major release cycles, but
openssl-users> what we MUST NOT do is just change the API of an
openssl-users> existing function.

I think we need to be a bit more nuanced in our views.  Bug fixes are
potentially behaviour changes (for example, I recently got through a
PR that added a stricter check of EVP_PKEY_asn1_new() input; see #6880
(*)).

Also, we might want to look at our own documentation to see what can
be reasonably expected.  doc/man3/DEFINE_STACK_OF.pod defines all the
stack functions.  Just scanning for NULL shows that sk == NULL is
defined for sk_TYPE_num() and sk_TYPE_set().  That possibility isn't
mentioned for any other of the safestack functions, and the behaviour
could therefore be seen as undefined.

"But this is how it has worked so far!"  Yeah?  Still undefined behaviour.
I think we're doing ourselves a disservice if we get too stuck by
behaviour that can only be reasonably derived by reading the source
code rather than the docs.

So there's a choice, and if we accept that NULL is valid input the the
safestack functions, we should document it.  If not, then sk == NULL
is still mostly undefined, and crashes are therefore as expected as
anything else.

However, caution isn't a bad thing.  I think that as part of a minor
version upgrade, removing existing NULL checks may be a bit rad.
However, I'd say that for the next major version, we're free to change
an undefined behaviour to something more well defined, as we
see fit.

Cheers,
Richard

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