Skip to content

My favorite bug

It was early November 2013, and me and a friend of mine, Helgi, were preparing to compete in our annual ACM-ICPC regional contest. We had been adding various algorithms and data structures to our code library, which we were allowed to bring with us to the contest. He had recently taken an Operating Systems course, where he had implemented a Skip list for use in a memory allocator. To my knowledge, Skip lists have not been used much in programming contests, but since it is a relatively simple (to code) data structure that supports the same operations as balanced binary search trees with the same (expected) time complexity, we thought it might be a good idea to add it to our library. (Note that C++, which was our programming language of choice, already provides a balanced binary search tree through its standard library, but it doesn’t support augmentation, which is something that is frequently needed in programming contests.)

So one evening we sat down and did just that. He found his old implementation, which was written in low-level optimized C, and we started translating it to a more usable high-level C++ implementation. When we were done, we started testing it to make sure the implementation was correct, and that we didn’t mess anything up in the translation. At first we made some small manual tests, and things looked good. This, of course, wasn’t very exhaustive, so after that we started generating large random test cases, and then compared the Skip list’s results to C++’s set. Looking at some old Github commits, I found that the program looked basically like this:

Skip list implementation, and tester

However, the program crashed every time we ran it, outputting this nasty error message:

So somewhere in our code was a bug that was causing this memory corruption. We spent a good while staring at the code looking for a possible explanation, but turned up nothing. Actually, if you want, this would be a good time to look at the code yourself, but I have a feeling you’ll reach the same conclusion.

It came to mind that maybe we made an error during the translation. So we went back to the original implementation, modified our test generator to use its low-level interface, and then ran the program again. It worked like a charm, with no errors! So now we knew that we must have made some error during the translation. So we went back to the translated code, and probably stared at it a little longer. But since that was getting us nowhere, we decided to bring out the big guns, and went on to run the program with GDB.

My friend led the way, as he had more experience with GDB. My memory is a bit fuzzy here, but this is roughly what happened. The first thing we observed was that the error was occurring in a node’s destructor, where we were freeing some memory that didn’t belong to us. Unfortunately this didn’t give us much information about why this was happening.

After spending more time in the debugger, we got our first big break. It seemed that the Skip list’s destructor was being called more than once. This would explain the odd behavior we were seeing with the node’s destructor, as well as the memory errors. And indeed, after modifying the code to output some debugging information, we verified that this was the case. But that was very strange. We weren’t calling the destructor explicitly, so the only place it should have been called was (implicitly) at the end of the program.

So we went back to staring at the code, knowing that the destructor was unexpectedly being called multiple times. All of a sudden we realize what was happening. The way I remember it, me and my friend noticed this at exactly the same time, and we just looked at each other, both speechless. But this only lasted a second before we burst into laughter, realizing how hilarious this was.

The perpetrator was the size() function. Not the Skip list’s size() function, but the size() function I had defined at the top of the code as follows:

It’s a convenience function I had defined to get rid of some warnings about comparisons between signed and unsigned integers, and we had used it a few times in the testing code. For example, when making sure that the number of elements in the Skip list (t1) was equal to the number of elements in the set (t2), we did the following:

So what exactly is wrong with the function? The issue is that the parameter x is being passed by value (which is the default mode in C++) as opposed to being passed by reference. This means that every time we call the function, passing it some object (in this case, the Skip list) the following happens:

  1. The object is copied,
  2. the copy is passed on to the function,
  3. the function executes using this copy of the object, and when the function terminates,
  4. the copy is destroyed (by calling the destructor).

So every time the test code called size(t1), it was making a copy of the Skip list using the copy constructor, and then calling the destructor of the copy.

When the copy constructor or the destructor functions are not implemented, C++ will provide a sane default implementation. And actually, if we had just used the default implementations, we wouldn’t have gotten this error. However, when the object is allocating memory (as was the case with the Skip list), one usually wants to make a custom implementation of the copy constructor to copy the actual value of the allocated memory (not just the pointer as the default implementation does), and the destructor to actually free the memory (which the default implementation does not). But we had only implemented the destructor, not the copy constructor. So when a copy of the Skip list was being made (for the size() function), the default copy constructor simply copied the pointers over. Then, when the copy’s destructor was called, it freed the memory of the pointers. And since this was the memory that the original copy of the Skip list was using, it was now unusable. But the test code continued anyway, running into lots of memory errors, and eventually crashing.

But this is not the only bad effect that the size() function could cause. Consider the following piece of code:

It creates a vector of a million integers, then iterates through the vector, setting every value to 42. Usually this would run in a few milliseconds on your average computer (4ms on my machine, actually). However, since size()‘s parameter is passed by value, the vector of a million elements is copied on each iteration of the loop. And since there are a million iterations, this will take a long time… In fact, this ran for just over 13 minutes and 38 seconds on my machine.

Anyway. Let’s shift back to the exact moment when me and my friend realize what was causing the memory error. Not only did I realize the possible effects of the size() function as it was currently written, it also dawned on me that, since this function actually came from my default C++ template that I had configured my editor to import automatically every time I created a new C++ file, this error was present in basically all C++ code I had written for a long time. A few months at least!

This was of course trivial to fix, as can be seen by this commit, and our test program ran flawlessly after fixing this.

Aftermath

Man, was I happy to have found this. And, boy, was it a great learning experience. I have been very paranoid about how functions are declared—actually any unexpected thing that could be happening underneath our implementation—since then. I hope that’s a good thing…

But I’ve thought about this experience multiple times since it happened.

Why did it happen in the first place? As I mentioned before, the reason I started using this size() function was to get rid of a compiler warning. In C++’s standard library most containers’ size() function returns an integer of the type size_t, which is an unsigned integer. So if you compile something like the following piece of code:

the compiler will give you a warning about a comparison between a signed and an unsigned integer. Seeing a ton of these warnings will get tiring pretty quickly. Another related issue is the following. Say you want to iterate through all but the last element of a container. You might implement this as follows:

But this code will not work correctly when the container is empty. Since v.size() returns an unsigned integer (of value zero in this case), subtracting one will underflow, giving the largest representable value of size_t instead of the expected -1. This, in turn, will result in an infinite loop.

An obvious simple fix to resolve both of the above issues is to cast the result to an integer, as follows:

But since one writes such for loops so frequently, it would be pretty annoying to write this every time. Another way, which I think is quite common in the competitive programming community, is to define something like the following macro:

and then use it as follows:

I had probably seen this macro a few times when I was starting out doing competitive programming, and at some point I decided to make my own version. I thought SZ looked ugly, and I was more used to typing size, so I wanted to keep on doing that. But creating a macro with the name size would have been a bit dangerous as size is a common variable name, which would cause trouble. So I went the other way and created the following function instead of a macro:

I’m not sure why I didn’t make the parameter pass-by-reference, but I’m sure I knew what the difference was before I started doing competitive programming (and hence, before I wrote this function). But it’s an easy mistake to make, even by an experienced coder who isn’t using his full brain power when implementing such a seemingly trivial function.

I’ve also wondered why I didn’t run into issues earlier. One contributing factor was probably that I was inexperienced, and didn’t know how fast I should expect such loops to run. Another contributing factor might also have been the fact that competitive programmers usually don’t have to be concerned with writing copy constructors and destructors. Usually we just let the C++ runtime free all memory when the process terminates. At least I’m sure that none of the data structures I had in my library by the time we found the bug implemented these constructors.

At some point I tried pinpointing when I started using this function. I looked at my submission history on TopCoder and Codeforces. On TopCoder, I found that I was not using this function in the contest on October 26th, 2011 (Unfortunately you have to log in to TopCoder to access the link). However, I was using the function on November 12th, 2011. Finding this left me devastated. This bug was present in all my programming contest solutions from November 12th, 2011 till November 3rd, 2013… That’s the first two years of my competitive programming career! And there’s no telling how much trouble this caused me.

As an example, I found this submission I made to the first problem of the second contest I participated in on Codeforces. I tried to solve the first two problems, but both of my solutions turned out to be too slow. However, for my submission to the first problem, by simply adding that single letter (&) to make the size() function pass-by-reference, the submission is accepted. I can’t help but feel bad for my old self, but at the same time I’m laughing so hard.

Recently I was taking a second look at a problem I couldn’t quite solve some years ago, although I had tried implementing a solution. I read the problem and came up with a solution. As far as I could remember, it was the same solution I thought of the first time. Being a little bit lazy, I found my old code instead of implementing everything from scratch. I looked over the code, and it seemed fine. So I submitted it, but it turned out to be too slow. I spent a good amount of time looking over the code, but didn’t understand why it was so slow. But all of a sudden, I froze. There it was! That old, broken, size() function… I added the &, and resubmitted. Accepted!

Even though I thought I had fixed the bug many years ago, it still hunts me to this day!

Published inCompetitive ProgrammingStories

5 Comments

  1. Trass3r Trass3r

    Unfortunately you still don’t consider this a bad idea in the first place.
    Your workaround wouldn’t even have compiled with -Werror=conversion.
    The lesson for readers should be warnings aren’t there to nag you. They are evidence of bad language design inherited from C and must be taken seriously (cf. how C# just forbids most of these implicit conversions).

    • I’m not sure if “consider this a bad idea” refers only to the implicit cast, or to other things as well. I’ll address the implicit cast here, but feel free to let me know if you were referring to other things as well.

      The reason for the -Wconversion warning is to warn the programmer about an implicit conversion they didn’t intend to do. And of course, this can be very dangerous when done by accident. However, I very much made an informed decision to cast the result to a 32-bit signed integer.

      The reason I didn’t make this an explicit cast was simply to save a few characters, which I would otherwise have to type in every time I wanted to use my template (in a contest). In fact, the only reason I see for making the cast explicit (and I have a feeling you won’t like this), is to get rid of the -Wconversion warning. That way, if I were to start using the -Wconversion flag during contests, I would only be warned about casts I could have made by accident.

      But please keep in mind that this is only my opinion in the context of competitive programming.

      • Trass3r Trass3r

        I meant creating the function, often you can just make the loop variable size_t and you’re done 🙂
        Of course sometimes you can’t, e.g. for OpenMP 2 loops.
        The size-1 example is compelling. max(x, 1) – 1 is not so elegant. i + 1 < size? Not sure.

        But Sean Parent would still go for "no raw loops" 😉
        http://rextester.com/MFH77611

  2. Kitihounel Kitihounel

    How to Shoot Yourself In the Foot (https://www-users.cs.york.ac.uk/~susan/joke/foot.htm)

    C++
    You accidentally create a dozen instances of yourself and shoot them all in the foot. Providing emergency medical care is impossible since you can’t tell which are bitwise copies and which are just pointing at others and saying “that’s me, over there.”

    I think that joke summarizes the situation.
    Now, it is a little oversight that causes all this pain. If you want a more sneaky bug, read this: http://www.elpauer.org/2011/08/the-most-stupid-c-bug-ever/

Leave a Reply

Your email address will not be published. Required fields are marked *