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.