This article is more than 1 year old

'Boss, I've got a bug fix: Nuke the whole thing from orbit, rewrite it all'

And is this a bug or a beauty in OpenSSL?

Line Break Hello, world. Welcome back to Line Break, our weekly column of terrible code readers have spotted in the wild – think of it as a group therapy session.

Let's skip to the main() course.

Beauty or the beast

Since SSL/TLS is in the news again, let's start with some weird code spotted by Georgi in OpenSSL and LibreSSL, a fork of OpenSSL created by the OpenBSD community. It's in ssl/s3_clnt.c, specifically this block at line 984:

if (CBS_len(&cert_list) < 3)
        goto truncated;
if (!CBS_get_u24_length_prefixed(&cert_list, &cert)) {
        al = SSL_AD_DECODE_ERROR;
        SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE,
            SSL_R_CERT_LENGTH_MISMATCH);
        goto f_err;
}

and at line 1080:

if (0) {
truncated:
        /* wrong packet length */
        al = SSL_AD_DECODE_ERROR;
        SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE,
           SSL_R_BAD_PACKET_LENGTH);
f_err:
        ssl3_send_alert(s, SSL3_AL_FATAL, al);
}
err:
        EVP_PKEY_free(pkey);
        X509_free(x);
        sk_X509_pop_free(sk, X509_free);
        return (ret);

This is in ssl3_get_server_certificate() in LibreSSL 2.3.2 and OpenSSL 1.0.1p. What's happening here is that the first block of quoted code jumps to the labels truncated or f_err in the function using goto if something goes wrong; the code at these labels flag up the error, and then the execution falls through to err which does some tidying up and exits the function.

Using goto to handle failures in C code is actually not a terrible idea because it allows a function to safely back out of an operation if a problem occurs. Code at these error-catching labels can tear down and free any allocated data structures, locks can be released, and so on.

But what's odd here is that the truncated and f_err labels are wrapped in an if(0) block. When the flow of execution reaches an if(0) block, the code inside never runs because the zero evaluates to false. The code will often not be included in the final executable because the compiler's optimizer will strip it out as dead code.

But the optimizer can't strip this particular code out because truncated and f_err are being used and can be jumped to despite being in an if(0) block. Basically, they will be included.

The if(0) is just preventing the code above the block falling through into the error-raising code, and letting it instead skip to err where allocations are freed and a success code is returned as the error code.

This is either really cute or really fugly, depending on your taste. The main problem some people will have with this is the bit where the code exits f_err and thus exits a block that can't be fallen into, only jumped into.

"I am not a C expert, but this looked weird to me. I asked three C coders about it," Georgi told us. "Two of them said: 'This sucks so much.' One said it was a "very nice idea", and denied he was joking."

Lesson: There's a fine line between smart and stupid. With tricks like this, just make sure you're not doing anything that'll trigger homicidal rage in your fellow team members.

You can't spell assembly without ass

Actually, sometimes, the desire to stab someone in the head might be justified. Tom writes in with this horror story:

I was working in a team doing scientific work. I was the expert in C, and was able to figure out how to fix bugs in the source – even the really braindead problems.

I was sent to investigate the sources of a particular bloke from another team. A real sociopath who never talked to anyone. The problem was: no one, including the really sharp ones, could understand his source code, less recompile it.

First, I was totally puzzled by his code: he was only using a handful of variables, two to three, plus a global data zone like this:

char data[MAX];
char *pc1;
char *pc2;
char *pc3;

Then, every single computation would be calculated using data[X] and data[Y], with the result being stored in data[Z]. However, it was always done through a variable named pc.

That would give something like:

pc1=data;
pc2=data+1;
pc3=data+2;
*pc1=*pc2 OPERATION *pc3;

Types of any kind were ignored, like they never existed in C.

Rinse and repeat with other cells of data. Whole programs were written all in this creepy schema. After a while, I realized the guy was implementing ancient assembly language, where you load data into a small set of registers and then run operations on those registers and store them back in memory. Of course, comments were notorious for slowing down the code – sarcasm – so there was no sight of them.

The bloke obviously started with Z80 or 68000 machine code and never got past it.

This, as scary as it was, didn’t explain the inability to recompile any of this bloke’s code. See, all of his source would rely on a static library called "library", which contained frequently used functions. First, I was pleasantly surprised to see the guy use a modern approach like this.

After trying frantically to freaking compile any of his programs, and failing miserably, I decided to talk to him, as difficult as it was.

He explained to me that, for every new program, he would change every single one of the library's functions completely so they would suit the new program.

Of course, he never did any versioning of any sort, meaning that every executable program would rely on the library being in whatever state it was at the time of compilation, and would be utterly non re-compilable later on, leaving you stuck with executables that could never be rebuilt from source.

I ended-up coming back to my boss with the solution to his problem: nuke the whole thing from orbit, rewrite everything.

Lesson: Your team isn't as bad as this. Give them a hug.

Sort it out

What's the worst sorting code you've seen? Before you post in the comments, check out what reader Max sent in:

I was maintaining a purchased FORTRAN compiler for which we had the source. I was using it to develop some long-forgotten application. Getting a symbol cross-reference was a run-time option which I used sparingly, because it slowed down things noticeably.

As time went on, and the application grew, it became obvious that the more symbols, the slower it was, and it finally reached ridiculous proportions. I dug into the compiler code, and found the cross-reference sort routine. It passed through the entire symbol table once for each entry, found the next entry in alphabetical order, output the cross-reference line, marked that symbol as consumed, and went back to repeat, quitting only when it found no unconsumed symbol. It was using an O(n**2) sort!

After pounding my head against the wall for a while, I replaced it with something out of Knuth, and got on with my job. I left that job a couple of years later, and never saw anything to match those two in the decades since.

Lesson: You don't have to memorize sorting algorithms by heart (well, maybe you do if you enjoy doing whiteboard exercises during Google job interviews) but it really helps to know a bad one from a good one – and to avoid premature optimization, of course.

Please keep your tales of woe coming in; just drop us an email and we look forward to including your stories. And thanks for the hundreds of comments posted so far – we're planning to compile a best-of-the-comments-special in the next few weeks. ®

Click here to see all Line Break columns

More about

TIP US OFF

Send us news


Other stories you might like