I am going to tell you a story about an issue that brought me a rather painful debugging session, and the lessons that can be learned from it.

A few years back I was restructuring the libraries of a larger project. Restructuring means I shoved around some compilation units that were in different libs before and put them in the same lib, because they belonged together. Compilation and linking worked without problems, and since I had not touched any code, I was very sure that everything should work as before. A basic functionality test suite did not show any failures, so I checked everything in and went home.

The end.

Not quite.

The next morning I started the application and it almost immediately crashed. I started the debugger to find out where the crash originated from, in order to tell the developer who was responsible for that part.

Hours of debugging and swearing later, I had found the source of the crash. A pointer that could not possibly be touched by anything legal got altered and accessing it resulted in the crash. A data breakpoint told me it was a std::string constructor that altered the pointer. More precisely it was the construction of the third string member of an object that had only two strings and nothing else. What the…?

Examining where that constructor call originated from, I got to a code location that looked like this:

//SomeFooBarClass.cpp

class NamingHelper {
  string name_;
  string suffix_;
  string info_;
public:
  NamingHelper(string const& name, string const& suffix)
    : name_(name), suffix_(suffix), info_("default-info")  //<======!!!====
  {}
  //...
};

void SomeFooBarClass::doSomeNaming() {
  NamingHelper helper("meow", "-fix");
  //...
}

In the initialization list, the construction of info_ was what seemed to wreak havoc with some memory that did belong to another object. When I looked one call back in the stack trace, I got this:

//SomeFooBazClass.cpp

class NamingHelper {
  string name_;
  string suffix_;
public:
  NamingHelper(string const& name, string const& suffix)
    : name_(name), suffix_(suffix)  
  {}
  //...
};

void SomeFooBazClass::doSomeNaming() {
  NamingHelper helper("meow", "-fix");          //<======!!!====
  //...
}

See the difference? NamingHelper had only two members. Seconds before it had three. It took me a while to realize that I was in a different source file. The surrounding code was almost identical, as were the class names.

Cause of the Problem

Sometime in the past, there had been only SomeFooBarClass, with a two-element NamingHelper. When the very similar SomeFooBazClass was needed, someone just copy-pasted the whole class, made some minor changes and checked in, violating several rules of clean coding, like the DRY principle. He should have factored out the common behavior or generalize the existing class by adding a parametrization for the differing behavior.

Months later, someone else made a minor change to SomeFooBarClass, adding the info_ member to the NamingHelper of that class’ implementation. The two classes were in different libraries at that time, otherwise, the crash or a similar problem should have occurred back then already.

When I put both compilation units into the same lib, I unknowingly violated the One Definition Rule: I had two differing class definitions with the same name, in the same namespace, in two different compilation units. The linker does not need to warn about that, it simply can assume the class definitions are the same. Having the same class definition in different translation units happens all the time if you include the same header in more than one source file.

Having two instances of the constructor NamingHelper::NamingHelper(string const&, string const&) is not an issue for the linker, either. Both constructors have been defined inside the class definition, making them implicitly inline, and having the same inline function defined in several translation units is a linker’s daily business. It can assume that each definition is the same because the ODR says they have to be, and then picks whichever definition it wants.

In this case, it picked the constructor definition of the three-element NamingHelper. The compiler on the other hand, while compiling SomeFooBazClass.cpp did know only a two-element NamingHelper and therefore reserved only enough space for the two strings on the stack. When the three-element constructor was executed, the third string got constructed in another object’s memory, a typical case of undefined behavior.

Lessons Learned

The first thing to notice is that this error can not only occur because someone carelessly copy-pastes some code that should be refactored instead. NamingHelper is a very generic name for a helper class, and it is not very unlikely that two developers come up with the same names for their helper classes. That is not necessarily bad, although too generic names or poor names, in general, tend to obfuscate the meaning of code.

Take care when naming things. When using short, generic names make sure they don’t reach too far.

In other words, i may be OK for a loop variable, but it certainly is not for something that can be accessed globally. In this case, the generic helper class names reached out of their respective translation units, which should have been prevented. This can be achieved easily by using anonymous namespaces, which I used to solve the problem in the end.

Consider giving internal linkage to helper classes and functions, e.g. by putting them in an unnamed namespace.

In hindsight, I should have done what the copy-paste guy hadn’t done: Refactoring the two classes. Of course, one should approach refactoring in a pragmatic way, not in a dogmatic way, i.e. don’t start a huge refactoring session after you changed just two or three lines of code. But in this case, a very ugly code smell had caused me several hours of debugging, so I should have spent an hour or two to set things right. I just had not read about “Clean Code” and heard of the “Boy Scout Rule” yet.

Instead of just fixing bad code, improve it.

Last but not least there was another error I had made: I had made false assumptions. By only running a basic test suite before checking in I exposed others to an error I could have found running all unit tests.

Don’t just think that nothing can go wrong, prove it. Run all unit tests before checking in.

Yes, I mean all unit tests. But I mean only real unit tests, not integration tests or full system tests that are labeled as “unit tests”. There is a huge difference between the two, which I will write about in a future post.