In message <[email protected]> on Thu, 9 Aug 2018 11:02:16 -0400, Viktor Dukhovni <[email protected]> said:
openssl-users> openssl-users> openssl-users> > On Aug 9, 2018, at 9:49 AM, Salz, Rich <[email protected]> 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 [email protected] OpenSSL Project http://www.openssl.org/~levitte/ _______________________________________________ openssl-project mailing list [email protected] https://mta.openssl.org/mailman/listinfo/openssl-project
