Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 19 - the answer

What's wrong with this code, part 19 - the answer

  • Comments 6

In yesterday's post, I asked about a relatively simple piece of code that tried to use the COM type conversion logic to determine if a parameter to a function is an integer index or string key.

Here's the code, for reference:

HRESULT GetValue(VARIANT Index, VARIANT& Value)
{
    // Try Index as offset
    HRESULT hr = VariantChangeType(&Index, &Index, 0, VT_I4);
    if(SUCCEEDED(hr))
        return GetValue(Index.lVal, Value);

    // Try Index as key
    hr = VariantChangeType(&Index, &Index, 0, VT_BSTR); 
    if(SUCCEEDED(hr))
        return GetValue(Index.bstrVal, Value);

    // Bad Index
    return E_INVALIDARG;
}

This function was called as:

void SomeFunction()
{
    CComVariant Value;
    GetValue(CComVariant(_T("1")), Value);
}

The problem occurs on the call to VariantChangeType in GetValue - on this example, VariantChangeType calls SysFreeString on Index.bstrValue and sets Index.intValue to 1.  When the call returns to the invoker, the destructor for the CComVariant attempts to call SysFreeString on its Index variable, and that results in a double free.

When I originally thought about the problem, I thought that the problem was with the invocation.  I thought that the problem was that the CComVariant didn't have a copy constructor that implemented a deep copy, since the default rule for C++ is to perform a shallow copy.  But that's not the root of the problem.

You can easily see this if you rewrite the invocation without using C++ at all:

void SomeFunction()
{
    VARIANT Value, CallersIndex;
    VariantInit(&CallersIndex);
    CallersIndex.vt = VT_BSTR;
    CallersIndex.bstrVal = SysAllocString(_T("1"));
    GetValue(CallersIndex, &Value);


    VariantClear(&CallersIndex);
}
 

There's no copy constructor, because this is straight C code (there are some type issues with the Value parameter).  C uses a shallow copy, so it will allocate a temporary variable for Index on the stack and perform a bitwise copy of the members of Index onto the temporary on the stack.  And the exact same problem will happen once again - the GetValue routine will once again free the value in the Index parameter (which points to the same memory as in the CallersIndex variable (it was a shallow copy, remember)).

The real root cause of the problem is in the GetValue function:

    // Try Index as offset
    HRESULT hr = VariantChangeType(&Index, &Index, 0, VT_I4);
    if(SUCCEEDED(hr))
        return GetValue(Index.lVal, Value);

The problem is that the VariantChangeType call uses the same Index variable for the VariantChangeType call.  Now the documentation for VariantChangeType says that it's ok to call the API in place ("If this is the same as pvarSrc, the variant will be converted in place").  But an in-place conversion of a string to an integer will result in the string being freed, leading to the root problem.

Since there's no action that the caller can possibly take to resolve this, the onus is on the GetValue function to fix the problem - functions that take by value parameters MUST NOT modify those parameters in externally visible ways (unless the modification is explicitly spelled out by the API contract).  When the value parameter to a function is a simple data type, this is obvious, but when it is a complex type (like VARIANT), it can become complex (as in this example).

 

Kudos:

Tzagotta initially went off on the wrong tangent, but on the 2nd try, he/she nailed the problem.

Adrian sort-of caught it, and Igor Tandetnik noticed that it was ok to call VariantChangeType in-place.

ChrisR pointed out the issue with shallow vs. deep copies.

And Michael G pointed out that ultimately this was a contract issue.

  • This can get really nasty if the caller passes an IDispatch (implementing an DISPID_VALUE) as the Index. In this case VariantChangeType will do an invalid release of the interface, but it won't crash immediately, and you'll have a double-release to debug.

  • Oooh, yech.

  • > CallersIndex.bstrVal = SysAllocString(_T("1"));

    Bug.  This differs from the previous day's code

    >  GetValue(CComVariant(_T("1")), Value);

    CComVariant has constructors for both Unicode strings and ANSI strings.  In an ANSI environment you're wasting execution time but it will work.

    In Win32, SysAllocString doesn't have an override for ANSI strings.  In an ANSI environment you shouldn't even be able to compile.  (Though I'm being lazy and not experimenting to see if MSDN is telling the truth this time.)

    I wonder if this needs a quiz or not.  When you need L"1" why should you code L"1".  When you need _T("1") why should you code _T("1").  When you need "1" why should you code "1".

  • What about the BSTR leaked from:

    VariantChangeType(&Index, &Index, 0, VT_BSTR);

  • Sark, I'm not sure that the 2nd one leaks - if you change a BSTR to a BSTR, I would expect VariantChangeType to be a NOP.

  • I know I'm late to the party on this, but not only should you not use Index as the destination in VariantChangeType(), but you shouldn't use it as the source either.

    In addition to saying "If pvargDest is the same as pvarSrc, the variant will be converted in place" the documentation for VariantChangeType() also says (in the comments):

    "The pvarSrc argument is changed during the conversion process"

    Apparently this can happen even if pvargDest and pvarSrc are different.  I imagine that this could result in a free of the bstrVal in Index. This might not happen as things stand today, but that's an implementation detail of the API that is not guaranteed.

    It seems you should have code something like:

    VARIANT temp;

    VariantInit( &temp);

    hr = VariantCopy( &temp, &Index);

    if (!SUCCEEDED( hr)) { /* handle error */ }

    hr = VariantChangeType(&temp, &temp, 0, VT_I4);

    // etc...

    VariantClear( &temp);  // this must be called in each exit path

Page 1 of 1 (6 items)