What’s Wrong With This Code?


I meant to include this in my post about Building a DNS Resolver, as it is a small, yet significant bug that took me a little while to find. What the code should be doing, is reading a string from a file (where all strings in the file are guaranteed to be 255 characters or less), then pushing that string into a Queue. So before you jump to the whole article, can you see what’s wrong with the code?

What tipped me off was that, after reading in a bunch of IP addresses, I would grab the first one, parse it using strtok, do whatever with that, then grab the next string, parse it, etc.

The thing is, whenever I would grab the second string, instead of being something like 127.0.0.1 it would be 126, which was invalid. The third string would also be 126, as well as the fourth, fifth, etc.

Initially I thought the problem was with my parsing, then the problem was with how I grabbed the next string from the Queue. Eventually I traced it back to this bit of code; where I was reading and inserting each item into the Queue. Then I had one of those *slaps his forehead* moments where I couldn’t believe I had made such a simple mistake. Figured it out yet?

The code instantiates a pointer to a string, allocates memory, then reads that string in. Then it inserts the POINTER into a Queue. Each subsequent iteration overwrites the data held in the memory the pointer points to, and the same address is constantly inserted into the Queue. Therefore a change on one item of data changes the entire Queue of items.

The first string I would read in would be something like 126.0.0.5, which is then tokenized by periods, leaving the pointer to finally point to only 126. Thing is, as that changed the whole container, every subsequent IP was invalid.

Here’s the really simple fix to the code:


Just allocating new memory for the pointer on each iteration will ensure the pointer points to separate memory addresses each time it is inserted into the Queue. Therefore, new string won’t overwrite old ones, and you don’t have to worry about accidentally ruining the whole container at once!

Advertisements

Tags:

One Response to “What’s Wrong With This Code?”

  1. Thomas Says:

    Unfortunately you did not show how Q is defined. If it was queue you would not have had this bug. Because std::string takes a copy of null terminated char* string.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s


%d bloggers like this: