What's wrong with this code, part 19

Wow, I've done 19 of these?  Cool.

I got an email question from a reader earlier today, and I realized that his question would make a great "What's wrong with this code" question.

He has a C++ function "GetValue" that is used to retrieve a value from something (it doesn't actually matter what).  His function is intended to be called from OLE automation, so it has three versions, one which takes a VARIANT, one which takes an integer index, one which takes a BSTR key.  If the input VARIANT is an integer, it assumes that it's an index, if the input VARIANT is a string, it assumes that it's a key.

Here's the version of the code as provided by the reader:

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; }

His idea was that the function would be called like this:

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

Unfortunately, this doesn't quite work :(.  Why not?

 

As always Kudos and comments tomorrow.

Edit: I can't count :)

Comments

• Anonymous
  November 13, 2006
  If the Index parameter is supposed to be interpreted as a key when it is a string, as you have stated, then the CComVariant(_T("1")) parameter value should be interpreted as a key since it is a string (BSTR). Instead, the code incorrectly interprets this as an integer, since the first VariantChangeType is able to successfully convert the string "1" to an integer. I think the developer should be instead looking at the VARTYPE of the variant directly, rather than calling VariantChangeType, if the interpretation I have made of the requirements is correct.

• Anonymous
  November 13, 2006
  tzagotta, actually CComVariant(_T("1")) should be treated as an index, not a string. When dealing with parameters passed in via automation, you often get integer parameters passed in as strings, that's why VariantChangeType works.

• Anonymous
  November 13, 2006
  OK, the next issue I see is that if you pass in a string like CComVariant(_T("1")), since the Index parameter is passed by value, then Index is pointing to the same BSTR pointed to by the caller's temporary CComVariant. If the first VariantChangeType succeeds, I would expect that function to free the BSTR during the conversion. The caller then also frees the same BSTR when the temporary CComVariant's destructor runs. This double-free could/would probably lead to a crash. I think I also see a memory leak, which happens when the first VariantChangeType fails but the second succeeds, when the user has passed a non-string type that can be successfully converted to a string. In that case, there is no code that frees the Index's referenced BSTR before the function returns.

• Anonymous
  November 13, 2006
  I'm testing the code on VC 6 and it's working. Therefore, the solution is for everyone to use VC 6. ;) I'm thinking any problems in the code stem from the GetValue functions treating a VARIANT as if it had auto-cleanup like CComVariant. Like tzagotta said, if the second VariantChangeType succeeds, the resulting BSTR is leaked.

• Anonymous
  November 13, 2006
  Yes, this is breaking the rules of memory ownership. You do not have ownership of a parameter that is passed to you by value so you should never free such a parameter whether it is a BSTR, VARIANT, or whatever, and that's exactly what is happening by calling VariantChangeType(). If you have a function that wants to mess around with a parameter that has been passed to you by value, you should make a copy of the parameter and mess around with the copy.

• Anonymous
  November 13, 2006
  The comment has been removed

• Anonymous
  November 13, 2006
  The call to VariantChangeType aliases the source and destination.  The documentation doesn't say if this is kosher or not, so, theoretically, there are two ways this could fail:

1.  The integrity of the source argument is disrupted during the conversion (in the same way a memcpy can cause havok with overlapping buffers).  The conversion could thus fail, even if it were otherwise possible.
2.  VariantChangeType may start to change the destination argument before it realizes the conversion is not possible, leaving the destination argument trashed.  (The documentation doesn't make any promises in this regard.)  Since they're aliased, your copy of the source argument is also trashed.  In the event of failure, the code tries again, now with a possibly trashed source argument. Why doesn't VariantChangeType use a const pointer for the source argument?  Then the compiler would save you in one of these cases.

• Anonymous
  November 13, 2006
  @Adrian: Actually, the documentation for VariantChangeType explicitly states that conversion in place is allowed: pvargDest A pointer to the coerced argument. If this is the same as pvarSrc, the variant will be converted in place.

• Anonymous
  November 13, 2006
  In C++ we know a shallow copy is made by default, what about other clients.  The problem is knowing from within the function whether you received is a deep or shallow copy. The language doesn't dictate, so rules (or documentation) are needed.  If it is agreed that a deep copy is passed to the function the function needs to perform the cleanup at which point cleanup functions need to be specified (VariantClear, SysFreeString, CoTaskMemFree) so the correct memory manager is used. Conversely if it is agreed a shallow copy is passed these function better not call the cleanup functions either directly or indirectly (VariantChangeType).

• Anonymous
  November 13, 2006
  > actually CComVariant(_T("1")) should be treated as an index, > not a string. OK, this is a reasonable rule, but ... [Digression:] > When dealing with parameters passed in via automation, you > often get integer parameters passed in as strings, that's why > VariantChangeType works. OK, this is a superb reason for that reasonable rule, but ... [End of digression] Notice that this rule contradicts the rule in your original posting.  tzagotta's first comment accurately diagnosed one of the bugs under the original rules stated in your base note.  Sure you can change the rules and you have good reason, but you still "owe" him/her credit. I agree with the memory leak diagnosis in tzagotta's second comment.  ... OK, no need, enough others already agreed and someone proved it.

• Anonymous
  November 13, 2006
  > The language doesn't dictate, so rules (or documentation) > are needed. There is a rule for this in COM: http://msdn2.microsoft.com/en-us/library/ms976831.aspx In COM, the rule doesn't vary from language to language, rather you're supposed to declare in the interface which parameters are [in], [out], or [in,out], and then those parameters are supposed to follow the rules in the article above. In this case, this particular function is a non-COM C++ specific function since it uses VARIANT&, but the first parameter would be considered an [in] parameter.

• Anonymous
  November 13, 2006
  I have spotted this bug in ADO. It will destroy VARIANT strings passed by value where it expects integers or other types, because it erroneously considers BSTRs "value" types (it only checks for the "byref" flag). Very interesting crashes when said arguments were passed from JScript code... even more fun considering JScript pools its strings in a private heap, and this tends to hide the bug in all but the most preposterous cases

• Anonymous
  November 13, 2006
  I have spotted this bug in ADO. It will destroy VARIANT strings passed by value where it expects integers or other types, because it erroneously considers BSTRs "value" types (it only checks for the "byref" flag). Very interesting crashes when said arguments were passed from JScript code... even more fun considering JScript pools its strings in a private heap, and this tends to hide the bug in all but the most preposterous cases. Some idiot even made a "security advisory" about it, because it could easily pass for an overflow issue

• Anonymous
  November 13, 2006
  I have spotted this bug in ADO. It will destroy VARIANT strings passed by value where it expects integers or other types, because it erroneously considers BSTRs "value" types (it only checks for the "byref" flag). Very interesting crashes when said arguments were passed from JScript code... even more fun considering JScript pools its strings in a private heap, and this tends to hide the bug in all but the most preposterous cases. Some idiot even made a "security advisory" about it, because it could easily pass for an overflow issue

• Anonymous
  November 14, 2006
  I think this also should be "Part 19" since you had a "Part 18" post in March of this year.

• Anonymous
  November 14, 2006
  In yesterday's post , I asked about a relatively simple piece of code that tried to use the COM type