Current version

v1.10.4 (stable)

Navigation

Main page
Archived news
Downloads
Documentation
   Capture
   Compiling
   Processing
   Crashes
Features
Filters
Plugin SDK
Knowledge base
Contact info
 
Other projects
   Altirra

Archives

Blog Archive

"Fixing" working code

One of the lessons you eventually learn the hard way as a programmer is never to "fix" working code. If you have a confirmed bug, then fix it; if you have a feature you need to add, then by all means, change it.

"Fixing" code that just looks buggy, without actually knowing how it is bad, is a mistake.

Let's say you have an 80% chance of getting code right in the first place, and thus a 20% chance of getting it wrong. That means if you just look at it again, you have a 64% chance of verifying it right again, a 16% chance of writing it incorrectly but catching the bug, and a 4% chance of blowing it both times. That leaves a 16% chance of writing it correctly and thinking it's wrong. There's then a chance that you'll act on that instinct and break working code.

Here's a real example that I just discovered:

A certain vendor's C runtime library has code to display an error dialog under certain circumstances, such as an assert, with a program path. If the program path is too long, it truncates it and only displays the end of the path with an ellipsis at the start. This was done via strncpy():

if (strlen(path) > MAXLENGTH) {
    path += strlen(path) - MAXLENGTH;
    strncpy(path, "...", 3);
}

In a later version of the product, someone noticed the use of strncpy() and replaced it with strncpy_s():

if (strlen(path) > MAXLENGTH) {
    path += strlen(path) - MAXLENGTH;
    strncpy_s(path, "...", 3);
}

Now, I'm not really a fan of the secure string library -- I'd rather just use a string class instead -- but strncpy() is admittedly a bit risky to use given its tendency to create non-null terminated strings. You can use strncpy() safely, but it's easy to forget or goof the boundary check. Safer alternatives like strlcpy() are often used instead. At first glance, the change looks like a perfectly good improvement to eliminate any potential buffer overrun mistakes... except for two problems. One, the source is a string literal and the destination is a fixed buffer that is far larger than four characters, so there was never any issue with non-termination. Second, the original code actually depended on strncpy()'s non-terminating behavior, because it is putting the ellipsis at the beginning of the string. The attempt to fix a non-existent bug actually broke the code.

So, basically, don't guess at bugs. If you think there is a bug, reproduce it first so you can actually confirm it and verify that you actually fixed something instead of breaking it, and then keep that case around for regression testing.

Comments

This blog was originally open for comments when this entry was first posted, but was later closed and then removed due to spam and after a migration away from the original blog software. Unfortunately, it would have been a lot of work to reformat the comments to republish them. The author thanks everyone who posted comments and added to the discussion.