C++ std::string alternative to strcpy?

Question

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.

You can ignore this warning, strcpy is not deprecated. Define _CRT_SECURE_NO_WARNINGS permanently in your settings and be done with it. — Benjamin Lindley
– Benjamin Lindley, Commented Oct 21, 2011 at 16:09
Possible duplicate of: stackoverflow.com/questions/4804154/… — DeCaf
– DeCaf, Commented Oct 21, 2011 at 16:09
The danger with strcpy is that if the source string is not NULL terminated, or is longer than the destination buffer, you will get a buffer overflow - which is one of the most common sources of security flaws in C / C++ code. strcpy_s also takes the size of the destination buffer, and guarantees that after successful completion, the destination buffer will be null terminated. — Eclipse
– Eclipse, Commented Oct 21, 2011 at 16:17
@MooingDuck, the problem with strncpy is that it is broken: if it hast to truncate it won't null terminate. So better to use a quick wrapper that always adds zero at the end. — edA-qa mort-ora-y
– edA-qa mort-ora-y, Commented Oct 21, 2011 at 18:03

zwol · Accepted Answer · 2015-07-02 15:42:40Z

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:

The *_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).
Blind changes tend to not actually fix this class of bug. For instance, the "safe" variants of 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 // ... }

strcpy_s guarantees null termination, it's not a duplicate of strcpyn which is why it's a different function
@shf301: True but it blindly causes the same problem with truncation. Thus the advice here is sound.
@shf301 I didn't know that; that makes it a duplicate of strlcpy rather than strncpy. (strlcpy itself, alas, is a BSDism, but MS really could stand to learn that it is not a good thing to make up one's own names for stuff.)
It is my considered opinion that the _s functions should not be used even in code not intended to be portable, because the people who made them up ought to be punished for their failure to check whether they were duplicating existing APIs, and refusing to use them is the only stick available.
Technically they weren't duplicating existing APIs, since they added the parameter validation error return values and the buffer size parameter is always expressed in bytes (while lengths are expressed in standard-character-widths, which is an important distinction if you're trying to write _UNICODE agnostic code).

Martin Beckett · Accepted Answer · 2011-10-21 16:09:44Z

lf.lfFaceName = faceName.c_str();

No you shouldn't do that because you are making a local copy of the poitner to the data held inside the std::string. If the c++ string changes, or is deleted, the pointer is no longer valid, and if lFaceName decides to change the data this will almost certainly break the std::string.

Since you need to copy a c string, you need a 'c' function, then strcpy_s (or it's equivalent) is the safe alternative

I don't think it works like that if lf.lfFaceName is an array.

themel · Accepted Answer · 2011-10-21 16:22:08Z

Have you tried? Given the information in your post, the assignment should generate a compiler error because you're trying to assign a pointer to an array, which does not work in C(++).

#include <cstdio> #include <string> using namespace std; struct LOGFONT { char lfFaceName[3]; }; int main() { struct LOGFONT f; string foo="bar"; f.lfFaceName = foo.c_str(); return 0; }

leads to

x.c:13: error: incompatible types in assignment of `const char*' to `char[3]'

I'd recommend using a secure strcpy alternative like the warning says, given that you know the size of the destination space anyway.

mloskot · Accepted Answer · 2011-10-21 16:28:45Z

#include <algorithm> #include <iostream> #include <string> enum { LF_FACESIZE = 256 }; // = 3 // test too-long input struct LOGFONT { char lfFaceName[LF_FACESIZE]; }; int main() { LOGFONT f; std::string foo("Sans-Serif"); std::copy_n(foo.c_str(), foo.size()+1 > LF_FACESIZE ? LF_FACESIZE : foo.size()+1, f.lfFaceName); std::cout << f.lfFaceName << std::endl; return 0; }

shf301 · Accepted Answer · 2011-10-21 16:30:19Z

lf.lfFaceName = faceName.c_str(); won't work for two reasons (assuming you change faceName to a std:string)

The lifetime of the pointer returned by c_str() is temporary. It's only valid as long as the fileName object doesn't change and in alive.
The line won't compile. .c_str() returns a pointer to a char, and lfFaceName is a character array and can't be assigned to. You need to do something to fill in the string array, to fill in the bytes at lfFaceName, and pointer assignment doesn't do that.

There isn't anything C++ that can help here, since lfFaceName is a C "string". You need to use a C string function, like strcpy or strcpy_s. You can change your code to:

strcpy_s(lf.lfFaceName, LF_FACESIZE, faceName);

I think you meant to write strncpy, not strcpy. Also, you have a syntax error in your example code.
No, you meant strncpy. As I explain in my answer, strcpy_s should never be used. Also, you still haven't fixed the syntax error.

Collectives™ on Stack Overflow

C++ std::string alternative to strcpy?

5 Answers 5

7 Comments

1 Comment

Comments

Comments

2 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

7 Comments

1 Comment

Comments

Comments

2 Comments

Linked

Related