The “goto fail” bug as a coaching tool

I was working with a senior developer at a client recently, and came across a C# method several hundred lines long. It consisted of a series of if/else blocks with multiple levels of indentation. Each large if/else block began with source comments that described the intent of the block. Calls to static utility methods were peppered throughout. In effect, it was a procedural-style program embedded in a C# method.

This is a very common pattern in existing code bases, in all industries, in all programming languages, in code written decades ago and in code written yesterday. Anyone who helps developers understand and apply generally-accepted good practices has seen examples like this many times.

It wasn’t particularly complicated code, and the seams were indicated by the source comments. I said the method might be too long to be easily understood and safely modified, and suggested we refactor to pull out discrete chunks of functionality and get them into smaller methods with intention-revealing names.

The senior developer was genuinely puzzled. He said the method was fine just as it stood. He said it wasn’t long. To show me that it wasn’t long, he began to scroll through it. “See? It’s only a few hundred lines. It’s not long at all.”

In fairness, “long” is relative. I recall a single Java method at one client that was 12,000 lines long. Another client had coded an entire application in the constructor of a Spring-loaded bean, plus a set of static utility methods. Another client had a COBOL program that was 72,000 lines long. There are plenty of less-dramatic examples, as well. So, a few hundred lines…not so bad, relatively speaking.

When I see code like this, I often ask the team (or at least one member of the team) to sit with me and refactor. Some coaches proactively offer to do this, asking a new team to show them their “worst” hunk of code, and then showing them how it can be wrangled. The standard response from teams is to insist the code is hopelessly tangled, and it isn’t feasible to break it apart.

But the truth is in most of these cases it’s quite feasible to break the code apart. The most typical problems are

• hard-coded references to dependencies
• calls to static methods
• sheer length, usually due to multiple responsibilities or multiple processing steps coded in the same method or module

These problems can be mitigated by applying a few basic refactorings over and over again. It isn’t a difficult process, but it can be a time-consuming exercise. If we can just get people to sit with us and get started, they usually see how to continue the process on their own within a few minutes.

Some people are willing to watch you try (possibly expecting you to fail). Others, however, need to be reassured or “convinced” before they will let you start tearing their code apart. It can be challenging to help people understand how long is too long, when they are not in the habit of thinking about code in this way.

The “goto fail” bug

The story of the 2014 “goto fail” bug in Apple’s Secure Transport library might help people see the value of separating small sections of logic into separate methods that can be tested in isolation. In the case of the “goto fail” bug, making the code unit-testable could have exposed the error before the code went to market. Even without unit tests, breaking the code up into readable and meaningful sections could have exposed the error, as well; it would, at least, have made the extra line harder to overlook.

The structure of most long methods in existing code resembles the structure of the C function that caused the “goto fail” bug. Function SSLVerifySignedServerKeyExchange is very long and consists of one “if” block after another. The code is here: opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c

Buried in the middle of the function we find this code:

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

In the original context, the second goto statement is not as obvious as it appears when we extract this short snippet. It would be very easy to overlook. Actually, it was overlooked. That was the problem.

Martin Fowler’s suggested refactoring of the function (documented here: martinfowler.com/articles/testing-culture.html;) results in this more-readable and more-testable version of the offending snippet:

static OSStatus
HashHandshake(const HashReference* hashRef, SSLBuffer* clientRandom,
    SSLBuffer* serverRandom, SSLBuffer* exchangeParams,
    SSLBuffer* hashOut) {
  SSLBuffer hashCtx;
  OSStatus err = 0;
  hashCtx.data = 0;
  if ((err = ReadyHash(hashRef, &hashCtx)) != 0)
    goto fail;
  if ((err = hashRef->update(&hashCtx, clientRandom)) != 0)
    goto fail;
  if ((err = hashRef->update(&hashCtx, serverRandom)) != 0)
    goto fail;
  if ((err = hashRef->update(&hashCtx, exchangeParams)) != 0)
    goto fail;
  err = hashRef->final(&hashCtx, hashOut);
fail:
  SSLFreeBuffer(&hashCtx);
  return err;
}

You might be able to use a story like this that centers on a “famous” bug to illustrate the value of clean code structure and microtesting, as well as showing that it isn’t so difficult to remediate code like this. After all, your client can become famous, too. The question is what they would prefer to be famous for.

There’s nothing complicated about the Apple code. The reason the duplicate goto statement was overlooked is that there was a lot of similar-looking code all in a single mass in that one function. Isolated, fine-grained executable tests would have surfaced the incorrect behavior quickly.