Programming Evil #143

I hope everyone had a good holiday weekend but that's not why we're here. Oh no. We're here to talk about programming evils and in true rant style I'm going to number these but not in any rational way.

I've found a simple piece of code that seems benign enough (details hidden to protect the guilty):

byte buffer[ BUFFER_SIZE ];
sprintf( buffer, "map_path/%s", szGetMapName() );

Astute readers might note that if szGetMapName() happens to return a really big name or a NULL, that bad things might happen to the game. Me, in an attempt to be a good worker bee and concerned with security, decided I'd change it thusly:

byte buffer[ BUFFER_SIZE ];
snprintf( buffer, sizeof(buffer), "map_path/%s", szGetMapName() );

except that this now errors out on every overrun rather than crashing. Better, but still not what I was going for. Note that standard C snprintf does not error out or anything weird. It is, in fact, quite a well-behaved function. We've overridden snprintf to our own crazy version of snprintf that errors on buffer overflow. Our version is not quite so well-behaved.

I hereby christen this as Programming Evil #143: If you override standard functions, don't change the functionality!

In this particular case, the chance of buffer overrun is almost nil and the function played by szGetMapName() can't return NULL and the define played by BUFFER_SIZE is pretty big making it exceptionally hard to overrun it. Regardless, I wanted to make it more bulletproof--an attempt that has been quite thwarted.