Steven Engelhardt

Today I was debugging some strange behavior of some code on my machine where a graph was randomly not displaying any data. I managed to track the bug down and it was quite amusing on many levels. Let me explain how it worked:

As a part of the data retrieval process, the code populated a SAFEARRAY of VARIANTs containg BSTRs. This SAFEARRAY had 6 members. There was then a section of code to make debugging easier which looked like:

#ifdef _DEBUG
    SAFEARRAY* psa = NULL;
    VARIANT* vArray = NULL;

    psa = varPlotData->parray;
    hr = SafeArrayAccessData(psa, (void**)&vArray);
    short nIndex(0);
    CComBSTR bstr1 = vArray[nIndex].bstrVal;
    CComBSTR bstr2 = vArray[++nIndex].bstrVal;
    CComBSTR bstr3 = vArray[++nIndex].bstrVal;
    CComBSTR bstr4 = vArray[++nIndex].bstrVal;
    CComBSTR bstr5 = vArray[++nIndex].bstrVal;
    CComBSTR bstr6 = vArray[++nIndex].bstrVal;
    CComBSTR bstr7 = vArray[++nIndex].bstrVal;
    CComBSTR bstr8 = vArray[++nIndex].bstrVal;
    SafeArrayUnaccessData(psa);
    VariantClear(vArray);
#endif

Even a non-Windows programmer should be able to see that since this piece of code accesses a non-existant 7th and 8th member of the array, there is a good chance that the access will dereference an invalid pointer which will generate an exception. Apparently someone had noticed that problem and decided to wrap the debug code block with a try { } catch (...) { return hr; } statement that would silently catch the exception and return what would inevitably be a success code (as hr was set to the value from SafeArrayAccessData() which is virtually guaranteed to succeed). Obviously, I strongly recommend against blindly catching all exceptions and returning what could possibly be a success code.

However, it turns out that the graph would only display if an exception was thrown, which happens randomly! The reason? VariantClear() was being called on an item which isn’t a VARIANT you should free — it is a pointer to an array of VARIANTs returned by SafeArrayAccessData()! The VariantClear() was destroying the first VARIANT in the array, and the function which later tried to read this value would fail, preventing the graph from displaying. If an exception was thrown, VariantClear() wasn’t called, and the graph drawing function was able to function normally!

The correct solution is to use SafeArrayGetLBound and SafeArrayGetUBound to determine the bounds of the SAFEARRAY, restructuring the debug code accordingly, and to remove the call to VariantClear(), as SafeArrayUnaccessData() is sufficient.