goto hell;

A lot has been made of the recent goto fail; issue on multiple Apple platforms, some of it disappointingly ill informed, especially in the comment sections.

goto is evil, you see. Possibly even eve-yil. Except it isn't, at least not always. There is a perfectly acceptable C idiom when goto is a good choice.

Unfortunately we have been told that goto is considered harmful, which is a wonderfully understated way of saying you can go catastrophically wrong when abusing it. And that is true, you can write some horrific code with a poor use of goto, but read on and you'll see why speaking in absolutes may also be considered harmful (well, sometimes.)

In C you have no garbage collection, auto pointers or automatic reference counting unless you implement it yourself. In C you will probably end up with quite a lot of dynamically allocated memory, which needs to be cleaned up in case of an error. There are a few ways to do this, but the cleanest one uses goto.

Really? Do tell

Take for example the following code

static OSStatus SSLVerifySignedServerKeyExchange(…)
{
    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
        return err;
    }
    if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
        SSLFreeBuffer(&hashCtx);
        return err;
    }
    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
        SSLFreeBuffer(&hashCtx);
        return err;
    }
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        SSLFreeBuffer(&hashCtx);
        return err;
    }
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
        SSLFreeBuffer(&hashCtx);
        return err;
    }

    SSLFreeBuffer(&hashCtx);
    return err;
}

This is a partial rewrite of Apple's SSL code with the gotos taken out. In case of every error we have to free the hash context. If we forget to do it we are introducing potential memory leaks. It's a lot of repetitive code and pretty horrible to look at. Not only that but many people subscribe to the belief that you should have one exit point in a function and all those returns are poor form. I think there's a lot to be said for that. 

In fact the SSLEncodeRSAKeyParams function in the same SSL code uses this approach. It's a mess of multiple exit points and clumsy clean up. (In reality the big issue with the Apple code isn't to do with a single goto, it's how absolutely god-awful the code is.)

So lets try that again and invert the conditions to only execute in case of a positive result

static OSStatus SSLVerifySignedServerKeyExchange(…)
{
    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) == 0) {
        if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) == 0) {
            if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) == 0) {
        ....
    }

    SSLFreeBuffer(&hashCtx);
    return err;

}




Stop! Stop! Stop! Stopstopstopstop. Ok, that was a silly idea. To paraphrase Peter Venkman, no sane person would stack code like this. A nested hell of if statements (and madness) this way lies.

Once more, lets bring the conditions together

static OSStatus SSLVerifySignedServerKeyExchange(…)
{
    err = (ReadyHash(&SSLHashSHA1, &hashCtx) != 0) &&
          (SSLHashSHA1.update(&hashCtx, &clientRandom) != 0) &&
          (SSLHashSHA1.update(&hashCtx, &serverRandom) != 0) &&
          (SSLHashSHA1.update(&hashCtx, &signedParams) != 0) &&
          (SSLHashSHA1.final(&hashCtx, &hashOut) != 0);

    SSLFreeBuffer(&hashCtx);
    return err;
}

You know what, this could work. It's more concise, but only for this particular code. There will be other code where this is not possible. E.g.

bool_t someFunction(...)

{

    char *c = NULL;

    int  *i = NULL;

    int  *j = NULL;

    c = malloc(10);

    if ( NULL == c ) {

        /* Handle failure and exit */

    }

    i = malloc(10 * sizeof(*i));

    if ( NULL == i ) {

        /* Free c, handle failure and exit */

    }

    j = malloc(10 * sizeof(*j));

    if ( NULL == j ) {

        /* Free c, Free i, handle failure and exit */

    }

    return true;    

}

It's not possible to treat this function in a similar way to the last. So what do we do? The C solution is simple, when you encounter an error you set the return value and go to the end of your function to clean up any memory that was allocated. 

#define FAIL() do { ret = false; goto cleanup; } while(0)
bool_t someFunction(...)

{
    bool_t ret = true;

    char *c = NULL;

    int  *i = NULL;

    int  *j = NULL;

    c = malloc(10);

    if ( NULL == c ) {
        FAIL();

    }

    i = malloc(10 * sizeof(*i));

    if ( NULL == i ) {

        FAIL();

    }

    j = malloc(10 * sizeof(*j));

    if ( NULL == j ) {

        FAIL();

    }

cleanup:
    free(c);
    free(i);
    free(j);

    return ret;

}

Some people online got so worked up about this code being used in case of a failure spectacularly failed to understand the cleanup code executes at the end of the function regardless of whether there was a failure or not. A lot of the ill informed commentary on goto fail; may even have been avoided if Apple used end: or cleanup: as the goto label. 

Definitive proof of evil

Another aspect of the goto fail; outcry was that it was taken as proof that, even if goto might not always be harmful, it should never be used just in case. Because that would have made sure the error in the SSL server authentication code would never happen, right? This is also incorrect, check out this code

bool_t checkForSomethingReallyImportant(void)
{
    if ( isThatImportantThingOk() )
        return true;
        return true;

    return false;
}

I've just effectively made the same copy and paste error as in the SSL code, without resorting to a goto.

Finally, lets talk braces. Braces would have solved this issue. Really?

bool_t checkForSomethingReallyImportant(void)
{
    if ( isThatImportantThingOk() ) {
        return true;
    }
        return true;

    return false;
}

Now, really? The assumption many make is that the braces would have had the two goto fail; lines in the same scope, but who knows. Possibly for a cut and paste of the same line twice, but not necessarily for a code merge gone wrong.

So what could have found this? Off the top of my head: better compiler flags, code coverage tools, code reviews and better tests. If you have code that checks for bad certificates, then for the love of all that is holy have a test that provides your code a bad certificate. goto isn't harmful, cutting corners is.