views:

78

answers:

3

I am writing some C code which is expected to compile on multiple compilers (at least on MSVC and GCC). Since I am beginner in C, I have all warnings turned on and warnings are treated as errors (-Werror in GCC & /WX in MSVC) to prevent me from making silly mistakes.

When I compiled some code that uses strcpy on MSVC, I get warning like,

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.

I am bit confused. Lot of common functions are deprecated on MSVC. Should I use this secured version when on Windows? If yes, should I wrap strcpy something like,

my_strcpy()
{
#ifdef WIN32
 // use strcpy_s
#ELSE
 // use strcpy    
}

Any thoughts?

+1  A: 
Pete Kirkham
Thanks. So are you saying that I should suppress the warnings by defining the macro which MSVC documentation says?
Appu
@Appu you should be using the 'n' versions anyway, which are safer than the non-'n' versions. If you are using the safe versions, then the warning is telling you something you already have mitigated, so it's OK to disable it.
Pete Kirkham
i understand the advantage n version gives us. But isn't that a overhead? Consider I need to copy a string whose length is not known. So I have to first use `strlen` to compute the length and use `strncpy` to do the copy. Essentially the strings will be be iterated twice where `strcpy` does it once. Or am I missing something?
Appu
strncpy is **NOT** a "safe version of strcpy". It has very weird behavior intended for storing data in fixed-width fields that might not be null terminated. It also wastes lots of time zero-filling the area beyond the end of the string.
R..
@R.. I agree that it is a bit weird, but it does avoid the possiblity of buffer overflow, therefore it is safer. You know from the return value whether or not the result is nul terminated. Did you not read the part of the answer where I said it was less safe?
Pete Kirkham
@Appu if you want to copy the whole of a string, you need to know how large a buffer to create to copy it into, so you have to have the 'n' anyway. strcpy can buffer overflow if you don't know the length of the string and have not allocated a large enough buffer, or if the input string is not nul-terminated. So you need the target buffer size, not the source size, which strnlen would give you.
Pete Kirkham
There's no sense in using a function which does unnecessary work and leaves a mess you have to check for and clean up from after calling it. You could just as easily use `snprintf(dest, len, "%s", src);` which always gives the correct behavior (as long as `len` fits in an `int`).
R..
@R.. you're right on that count - that was a typo. But you still have to check the return value for whether or not it is nul terminated.
Pete Kirkham
Nope, `snprintf` always yields null-terminated strings. See 7.19.6.5 in ISO C99.
R..
@R.. that's odd - did the spec change from the original POSIX version?
Pete Kirkham
There was an incompatibly-different `snprintf` in SUSv2. I'm not sure if it was considered to be "POSIX" then or an XSI extension. It had lots of bad behavior like returning -1 when the length was insufficient so that you had to repeatedly retry with larger buffers until you found one large enough. ISO C `snprintf` always returns the required length. The only unfortunate thing is that it does not support sizes greater than INT_MAX. The size argument can be greater than INT_MAX but if formatting the string requires more than INT_MAX bytes it will fail with an overflow error (returning -1).
R..
+2  A: 

There are lots and lots of discussions about this topic here on SO. The usual suspects like strncpy, strlcpy and whatever will pop up here again, I'm sure. Just type "strcpy" in the search box and read some of the longer threads to get an overview.

My advice is: Whatever your final choice will be, it is a good idea to follow the DRY principle and continue to do it as in your example of my_strcpy(). Don't throw the raw calls all over your code, use wrappers and centralize them in your own string handling library. This will reduce overall code (boilerplate), and you have one central location to make modifications, if you change your mind later.

Of course this opens up some other cans of worms, especially for a beginner: Memory handling responsibility and interface design. Both a topic on its own, and 5 people will give you 10 suggestions of how to do it. A central library usually has the nice effect that it enforces a decision, which you will follow throughout your whole codebase, instead of using method a in module A and method b in module B, causing you trouble when you try to connect A with B...

Secure
Thanks. Does this mean whatever I have done will be the way to go rather than suppressing the warning message?
Appu
Dangerous question. ;) There are compilers with warnings that I'd consider "over-paranoid". OTOH, even these are most often a good hint that you should inspect some construct more carefully. The standard allows these warnings, even a "Warning: You are trying to compile to an executable which could get infected by a virus." is possible, though useless. Usually you shouldn't suppress anything, carefully read it and research it if in doubt. Only suppress when you know exactly what you're doing. Else in the given case, there are other "deprecated" functions for which you won't get a warning...
Secure
Microsoft's warnings are intended more to get you hooked on using nonstandard MS functions than to protect you from coding mistakes...
R..
Of course, and the exact reason why I've set deprecated in quotes. Yet another reason why a wrapper library is a good idea. ;)
Secure
+2  A: 

Whenever you move data between non-constant-size buffers, you have to (gasp! omg!) actually think about whether it fits. Using functions (like the MS-specific strcpy_s or the BSD strlcpy) that purport to be "safe" will protect you from some obvious buffer overflow conditions, but won't protect you from the bugs that result from string truncation. It also won't protect you from integer overflows in computing the necessary sizes of buffers.

Unless you're an expert dealing with C strings, I would recommend forgetting about special functions and commenting every line of your code that will perform variable-length/position writes with a justification for how you know, at this point in the program, that the length/offset you're about to use is within the bounds of the size of the buffer. Do this for lines where you perform arithmetic on sizes/offsets too - document how you know that the arithmetic will not overflow, and add tests for overflow if you find you don't know.

Another approach is to completely wrap all your string handling in a string object that stores the length of the buffer along with the string and automatically reallocates when a string needs to be enlarged, and then only use const char * for read-only access to strings when you need to pass them to system functions or other libraries. This will sacrifice a good bit of the performance you'd expect from C, but it will help you ensure that you don't make mistakes. Just don't take it to the extreme. There's no need to duplicate stuff like strchr, strstr, etc. in your string wrapper. Just provide methods to duplicate string objects, concatenate them, and truncate them, and then with the existing library functions that operate on const char * you can do just about anything you'd want to.

R..