OpenSSL Blog

Beyond Reformatting: More Code Cleanup

Posted by Rich Salz ,

The OpenSSL source doesn’t look the same as it did a year ago. Matt posted about the big code reformatting. In this post I want review some of the other changes – these rarely affect features, but are more than involved than “just” whitespace.

Previously, we’d accept platform patches from just about anyone. This led to some really hard-to-follow pre-processor guards:

    #if !defined(TERMIO) && !defined(TERMIOS) \
        && !defined(OPENSSL_SYS_VMS) && !defined(OPENSSL_SYS_MSDOS) \
        && !defined(OPENSSL_SYS_MACINTOSH_CLASSIC) && \
        !defined(MAC_OS_GUSI_SOURCE)

Can anyone reasonably tell when that applies? And there were many that were just plain silly:

    #if 1 /* new with OpenSSL 0.9.7 */

    #ifdef undef

So we did a couple of things.

Unsupported platforms

First, we’re no longer supporting every known platform; it has to be something that either someone on the team has direct access to, or we have extensive support from the vendor.

Back in the fall we kicked off a discussion on the openssl-dev mailing list about removing unsupported platforms. Based on feedback, Netware is still supported and people are contacting some vendors to get them to step up and help support their platform. But the following are now gone:

  • Sony NEWS4
  • BEOS and BEOS_R5
  • NeXT
  • SUNOS
  • MPE/iX
  • Sinix/ReliantUNIX RM400
  • DGUX
  • NCR
  • Tandem
  • Cray
  • Win16

OpenSSL never supported 16-bit (neither did SSLeay, really), and we just never made that explicit.

Live/dead code

Almost every instance of #if 0 or #if 1 had the appropriate code removed. There are a couple of places where they remain, because it could be useful documentation. For example, this block in ssl/s3_lib.c:

    #if 0
        /*
         * Do not set the compare functions, because this may lead to a
         * reordering by "id". We want to keep the original ordering. We may pay
         * a price in performance during sk_SSL_CIPHER_find(), but would have to
         * pay with the price of sk_SSL_CIPHER_dup().
         */
         sk_SSL_CIPHER_set_cmp_func(srvr, ssl_cipher_ptr_id_cmp);
         sk_SSL_CIPHER_set_cmp_func(clnt, ssl_cipher_ptr_id_cmp);
    #endif

The once place they do remain is in the crypto/bn component. That’s just because I’m scared to touch it. :) That directory counts for about one-third of all remaining instances. Compared to 1.0.2, which had 343 instances in 165 files, we now have 43 instances in 31 files, and I hope we can reduce that even more.

ifdef control

Over time, some feature-control #ifdef’s evolved into inconsistencies. We cleaned them up, merging OPENSSL_NO_RIPEMD160 and OPENSSL_NO_RIPEMD into OPENSSL_NO_RMD160. Similarly, OPENSSL_NO_FP_API was merged into OPENSSL_NO_STDIO. (We’ll soon make that buildable again, for embedded platforms.)

Also in this area, configuration used OPENSSSL_SYSNAME_xxx which was internally mapped to OPENSSL_SYS_xxx. Removing that mapping, in conjunction with removing some old platforms, made parts of the internal header file e_os.h much simpler, but we there is still room for much improvement there.

The biggest change was removing about one-third of the nearly 100 options that we used to support. Many of these had suffered severe bit-rot and no longer worked. The biggest part was removing the ability to build OpenSSL without other parts of the library: BIO, BUFFER, EVP, LHASH, HASH_COMP, LOCKING, OBJECT, STACK are no longer optional; OPENSSL_NO_BIO has no effect (and will hopefully generate a warning). We also removed support for the broken SHA0 and DSS0, the CBCM mode of DES (invented by IBM and not used anywhere), and the maintenance of our own free lists, OPENSSL_NO_BUF_FREELISTS. This last got us some flack erroneously sent our way, but it was clearly a holdover from when the major supported platforms did not have efficient malloc/free components.

Dynamic memory cleanup

We did a big cleanup on how we use various memory functions. For example:

    a = (foo *)malloc(n * sizeof(foo))

had a needless cast, and would quietly break if the type of a changed. We now do this consistently:

    a = malloc(n * sizeof(*a));

Similar sizeof changes were done for memset and memcpy calls.

We also stopped checking for NULL before calling free. That was huge, requiring over a dozen commits. It also reduced code complexity, and got us a few extra percentage points on our test coverage. :)

Summary

The OpenSSL code base has a long and storied history. With these changes, the reformatting, and others, I think we’re making good on our commitment for the future.

Comments