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 (22)

  1. Mike Dunn says:

    The code should call StringCchCopyA and StringCchCatA since it’s operating on a char array.

  2. grue says:

    Umm.. Because it doesn’t actually do anything? (temp[] is a local variable).

  3. Sys64738 says:

    1. Why don’t you use TCHAR instead of char?

    2.Why don’t you use const pointer (const TCHAR *) instead of simple pointer?

    3. Why don’t you check that s1 and s2 are not NULL?

    Something like this:

    if ( s1 == NULL || s2 == NULL )

     return E_POINTER;

    4. 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

  4. Luciano says:

    Returning a pointer to a stack based local variabile is not a good idea?

    BTW why we should rewrite our code?

  5. Alan says:

    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.

  6. Vishal Rao says:

    temp is size 32, used twice, needs to be twice the size?

  7. Mike Dimmick says:

    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.

  8. John Doe says:

    The call to StringCchCat wrongly passes the size of the whole buffer temp instead of the size that remains after the call to StringCchCopy.

  9. Drew says:

    In this case they’re chars, so they’re bytes. That can’t be the problem.

    I would assume that if the code worked before using strsafe, then it was compiled ANSI, so that can’t be the problem either. But Mike Dimmick’s right about bombing when compiling for Unicode. Actually, all 3 previous posters have partial solutions there if this is just a A/W/bytes style problem.

    Sure, like Sys64738 mentioned, those functions won’t work if they’re passed null strings (or overlapped ones – evil!), but presumably this code worked before.

    I personally don’t like that the strsafe version changed the function signature. Callers were expecting to pass in two strings and either get nothing back or get nothing back and possibly overwrite something on either the stack or the heap, depending. Now they get an HRESULT and no side-effect overwrites. If I were a hacker that would upset me. :-) There are other examples that changed contracts that you’re not calling out, so that can’t be what you’re after either.

    I give up. Is this just a style problem or is there an actual bug here?

  10. Sys64738 says:

    Or since he’s using StringCchXxx, he could use _countof()

    (something like sizeof(temp)/sizeof(temp[0]) )

  11. Laura T. says:

    "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?

  12. Dave says:

    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. :-)

  13. Peter Ritchie says:

    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".

  14. Jason says:

    What about null terminators? Shouldn’t that be sizeof(temp) – 1?

  15. John says:

    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.

  16. John says:

    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.

  17. jeffdav says:

    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.

  18. Andrew says:

    Like grue said, this function doesn’t do anything, temp is local and is never used before the function returns. However the StringCchCat line simply writes over whatever StringCchCopy already put into temp (first argument to StringCchCat is the start point in the destination string for the concatenation) so it *really* doesn’t do anything.

  19. Yesterday, I posted a question about a security sample I ran into the other day. I mentioned that the

  20. Peter Ritchie says:

    "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.

  21. Norman Diamond says:

    > 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?

  22. Norman Diamond says:

    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