CodeSOD: Remove This

Denae inherited some 90s-era GUI code. The original developers have long since gone away, and the source control history has vanished into oblivion. In the 90s, the Standard Template Library for C++ was still very new, so when Denae needs to debug things in this application, it means doing some code archaeology and picking through home-brewed implementations of common data structures.

Denae's most recent bug involved understanding why some UI elements weren't updating properly. The basic logic the application used is that it maintained a List of GUI items which need to be repainted. So Denae dug into the the List implementation.

template <class Type> void List<Type>::Remove(Type t){int i;for (i=(num-1); i>=0; i--){if(element[i] == t){break;}}LoudAssert(i>=0);DelIndex(i);}template <class Type> void List<Type>::DelIndex(int i){LoudAssert(i<num);num--;while(i<num){element[i] = element[i+1];i++;}}

Let's start by talking about LoudAssert. Denae didn't provide the implementation, but had this to say:

LoudAssert is an internally defined assert that is really just an fprintf to stderr. In our system stderr is silenced, always, so these asserts do nothing.

LoudAssert isn't an assert, in any meaningful way. It's a logging method which also doesn't log in production. Which means there's nothing that stops the Remove method from getting a negative index to remove- since it loops backwards- and passing it over to DelIndex. If you try and remove an item which isn't in the list, that's exactly what happens. And note how num, the number of items in the list, gets decremented anyway.

Denae noticed that this must be the source of the misbehaving UI updates when the debugger told her that the list of items contained -8 entries. She adds:

We have no idea how this ever worked, or what might be affected by fixing it, but it's been running this way for over 20 years

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!