In message <[email protected]> on Thu, 9 Aug 2018 12:52:56 -0400, Viktor Dukhovni <[email protected]> said:
viktor> On Thu, Aug 09, 2018 at 06:40:14PM +0200, Richard Levitte wrote: viktor> > In message <[email protected]> on Thu, 9 Aug 2018 12:22:45 -0400, Viktor Dukhovni <[email protected]> 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 [email protected] OpenSSL Project http://www.openssl.org/~levitte/ _______________________________________________ openssl-project mailing list [email protected] https://mta.openssl.org/mailman/listinfo/openssl-project
