Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code sample...

What's wrong with this code sample...

  • Comments 22

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?

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

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

  • 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

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

    BTW why we should rewrite our code?

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

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

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

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

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

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

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

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

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

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

  • What about null terminators? Shouldn't that be sizeof(temp) - 1?

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

Page 1 of 2 (22 items) 12