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.