[openssl-project] Removal of NULL checks

[Viktor Dukhovni]

On Thu, Aug 09, 2018 at 05:53:11PM +0200, Richard Levitte wrote:

> 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
> (*)).

We went too far too quickly in the transition from 1.0.2 to 1.1.0,
e.g. by needlessly renaming some functions without providing the
legacy names (even as deprecated aliases) and by not adding to 1.0.2
the new accessors required for compatibility with 1.1.0.  Mostly
that could have been done (and could still be done) via new macros
in headers that add 1.1.0 accessors to 1.0.2:

	#if OPENSSL_API_COMPAT >= 0x10100000UL
	#define EVP_MD_CTX_new() EVP_MD_CTX_create()
	...
	#endif

As a result many applications that need to support both 1.0.2 and
1.1.0 (whichever is available) had to waste effort to create the
requisite #ifdefs, wrapper functions, ...

If we keep doing that, everyone will be using LibreSSL or another
alternative.  We must not casually change APIs.  Especially because
of our documentation deficit, which results in users learning about
our interfaces via experimentation or reading the source.

If we must change an interface, and *can* do it by introducing a
new function (that we adequately document), that must be the way
forward.  And *furthermore*, we can't remove the deprecated interface
until the new function has been in multiple stable releases.  Indeed
to promote adoption, such new functions (when simple enough) should
be considered for inclusion in the extant stable releases, making
it easy to migrate from old to new.

> "But this is how it has worked so far!"  Yeah?  Still undefined behaviour.

Blaming the user for changes in undefined behaviour does not get
us more happy users.

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

I think we're doing our users and ourselves a disservice if we're
too casual about API changes.  We can only get away with major
incompatibilities like those between 1.0.2 and 1.1.0 once.  If we
keep doing that, we'll lose the application base.

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

If the functions previously returned an error, they must continue to
do that barring overwhelming reasons to make a change.

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

No, we need a greater emphasis on backwards compatibility, and
introduce API changes more slowly, over multiple releases that carry
old and new APIs, and we must not change the behaviour of existing
functions without renaming them, except when the current behaviour
is clearly a bug.

It needs to be possible to recompile and run without auditing code.
The worst kind of incompatibilities are those that are not reported
by the compiler, and are only found at runtime, possibly under unusual
conditions.

-- 
	Viktor.