Rabid howler monkeys on crack wrote this code

Tuesday, January 31, 2012

Okay, yes, there are issues with the code to The Protocol Stack From Hell™. There's the vowel impaired names—oh, sorry, the vwl_imprd_nms, the usual ignoring return codes and tons of global variables—sorry, glblvrbls littering the code. And there's stuff like:

foo_t *p = NULL;

/* lots of code not touching p at all */

if (p) {
	/* lots of code that will never be executed because */ 
	/* p is always, *always* NULL at this point         */
} else {
	/* this code will always *always* be executed */
	/* p is never touched otherwise */
}

/* p is still never used */

Yes. At one point p was probaby used, then a code change sometime during the Clinton Administration (late first term most likely) removed the need for p but later code still checked it, so in order to keep the code from crashing (during the last year of the Clinton Administration, most likely) the “easiest fix that would work with minimal code changes because we want to avoid a five day regression test” is to just NULL out the variable where declared and call it a day.

Odder yet is the code that generates a string, checks to see if the generated string ends with two newline characters and then adds one or two newline characters if required (and yes, it checks for the first newline character, then the second) and further down in the code, it checks to see if the line has two newline characters and carefully removes them, one at a time.

Yes. The code adds two characters, only to remove them later on.

Again, I can see the requirements late during the Reagan Administration bumping up against the requirements during the early Bush 43 Adminstration and again, the easiest way to handle this is a local change that distrubs as little code as possible.

Although, there is one bit that does smack of rabid howler monkeys on crack taking a pass at the code, which I briefly mention in passing. It's basically the Poster Child™ for why certain C programmers should be taken out back behind the shed and disembowled with a grapefruit spoon.

Back then, I was tasked with modifying some code to log the Protocol Stack From Hell™ errors via syslog(), and all I had to work with was a C source file:

/* tons o' broilerplate text whereby we pledge our first born to feed the
 * lawyers of The Protocol Stack From Hell™ */

void Stpd_Init_Fnctn(void)
{
  MYSTERIOUS_ENTRY_CODE_WE_CANT_TOUCH();
  /* modifications here */
  MYSTERIOUS_EXIT_CODE_WE_CANT_TOUCH();
}

void Stpd_Lrm_Fnctn(lrm_t *ptr,int wat)
{
  MYSTERIOUS_ENTRY_CODE_WE_CANT_TOUCH();
  /* modifications here */
  MYSTERIOUS_EXIT_CODE_WE_CANT_TOUCH();
}

/* Muahahahahahahahahahahahaha! */
/* [S/X thunder ] */

(No, seriously, each function starts and ends with MYSTERIOUS_something_CODE_WE_CANT_TOUCH()) and an object file, which the C code is linked against to produce the final program.

Okay, nothing that out of the ordinary. Only we weren't getting the proper error messages from the lrm_t … thingy … we were given. Some back and forth with The Protocol Stack From Hell™ Technical Support® and we had the final solution, and if you can read C code, prepared to be horrified:

void Stpd_Lrm_Fnctn(lrm_t *prt,int wat)
{
  char *msg;

  MYSTERIOUS_ENTRY_CODE_WE_CANT_TOUCH();

  msg = (char *)prt + sizeof(lrm_t);
  syslog(LOG_WTF,"error: %s",msg);

  MYSTERIOUS_EXIT_CODE_WE_CANT_TOUCH();
}

For those not fluent in C, let me translate: “you will receive a block of memory called prt, which has a particular layout we laughingly call lrm_t. Ignore the data there, but instead, what you actually want lies just past the block of memory you received, into an area that Standard C calls “undefined behavior.” Abandon all hope ye who program here. And have a nice day.”

Then, I was horrified. Now, I get to see the code from “the other side” and “horrified” does not describe my reaction. “Running away, screaming in sheer madness of having peered deep into the Abyss” would be a bit closer, but still misses the mark. It goes something like this.

typedef struct {
	/* data data data */ /* [1] */
} lrm_t;

typedef struct {
	/* a mass of data */
	arbitrary_size_t reserved[6]; /* [2] */
} msg_t;

void rnd_fnct(int wat)
{
  /* don't worry, these are big enough */
  char inbffr[256],tmpbffr[256],msgbffr[256]; /* [3] */
  msg_t *msg;
  lrm_t lrm,*plrm;

  /* a bunch of code to receive an SS7 message and determine that the */
  /* contents need to be logged, as it indicates an error */

  msg = (msg_t *)inbffr;
  frmt_rnd_msg(msgbffr,msg); /* we know how big the resulting buffer is */
  
  /* okay, now for the real horror show */
  memcpy(&lrm,msg->reserved,sizeof(lrm_t)); /* [4] */

  /* No!  Don't go into the basement! */
  memcpy(tmpbffr,&lrm,sizeof(lrm_t));	    /* [5] */

  /* The call originated from inside the house! */
  strcpy(&tmpbffr[sizeof(lrm_t)],msgbffr);  /* [6] */

  /* Aieeeeeeeeeeeeeeeeeeeeeeeee! stabbity stab stab */
  Stpd_Lrm_Fnctn((lrm_t *)tmpbffr,wat);     /* [7] */
}

And now for the play-by-play commentary on this horror show:

This describes the layout of the memory block we're given in Stpd_Lrm_Fnctn(). It's not terribly big as structured memory goes, maybe around 60 or 70 bytes, but it primarily contains useless information as I found out.
It's a slightly larger block of memory, but notice the last field, reserved. A comment in the code says that this area is for “internal use only” and is around 24 bytes in size.

Keep in mind—this field is only 24 bytes in size. The size of the block of memory we're given in Stpd_Lrm_Fnctn() is around 60 or 70, which is larger than 24. 24 is smaller than 60 and 70. This is important.
“256 bytes should be enough for anyone, right?”

This, on a system with gigabytes of memory.
This copies data out of the reserved field into a block of memory of type lrm_t. Refer back to note 2. Notice how we want to copy 60 or 70 bytes of information, but the field we're copying from is only 24 bytes. This, my friends, is known as “undefined behavior” in C.

Only, this is air quotation marks okay air quotation marks because msg_t is technically the air quotation marks header air quotation marks of a larger message and thus, we can air quotation marks safely air quotation marks copy memory past the end of the header.

My thinking here—the error codes originally fit into the space set aside by reserved but grew over time, found out, but too much code relied on this situation, so they're stuck with it.

Or, you know, rabid howler monkeys on crack.
I just hope that lrm_t doesn't ever exceed 256 bytes in size.
I'm serious. I'm not making this up. The code actually uses strcpy().

strcpy() is bad because there is no checking to see if you have overrun the space set aside to receive the copied string.

The use of this function should cause modern C compilers to bitch mightily and stop compilation right then and there, and send the programmer to jail, do not pass Go, do not collect $200.00. Especially if the programmers are rabid howler monkeys on crack.

This. Is. An. Ex. Function!
And now we call our function. Hope the error message, plus the lrm_t, didn't exceed 256 bytes.

Rabid howler monkeys on crack.

I'm serious.

There aren't enough facepalms to do this code justice.

The Boston Diaries

Tuesday, January 31, 2012

Rabid howler monkeys on crack wrote this code

Obligatory Picture

Obligatory Contact Info

Obligatory Feeds

Obligatory Links

Obligatory Miscellaneous