What's wrong with this code sample...

Today, Michael Howard posted a link to updated documentation that contains the new list of banned APIs that is in place for Windows Vista.

 

This is GREAT, and I'm really glad to see it - we've excised all of these functions from our code, others should do it as well.

 

But then as I was reading through the article, I noticed this code in the example of how to fix a function that uses strcpy:

HRESULT Function(char *s1, char *s2) {
    char temp[32];
    HRESULT hr = StringCchCopy(temp,sizeof(temp),s1);
    if (FAILED(hr)) return hr;
    return StringCchCat(temp,sizeof(temp),s2);
}

Why did I cringe the second I saw this?

Comments

• Anonymous
  March 08, 2007
  The code should call StringCchCopyA and StringCchCatA since it's operating on a char array.

• Anonymous
  March 08, 2007
  Umm.. Because it doesn't actually do anything? (temp[] is a local variable).

• Anonymous
  March 08, 2007

1. Why don't you use TCHAR instead of char? 2.Why don't you use const pointer (const TCHAR *) instead of simple pointer?
2. Why don't you check that s1 and s2 are not NULL? Something like this: if ( s1 == NULL || s2 == NULL )  return E_POINTER;
3. The temp variable inside the function's body is put on the stack and is lost when function exits... is this the function's goal? Giovanni

• Anonymous
  March 08, 2007
  Returning a pointer to a stack based local variabile is not a good idea? BTW why we should rewrite our code?

• Anonymous
  March 08, 2007
  I cringed at the use of sizeof! sizeof will return the complete size of the array not the number of elements and if later you do change to WCHAR, sizeof will return double what it did with char. The earlier example in the article uses _countof or you could use sizeof(temp)/sizeof(temp[0]) which will return the number of characters.

• Anonymous
  March 08, 2007
  temp is size 32, used twice, needs to be twice the size?

• Anonymous
  March 08, 2007
  Using sizeof() with StringCchXxx. Since you're counting bytes with sizeof(), you should use StringCbCopy etc. If you compile with UNICODE defined, the code as given above won't work.

• Anonymous
  March 08, 2007
  The call to StringCchCat wrongly passes the size of the whole buffer temp instead of the size that remains after the call to StringCchCopy.

• Anonymous
  March 08, 2007
  The comment has been removed

• Anonymous
  March 08, 2007
  Or since he's using StringCchXxx, he could use _countof() (something like sizeof(temp)/sizeof(temp[0]) )

• Anonymous
  March 08, 2007
  "cchDest    [in] Size of the destination buffer, in characters. This value must equal the length of pszSrc plus 1 to account for the copied source string and the terminating null character. The maximum number of characters allowed is STRSAFE_MAX_CCH. " if s1 is bigger than temp, StringCchCopy will fail, and StringCchCat is even worse. Btw, the whole function is quite a NOP, as all it does is to check if the s1+s2 < 32. Just a snippet?

• Anonymous
  March 09, 2007
  Well, according to the docs for both StringCchCopy and StringCchCat, the second arg is the "Size of the destination buffer, in characters. This value must equal the length of pszSrc plus 1 to account for the copied source string and the terminating null character. " Clearly this code is wrong because it's not passing in the right destination buffer size. :-)

• Anonymous
  March 09, 2007
  sizeof() returns byte size, not char size.  I would prefer to see sizeof(temp)/sizeof(temp[0]) (or _countof(); but it's hard to get a consistent header that contains that...). Plus, both StringCchCopy and StringCchCat document the cchDest as "length of <argumentName> plus 1 to account for ...the terminating null character".

• Anonymous
  March 09, 2007
  What about null terminators? Shouldn't that be sizeof(temp) - 1?

• Anonymous
  March 09, 2007
  Aside from quibbles about ASCII / Unicode stuff, and the fact that the function doesn't actually do anything, I don't see what's wrong with the usage of the String* functions.  I.e. compiled as is in ASCII mode, the usage appears to be correct.

• Anonymous
  March 09, 2007
  I just tried it and indeed if s1 or s2 is NULL you get an access violation error.  Apparently the String* functions don't check if the source strings are NULL pointers.

• Anonymous
  March 09, 2007
  Bad things: Using sizeof() instead of ARRAYSIZE().   Returning in the middle of the funciton. StringCchCat() doesn't get the full size of temp, it really only has ARRAYSIZE(temp)-lstrlen(s1) left to work with.  But I'd have to check the documentation to see if StringCchCat() handles that correctly or not.

• Anonymous
  March 09, 2007
  The comment has been removed

• Anonymous
  March 09, 2007
  Yesterday, I posted a question about a security sample I ran into the other day. I mentioned that the

• Anonymous
  March 09, 2007
  "Returning in the middle of a function"?  If the code is strictly C you'd be following Structured programming techniques and this may be an issue.  In C++, structured programming begins to fall apart (in favour of object-oriented programming) because there's other ways to have multiple exit points in C++; and in some instances you must have multiple returns from a method.

• Anonymous
  March 10, 2007
  > Why did I cringe the second I saw this? For the same reason some of your loyal readers cringe when we read some of your posted code?

• Anonymous
  March 26, 2007
  Another answer: For the same reason some of your loyal readers cringe when we read MSDN.  I've just read a nearly innocuous example because several wrongs make a right, i.e. there is no buffer overflow.  Nonetheless every occurrence of "cch" should be a "cb".  If you really don't want to cringe though, you'd better avoid noticing that this is Visual Studio 2005 stuff. http://msdn2.microsoft.com/en-us/library/yw3yyscd(VS.80).aspx