Line break Shellshock. Heartbleed. That CCTV storage firmware with a hardcoded password. We've all seen some really bad code.

Maybe that's just me.

Given that many of our sysadmin readers have poured in tales of fixing impossibly broken servers for our On-Call series, we know our software-wrangling readers have faced similar battles with hysterical, broken and/or outright dangerous code.

We'd love to feature your tales of hunting and fixing bugs, or encountering and overcoming ridiculous feats of software engineering. If you want to share – no matter how grand or small the cockup you've spotted – drop your humble hack an email with the details, or message him on Twitter. Sources are held in the strictest confidence. Names, functions, and variables can be changed to protect the innocent.

El Reg would like to stress that this is not an exercise in shaming particular engineers – it's an opportunity to share, wince, and learn collectively to avoid the same mistakes.

To kickstart things, let's begin with one terrifying bug, two classics, and one final example from personal experience.

Valve's Steam runs rm -rf / on Linux clients

# figure out the absolute path to the script being run a bit # non-obvious, the ${0%/*} pulls the path out of $0, cd's into the # specified directory, then uses $PWD to figure out where that # directory lives - and all this in a subshell, so we don't affect # $PWD STEAMROOT="$(cd "${0%/*}" && echo $PWD)" # Scary! rm -rf "$STEAMROOT/"*

Once upon a time, if you moved your Steam user directory and then launched the Steam client, the above code would be run with $STEAMROOT undefined. So the program would run rm -rf / as the logged-in user, and thus wipe all of his or her files from the computer's file system. Lesson: Never ever run rm -rf without sanity checking the arguments.

Debian's knackering of OpenSSL key security

--- openssl/trunk/rand/md_rand.c 2006/05/02 16:25:19 140 +++ openssl/trunk/rand/md_rand.c 2006/05/02 16:34:53 141 @@ -271,7 +271,10 @@ else MD_Update(&m,&(state[st_idx]),j); +/* + * Don't add uninitialised data. MD_Update(&m,buf,j); +*/ MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c)); MD_Final(&m,local_md); md_c[1]++; @@ -465,7 +468,10 @@ MD_Update(&m,local_md,MD_DIGEST_LENGTH); MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c)); #ifndef PURIFY +/* + * Don't add uninitialised data. MD_Update(&m,buf,j); /* purify complains */ +*/ #endif k=(st_idx+MD_DIGEST_LENGTH/2)-st_num; if (k > 0)

In an attempt to avoid nagging warnings about the use of uninitialized data by Valgrind, a Debian GNU/Linux developer commented out some code in vital encryption library OpenSSL. That commented-out code happened to seed a crucial random number generator used for cryptographic key generation. That meant SSH and SSL server keys on Debian-powered systems were generated from the process ID of the calling code, which is so, so cryptographically weak, it'll make your head spin.

Everyone had to regenerate their encryption keys as a result. Lesson: Never ever touch cryptography code unless you're a level 10 Cryptographer. Certainly, never fsck with crypto code if a build tool like Valgrind whines at you: ask an adult.

Apple's cut'n'paste blunder – goto fail

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail; goto fail;

Backdoor, cockup, whatever you want to call it: an Apple engineer was moving code around a source code file and seemingly screwed up a cut'n'paste operation so that two goto fail; lines appeared under an if statement. This meant that even if the test case was successful, the code would still bail out anyway – and by bail out, we mean, bail out of checking a server's authenticity. The upshot of this typo was granting malicious systems the ability to masquerade as online banks, webmail providers, and so on, to steal people's usernames and passwords.

Lesson: If Apple can't get this right, we're never getting in a fabled Apple Car. Apply regression testing to key components, like, say, the fucking security libraries.

SWAP SWAP SWAP SWAP SWAP SWAP SWAP SWAP

static void swap_screens (struct session_struct *session) { int *main_area, *alt_area; int loop; #define SWAP { int swap = *main_area; *main_area++ = *alt_area; *alt_area++ = swap; } main_area = (int *) session->assigned_area + session->terminal_size.x * session->scrollback; alt_area = (int *) session->alternate_area; loop = session->terminal_size.x * session->terminal_size.y; if (loop & 1) { SWAP } if (loop & 2) { SWAP SWAP } if (loop & 4) { SWAP SWAP SWAP SWAP } loop &= ~7; while (loop) { SWAP SWAP SWAP SWAP SWAP SWAP SWAP SWAP loop -= 8; } }

SWAP SWAP! Back in the day, your humble hack was debugging an ARM-targeted terminal emulation program that was attempting to implement alternate screens: escape codes 1047 and 1049 would switch between two screens within a terminal, so data needed to be exchanged. SWAP SWAP SWAP SWAP! This was the resulting code. I still chuckle to this day thinking about it.

Lesson: If it smells bad, it's probably bad. See: Oddball Solution.

Tell us your coding horrors and the lessons to learn, and we'll make a regular series out of your collective pain wisdom. ®

Bootnote

Back in the day, your humble hack was hired by a small UK biz to reverse-engineer a rival's product. It turned out that product was a box that held a hard drive, and CCTV cameras streamed video data to it to be recorded. You couldn't plug any old drive in there – it had to specially formatted for the firmware to grok. Luckily, upon studying the extracted firmware, it turned out that hooking a serial port to the hardware and typing the password "wittenseer" over the serial channel opened up a factory-only menu from which you could initialize any drive you wanted, view an existing drive, dump its video contents, etc.

Click here to see all Line Break columns