Thursday, August 24, 2006

Red Flag

A red flag. It’s a warning. An alert. An indication of danger. A notification that something is amiss. There are red flags in the code we work on and the processes we follow. But do we see them? I missed a red flag recently. It happened like this:

I had this curious bug I was trying to fix. The behavior suggested that it was most likely corrupted or uninitialized memory. That’s what intuition borne of experience was telling me, anyway. Randomly timed incorrect behavior in code that was processing a static stream of data. The input data was constant from one run to the next, the bits flowing through the code always the same, but the end result varied pretty much randomly in where and when it failed.

This suggested to me that we were processing someone else’s data or uninitialized data (which is really just someone else’s data from within the same process).

This body of C++ code was unfamiliar to me, so I found myself picking the brains of a coworker who had been around a while. In discussing the bug I found myself looking over his shoulder as he scrolled through some of the code in question, and he commented on a variable assignment that wasn’t used later in the function.

It was one of those pfft moments. “Been there, done that, seen it a million times.” A thoughtless assignment statement that someone typed in but then lost their train of thought. It looked something like this:


void fn()
{
size_t cbBase;
void* pvData;

if (get_value("base", &cbBase, &pvData))
{
store_data("base", cbBase, pvData);

size_t cbExtended;
void* pvDataExtended;

if (get_value("extended", &cbExtended, &pvDataExtended))
{
store_data("extended", cbExtended, pvDataExtended);
cbBase = cbExtended;
}
}
}


And quickly we moved on to discuss what might really be wrong with the code. And that quickly I’d dismissed the red flag.

In a world where most of the code that I interact with is not my own, where dozens of changes wrought by numerous hands happen over a period of years can I really pass off a small, unexplained assignment like that above as an innocuous error? Any moderately complex code base will transmogrify over the years. Initial errors may indeed be simple coding issues that we wish would have been corrected by code review, but over time source code changes not randomly but with specific intent. And with any luck you have both bug reports and a source code revision system on which you can rely to find that intent.

The red flag, of course, was the meaningless assignment statement. More than a day later as I waded through diffs of check-ins from ages past I ran across the rationale for the assignment. In previous check-in an attempt was made to correct some bad behavior. A previous version of the code looked more like this:


void fn()
{
size_t cbBase;
void* pvData;

if (get_value("base", &cbBase, &pvData))
{
store_data("base", cbBase, pvData);

size_t cbExtended;
void* pvDataExtended;

if (get_value("extended", &cbExtended, &pvDataExtended))
{
store_data("extended", cbExtended, pvDataExtended);
cbBase = cbExtended;
}

if (cbBase < MINIMUM_EXPECTED_DATA_SIZE)
{
backfill_missing_extended_data();
}

}
}


Ah, the unexplained assignment was orphaned by a previous check-in. In an effort to correct a particular problem a developer had removed code but left behind an ineffective assignment. Interestingly--partly because I like a tidy ending--the bug the developer was fixing was strongly related to the bug I was pursuing. The original author’s intent for the assignment, it turns out, was probably not


cbBase = cbExtended;


But


cbBase += cbExtended;


I reintroduced the missing code and patched up the assignment to find that, very conveniently, my bug was fixed as well. In the end, yes, it was incorrectly initialized data. It just wasn't where I expected to look.

Funny thing, those red flags. They’re hard to see. Where have you seen them lately? (Or not?)