[openssl-project] Removal of NULL checks

[Richard Levitte]

In message <20180809165255.GG14409 at straasha.imrryr.org> on Thu, 9 Aug 2018 12:52:56 -0400, Viktor Dukhovni <viktor at dukhovni.org> said:

viktor> On Thu, Aug 09, 2018 at 06:40:14PM +0200, Richard Levitte wrote:
viktor> > In message <20180809162245.GD14409 at straasha.imrryr.org> on Thu, 9 Aug 2018 12:22:45 -0400, Viktor Dukhovni <openssl-users at dukhovni.org> said:
viktor> > 
viktor> > openssl-users> It needs to be possible to recompile and run without auditing code.
viktor> > openssl-users> The worst kind of incompatibilities are those that are not reported
viktor> > openssl-users> by the compiler, and are only found at runtime, possibly under unusual
viktor> > openssl-users> conditions.
viktor> > 
viktor> > So in this particular case, such as unchecked calls of sk_ functions,
viktor> > including sk_TYPE_new(), just to discover later that "oops, the
viktor> > elements we thought we inserted aren't there"?  ;-)
viktor> 
viktor> The NULL checks were returning an error, not silently failing to
viktor> add the element.

I said *unchecked* calls of sk_ functions.

viktor> > Either way, sk == NULL will not be reported by the compiler, will only
viktor> > be found at runtime, possibly under unusual conditions.
viktor> 
viktor> Code that tested for the error and avoided a crash would absent
viktor> NULL checks crash (in unexpected cases).  The existing code should
viktor> be assumed to be correctly written for the current library behaviour.
viktor> What happens to already broken code is of little interest.

Code that is correctly written should check the value returned from
sk_TYPE_new(), no?

viktor> > The only
viktor> > difference is exactly how the user gets to find out in runtime; 1)
viktor> > mysterious failures because the stack that should contain n elements
viktor> > is really empty and unfillable, or 2) an immediate crash.
viktor> 
viktor> This is simply wrong I'm afraid.  The NULL checks in question
viktor> returned an *error* (rather than crash) that the application should
viktor> check.  Applications that are not doing their own NULL check and
viktor> expect us to do it for them, can check for return value.  This
viktor> makes possible more concise code:
viktor> 
viktor> 	X509 *x;
viktor> 	STACK_OF(X509) *s;
viktor> 
viktor> 	...
viktor> 	/* Allocate 's' and initialize with x as first element */
viktor> 	if (sk_X509_push(s = sk_X509_new(NULL), x) < 0) {
viktor> 		/* error */
viktor> 	}

I would regard that code incorrectly written, because it doesn't check
the value returned from sk_X509_new(NULL) (i.e. it doesn't properly
check for possible errors).  Correctly written code would be written
like this:

	X509 *x;
	STACK_OF(X509) *s;

	...
	/* Allocate 's' and initialize with x as first element */
	if ((s = sk_X509_new(NULL)) == NULL
	    || sk_X509_push(s, x) < 0) {
		/* error */
	}

However, if we actually want people to be able not to check if the
stack they wanted to allocate actually got allocated, the correct
course of action would be to make that a defined behaviour, i.e. fix
the docs accordingly.
I find it weird, however, that in this particular case, we would
expect users not to check if a stack was actually allocated, when such
checks is actually correct behaviour, for example on stuff returned by
OPENSSL_malloc(), EVP_PKEY_CTX_new() etc etc etc.

Cheers,
Richard

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