Monday, September 27, 2010

Here's a bug that I recently came across. I didn't figure it out, and it wasn't code I wrote, but it is an example of gratuitously bad code.

Originally, I had written some code for a quick-and-dirty demo that went something like:
    packetType = readPacket(input, buffer);
switch (packetType) {
case TYPE1: parseType1(buffer); break;
case TYPE2: parseType2(buffer); break;
...
}

where readPacket() read the appropriate number of bytes into the buffer depending on the packet type. Something like:
   packetType = input.read();
if (packetType == TYPE1) {
read some stuff into buffer;
} else {
count = input.read();
read count bytes into buffer;
}
return packetType;


Somewhere along the line, in a change in source control that was something along the lines of "update to the latest protocol", there were a number of changes, including gratuitously changing the code from something like the above, to something like:
    packetType = readPacket(input, buffer);
switch (packetType) {
case TYPE1: read more stuff from input; break;
case TYPE2: parseType2(buffer); break;
...
}

and readPacket() was changed to
    packetType = input.read();
if (packetType == TYPE1) {
return packetType;
} else {
count = input.read();
read count bytes into buffer;
}
return packetType;

which all would have worked fine, even though it was a stupid and gratuitous change, except that there was another place that called readPacket(), but ignored the packet. So, if that other place called readPacket() and got TYPE1, the packet wouldn't be fully read, and subsequent calls to readPacket() would return garbage. I don't know what the thinking was behind changing the code like that.

No comments:

Post a Comment