Debugging: Don't Believe Everything You Read

Recently saw a debugging blog* talking about how to track down unmanaged memory leaks. Clearly this was written by someone who lives in a managed-only world. They talk about allocating a buffer in native code. They start by showing an example of what _not_ to do:

void DoSomething(int size) { char* initialString = new char[size]; // do stuff }

Clearly this is wrong. The memory is allocated, and leaked when the function exits. As a proposed fix, the author offers this solution:

void DoSomething(int size) { char* initialString = new char[size]; // do stuff delete initialString; }

This is much, much worse. Now instead of just leaking the string, you've invoked undefined behavior, and the compiler is free to do all kinds of funky things. For those that aren't familiar with C++, when you create an array with new, you must use delete[]. Freeing the memory any other way is undefined behavior. You may get lucky- the compiler may take pity on you, and fix your mistake. More likely, it won't.

If you're playing along at home, that means that our now corrected code is:

void DoSomething(int size) { char* initialString = new char[size]; // do stuff delete[] initialString; }

It's a simple rule: use the method for freeing memory that goes with the method that you used to allocate it. (malloc -> free, new -> delete, new[] -> delete[], LocalAlloc -> LocalFree, CoTaskMemAlloc -> CoTaskMemFree, etc.)

This is, however, still wrong. This is looking at the solution as a C programmer, not a C++ programmer. The C++ compiler is smart enough to generate all of the memory management code for you. You shouldn't use new, unless you have a really good reason. The best solution is to not use a raw pointer at all, and use the standard library objects to help.

void DoSomething(int size) { std::string initialString(size, '\0'); // do stuff }

Now, the compiler will generate code that cleans up initialString as soon as it goes out of scope. (In more general parlance this is RAII, or Resource Acquisition Is Initialization.) Why is this helpful? Let's say you modify 'do stuff' to return early in an error case. Did you remember to delete that string? If not, you have a leak that happens intermittently, and it's now much harder to debug.

If you do need to work with a raw pointer (like if you're programming with COM on Windows) use a smart pointer type to help you manage it. Here's a slightly more complex example:

void DoSomething() { IUIAutomation* pAutomation; IUIAutomationElement* pElement; HRESULT hr = CoInitialize(NULL); if (FAILED(hr)) return; hr = CoCreateInstance(CLSID_CUIAutomation, NULL, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&pAutomation)); if (FAILED(hr)) return; hr = pAutomation->GetRoot(&pElement); if (FAILED(hr)) { if (pAutomation) pAutomation->Release(); } // do something with pElement if (FAILED(hr)) { if (pElement) pElement->Release(); if (pAutomation) pAutomation->Release(); } // do something else with pElement if (FAILED(hr)) { if (pElement) pElement->Release(); if (pAutomation) pAutomation->Release(); } // do some more stuff if (pElement) pElement->Release(); if (pAutomation) pAutomation->Release(); return; }

With smart pointers (in this case ATL) we can simplify that code to:

void DoSomething() { CComPtr<IUIAutomation> pAutomation; CComPtr<IUIAutomationElement> pElement; HRESULT hr = CoInitialize(NULL); if (FAILED(hr)) return; hr = CoCreateInstance(CLSID_CUIAutomation, NULL, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&pAutomation)); if (FAILED(hr)) return; hr = pAutomation->GetRoot(&pElement); if (FAILED(hr)) return; // do something with pElement if (FAILED(hr)) return; // do something else with pElement if (FAILED(hr)) return; // do some more stuff return; }

We don't need to mess with releasing the pointers manually, the CComPtr object handles it for us automatically.

*No, I'm not going to name-and-shame. That's not what this post is about.