通过

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

  • 项目

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.

Comments

  • Anonymous
    November 14, 2006
    The comment has been removed

  • Anonymous
    November 14, 2006
    Oooh, yech.

  • Anonymous
    November 14, 2006
    The comment has been removed

  • Anonymous
    November 15, 2006
    What about the BSTR leaked from: VariantChangeType(&Index, &Index, 0, VT_BSTR);

  • Anonymous
    November 16, 2006
    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.

  • Anonymous
    December 28, 2006
    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