What's wrong with this code sample, the answer

Yesterday, I posted a question about a security sample I ran into the other day.  I mentioned that the function made me cringe the instant I saw it.

Mike Dunn and Sys64378 danced around the root of what made the function cringeworthy, but Alan nailed it first.

The reason I cringed when looking at the code is that it worked by accident, not by design.

Here's the code again (C&P because it's short):

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

The code as written is correct (yes, grue, Luciano and others pointed out that it actually doesn't do anything, but neither did the original code (this is supposed to be a secure version of an existing function).

However the problem in the code becomes blindingly obvious when you follow Sys64738's suggestion:

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

The problem is that the function depends on the fact that temp[] is declared as an array of type "char".  Once you redeclare temp (and s1 and s2) as TCHAR's (which is a likely first step in a conversion to unicode), then it would INTRODUCE a security hole the instant that someone set UNICODE=1.

Mike Dunn's answer (use StringCchCopyA and StringCchCatA) would absolutely work in this situation (because when you enabled UNICODE, then you would get a compiler error).

But I think a far better solution would be:

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

If you use the Cb version of the safe String functions then you're safe regardless of the type of temp.

Things that weren't wrong (IMHO):

Checking for null s1 and s2.  First off, this is sample code (and thus the checks would distract from the point being made), and secondly, I'm a firm believer that you don't check for correctness of input values - IMHO it's far better to crash when a caller violates your API contract than to try to resolve the issue.