I know there is a similarly titled question already on SO but I want to know my options for this specific case.
MSVC compiler gives a warning about strcpy:
1>c:\something\mycontrol.cpp(65): warning C4996: 'strcpy': This function or
variable may be unsafe. Consider using strcpy_s instead. To disable
deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
Here's my code:
void MyControl::SetFontFace(const char *faceName)
{
LOGFONT lf;
CFont *currentFont = GetFont();
currentFont->GetLogFont(&lf);
strcpy(lf.lfFaceName, faceName); <--- offending line
font_.DeleteObject();
// Create the font.
font_.CreateFontIndirect(&lf);
// Use the font to paint a control.
SetFont(&font_);
}
Note font_
is an instance variable. LOGFONT
is a windows structure where lfFaceName
is defined as TCHAR lfFaceName[LF_FACESIZE]
.
What I'm wondering is can I do something like the following (and if not why not):
void MyControl::SetFontFace(const std::string& faceName)
...
lf.lfFaceName = faceName.c_str();
...
Or if there is a different alternative altogether then let me know.
The reason you're getting the security warning is, your faceName
argument could point to a string that is longer than LF_FACESIZE
characters, and then strcpy
would blindly overwrite whatever comes after lfFaceName
in the LOGFONT
structure. You do have a bug.
You should not blindly fix the bug by changing strcpy
to strcpy_s
, because:
*_s
functions are unportable Microsoft inventions almost all of which duplicate the functionality of other C library functions that are portable. They should never be used, even in a program not intended to be portable (as this appears to be).strcpy
(strncpy
, strlcpy
, strcpy_s
) simply truncate the string if it's too long, which in this case would make you try to load the wrong font. Worse, strncpy
omits the NUL terminator when it does that, so you'd probably just move the crash inside CreateFontIndirect
if you used that one. The correct fix is to check the length up front and fail the entire operation if it's too long. At which point strcpy
becomes safe (because you know it's not too long), although I prefer memcpy
because it makes it obvious to future readers of the code that I've thought about this.TCHAR
and char
are not the same thing; copying either a C-style const char *
string or a C++ std::string
into an array of TCHAR
without a proper encoding conversion may produce complete nonsense. (Using TCHAR
is, in my experience, always a mistake, and the biggest problem with it is that code like this will appear to work correctly in an ASCII build, and will still compile in UNICODE mode, but will then fail catastrophically at runtime.)You certainly can use std::string
to help with this problem, but it won't get you out of needing to check the length and manually copy the string. I'd probably do it like this. Note that I am using LOGFONTW
and CreateFontIndirectW
and an explicit conversion from UTF-8 in the std::string
. Note also that chunks of this were cargo-culted out of MSDN and none of it has been tested. Sorry.
void MyControl::SetFontFace(const std::string& faceName)
{
LOGFONTW lf;
this->font_.GetLogFontW(&lf);
int count = MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS,
faceName.data(), faceName.length(),
lf.lfFaceName, LF_FACESIZE - 1)
if (count <= 0)
throw GetLastError(); // FIXME: use a real exception
lf.lfFaceName[count] = L'\0'; // MultiByteToWideChar does not NUL-terminate.
this->font_.DeleteObject();
if (!this->font_.CreateFontIndirectW(&lf))
throw GetLastError(); // FIXME: use a real exception
// ...
}