Don’t point, it’s impolite

Before starting this I’ll post some code that gave me the headache.

It isn’t complex code, just a simple Win32 window class wrapper for the [surprise, surprise] window using WIN32 API.
class Win32
{
  public:
    void Create(std::string name);
    void Destroy();
  private:
    WNDCLASSEX mWindowClass;
    HINSTANCE mInstanceHandle;
};


void Win32::Create(std::string name)
{
  mWindowClass.cbSize = sizeof(WNDCLASSEX);
  mWindowClass.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
  mWindowClass.lpfnWndProc = Tool::WinProc;
  mWindowClass.cbClsExtra = 0;
  mWindowClass.cbWndExtra = 0;
  mWindowClass.hInstance = mInstanceHandle;
  mWindowClass.hIcon = LoadIcon(nullptr, IDI_APPLICATION);
  mWindowClass.hCursor = LoadCursor(nullptr, IDC_ARROW);
  mWindowClass.hbrBackground = (HBRUSH)(COLOR_WINDOW+1);
  mWindowClass.lpszMenuName = nullptr;
  mWindowClass.lpszClassName = name.c_str();
  mWindowClass.hIconSm = LoadIcon(nullptr, IDI_APPLICATION);
  RegisterClassEx(&mWindowClass);
}


void Win32::Destroy()
{
  UnregisterClass(mWindowClass.lpszClassName, mInstanceHandle);
}


I’ve omitted some tests and some other bits which aren’t really important for this post. But the gist of it is there, and it’s mostly fairly trivial.
Can you stop my one, HUGE, mistake there?

Run this code and watch it go up in smoke. The UnregisterClass() function in the Destroy() method will break and quite badly, in fact when I ran it, it reported that the class in question doesn’t even exist. But, but, I’ve registered it, I know I did and the RegisterClass() function worked quite fine, nothing broken there, so, what’s the problem??

A bit of debugging later and I found that the mWindowClass.lpszClassName in the Destroy function contains garbage. Apparently somewhere betweeen calling Create() and Destroy() something bad happened to the lpszClassName [have I mentioned how much I dislike this naming convention??]

Bit more of debugging, running through the breakpoints and examining the contents of the memory and…

Pointers…. Who the hell had the brilliant idea to use a naked char* for the window class name in the WNDCLASSEX [that’s WIN32 API for you Linux, Apple, or other OS people other there].

That should’ve given you the answer by now, if not, then here it goes.

When we call the Create function it generates the name string temporary on the stack and continues on. Then at some later point we’re assigning the name.c_str() to the mWindowClass.lpszClassName – this is a pointer, it’s not a true assignment of values but rather the char* is now pointing to the beginning of the temporary array of characters that is name. Everything works fine inside the Create, we finish what we have to do and leave the function. Woosh! The function temporaries [being temporary] get deleted, erased and/or filled with garbage and the mWindowClass.lpszClassName is now pointing at, GARBAGE!

Yey, found the bug; argh, should have been more careful with my pointers. Fixing it isn’t hard, just introduce a string variable in the Win32 class to hold the name through the life-duration of the class itself.

I should have done it this way originally but I thought I’d be smart about things and save some memory. The moral of the story is, as usual, code correctly, make sure everything is expressed properly and stop trying to be too smart for my own good.

It’s been a long December and there’s reason to believe next year will be better than the last…