CodeSOD: A Short Trip on the BobC

More than twenty years ago, BobC" wrote some code. This code was, at the time, relatively modern C++ code. One specific class controls a display, attached to a Thingamobob" (technical factory term), and reporting on the state of a number of Doohickeys", which grows over time.

The code hasn't been edited since BobC's last change, but it had one little, tiny, insignificant problem. It would have seeming random crashes. They were rare, which was good, but crashing software attached to factory equipment" isn't good for anyone.

Eventually, the number of crash reports was enough that the company decided to take a look at it, but no one could replicate the bug. Johana was asked to debug the code, and I've presented it as she supplied it for us:

class CDisplayControl{private: std::vector<IDoohickey*> m_vecIDoohickeys; std::map<short, IHelper*> m_vecIHelpers; short m_nNumHelpers;public: AddDoohickey(IDoohickey *pIDH, IHelper *pIHlp) { // Give Helper to doohickey pIDH->put_Helper(pIHlp); // Add doohickey to collection m_vecIDooHickeys.push_back(pIDH); pIDH->AddRef(); int nId = m_vecIDooHickeys.size() - 1; // Add Helper to local interface vector. This is really only done so // we have easy/quick access to the Helper. m_nNumHelpers++; m_vecIHelpers[nId] = pIHlp; // BobC:CHANGED pIHlp->AddRef(); // Skip deadly function on the first Doohickey. if (m_nNumHelpers > 1) { CallThisEveryTimeButTheFirstOrTheWorldWillEnd(); } }}

I'm on record as being anti-Hungarian notation. Wrong people disagree with me all the time on this, but they're wrong, why would we listen to them? I'm willing to permit the convention of IWhatever for interfaces, but CDisplayControl is an awkward class name. That's just aesthetic preference, though, the real problem is the member declarations:

 std::vector<IDoohickey*> m_vecIDoohickeys; std::map<short, IHelper*> m_vecIHelpers;

Here, we have a vector- a resizable list- of IDoohickey objects called m_vecIDoohickeys, which is Hungarian notation for a member which is a vector.

We also have a map that maps shorts to IHelper objects, called m_vecIHelpers, which is Hungarian notation for a member which is a vector. But this is a map. So even if Hungarian notation were helpful, this completely defeats the purpose.

Tracing through the AddDoohickey method, the very first step is that we assign a property on the IDoohickey object to point at the IHelper object. Then we put that IDoohickey into the vector, and create an ID by just checking the size of the vector.

We also increment m_nNumHelpers, another wonderfully Hungarian name, since n tells us that this is a number, but we also need to specify Num in the name too.

It's important to note: the size of the vector and the value in m_nNumHelpers should match. Then, based on the id, we slot the IHelper object into our map. This is done, according to the comment, so we have easy/quick access to the Helper".

Keep in mind, we just assigned the IHelper instance to a property of the IDoohickey, so we already have quick/easy" access. Quicker, because these are Standard Template Library classes, and while the STL is a powerful set of data-structures, back then speed wasn't really one of its attributes.

Also, note that BobC didn't trust source control, which isn't unreasonable for that long ago, but for only one of the lines changed. Though the tag, CHANGED" doesn't really give us much insight into what the change was.

Finally, we use than m_nNumHelpers to see if we've run this method at least once, because there's a step that should only happen when we have more than one IDoohickey and IHelper combination. As Johana's corrections" to the code make clear- if we call this at the wrong time, the world will end. We can't call it the first time through, but we must call it every other time through.

Which, if you carefully check the variable declarations, you'll catch the root cause of the seemingly random crashes:

short m_nNumHelpers;

In Johana's world, shorts are 16 bit integers. As these are signed, that means after it hits 32,767, it overflows and wraps back around to negative. So m_nNumHelpers > 1 becomes false, and we stop calling that method which we must call or the world will end.

Most of the time, the equipment gets power-cycled long before they hit the 32,767 invocations of this method, which is why this was so tricky to debug.

Speaking of tricky to debug," there's one more thing I see lurking in here, which based on what I saw in this method, makes me worry. As we know, BobC isn't super keen on counting, but we see calls to AddRef() in this code. I don't know, but I suspect that BobC implemented his own reference counting garbage collector.

Real garbage collection, of course, would be to completely refactor this code.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!