How to shoot yourself in the foot with PC-Lint

A lot of the software we use at work is provided by a software contractor (who shall remain nameless). Somewhere in their software there's a piece of code to reverse bytes in a byte buffer. It looked something like this:

static void swap(char *buf, int n)
{
    char tmp;
    for (int i = 0; i < n / 2; i++) {
        tmp = buf[i];
        buf[i] = buf[n - 1 - i];
        buf[n - 1 - i] = tmp;
    }
}

So, a perfectly ordinary bit of boilerplate code. What it does is, it takes a character buffer <buf>, which contains <n> bytes, and it reverses it: the first and the last bytes are swapped, then the second and the second-to-last bytes, etc. If you sketch the byte buffer on a piece of paper, it soon becomes obvious that the indexes of the bytes to be swapped are "i" and "n - 1 - i". So far, so good.

But our friends, being the conscientious and quality-oriented lot that they are, decided to run PC-Lint (a version of the ancient and venerable lint tool) on it, to see if it had any comments. Well, it did. It took a look at those "n - 1 - i" expressions and said:

error 834: (Info -- Operator '-' followed by operator '-' is confusing.  Use parentheses.)

"Use parentheses?", our friends said, "Sure, we can use parentheses". And they did:

static void swap(char *buf, int n)
{
    char tmp;
    for (int i = 0; i < n / 2; i++) {
        tmp = buf[i];
        buf[i] = buf[n - (1 - i)];
        buf[n - (1 - i)] = tmp;
    }
}

Four little keystrokes. And with it, a pretty nasty bug.

(In case it's not clear: "n - (1 - i)" is not equal to "n - 1 - i", it is equal to "n - 1 + i". The destination of the swap starts out at the end of the buffer and then goes right as i is increased, immediately walking off the end of the buffer.)

Software development tools like lint are great. They can point out the places where your code is maybe not as good as you think. But you have to use them wisely. Software development tools are not additive. They do not add smartness to a person, so that everyone who uses them gets a little smarter. Instead, they multiply the smartness that is already there. They make smart people smarter, but they make dumb people dumber.

Some notes:

I've not been completely honest. The expression did not change from "n - 1 - i" to "n - (1 - i)", but from "n - i - 1" to "n - (i - 1)". I've changed it to highlight the problem, and to make it clearer what it was supposed to do. And the difference is one of quantity, not quality. The buffer overrun is still there, it's just not quite as bad.

Surprisingly, the bad version of this function didn't trigger any of our copious automated tests, and after fixing it nothing seems to have changed. So I can only assume this function is dead code, unused and unloved.

Also, I cannot claim to have discovered this bug. It was actually flagged by GCC, saying "error: array subscript is above array bounds". It is pretty amazing that it spotted that. I can only assume that, by some magical combination of function call inlining and loop unrolling it managed to reduce that "n - (i - 1)" expression to a constant, and then found that it accessed memory that it shouldn't access. GCC is clearly a pretty nifty bit of kit.

That's a lot.

We were talking at work the other day about the transition (any day now!) from IPv4 to IPv6. One colleague said that at least we would have plenty of addresses, and another said something like "Ha! That's what they said about IPv4!" Well, yes. But the jump from 32-bit ...

JaccoVanSchaik dot Net

Other articles

That's a lot.