How to get rid of "unsafe" warnings (strcpy, sprintf, strdup)

Question

I'm trying to get rid of some compiler warnings that say strcpy, sprintf, etc are unsafe. I get why they're unsafe, but I can't think of a good way to fix the code, in a C++ style.

Here's a excerpt of the code:

extList->names[i]=(char *)malloc(length*sizeof(char));
strcpy(extList->names[i],extName);                     // unsafe
// strncpy(extList->names[i],extName,length);          // also unsafe

I can't think of a safe way to copy the data over in C++ without knowing the length of the stuff to copy. I know there's strlen(), but that's also unsafe since it assumes (maybe incorrectly) that the data is null-terminated.

Also:

// used to concatenate:
sprintf(extStr,"%s%s",platExtStr,glExtStr);

Using std::string to concatenate is easy enough, but then I need to get the data into extStr somehow (and not using strcpy, lol). The string::c_str() function returns a pointer to un-modifiable data, so I can't just set extStr equal to it. (And I'm not even sure if the c_str() pointer needs delete called on it later? Does it allocate space using "new"?)

Any advice on this stuff? This is part of a 10,000 line file that's not mine... so I'm not exactly keen on re-writing the thing in the C++ way.

Answer 1

You do know how much to copy - you allocated space for it!

Surely you wouldn't willingly copy more than the space you allocated?

I would prefer to use a method that explicitly avoids buffer overruns by limiting the number of items copied. Back when I was a C programmer we used

dest = malloc(len);         // note: where did we get len?
if ( dest is null )  panic!  // note: malloc can fail
strncpy(dest, src, len);
dest[len-1] =0;

This is slightly messy, and has been pointed out is using strncpy() a method which really was originally designed for fixed-width fields rather than strings. However it does ach

There are methods such as strdup() and strlcpy() which may we help.

My recommendations:

1). Your target should not be to suppress warnings but to make the code robust.

2). When copying strings you need to ensure these things:

• Protect yourself from bad input, for example an unterminated or excessively long string.
• Protect yourself from malloc failures,
• Strongly prefer copies of counted numbers of characters to copying until we see a null
• If you claim to build a string, then make abolsutely sure you null terminate it

If strlcpy() is available in your environment then you could use it, otherwise why not write your own little utilityy function? Then if there are warnings in just that function you've localised then problem.

Answer 2

I think that you should replace all the function-calls if possible to call an implementation of your own. A good example here would be a function to replace strcpy and call the compiler-specific version of strcpy inside it. Your implementation can then easily be modified to suit any compiler of your choice, specifically if you will be adding or changing platforms/compilers.

Example:

char* StringCopy(char* Destination, const char* Source, size_t DestinationSize)
{
#ifdef _MSC_VER
  return strcpy_s(Destination, Source, DestinationSize);
#else
  if(!(strlen(Source) >= DestinationSize))
     return strcpy(Destination, Source);
  else
     return 0x0;
#endif
}

Answer 3

You don't really need pragmas to disable them.

For win32/msvc, in ProjectProperties -> Configuration Properties -> C/C++ -> Preprocessor -> Preprocessor Definitions, add following macros:

_CRT_SECURE_NO_DEPRECATE  
_CRT_NONSTDC_NO_DEPRECATE

Or you can pass thos in command line parameters (-D_CRT_SECURE_NO_DEPRECATE). You can probably #define them at the beginning of certain *.cpp files. Also, there are probably more of them (see crtdefs.h - looks like there are a lot of them...). Those kind of warnings normally tell you with which macros you can disable them - just read compiler output.

Answer 4

In your first example, you know the length already. Since you aren't allocating length+1 bytes I'll assume that length INCLUDES the null terminator. In that case, just std::copy the string: std::copy(extName, extName + length, expList->names[i]);

In your second example assuming the source strings are null terminated you could compute the destination string length and use std::copy again to concatenate manually, or you could use std::string and the std::copy from the results of c_str into your destination (Again assuming you allocated enough space for it).

c_str() does not allocate memory that would require external deletion.

Finally note that sizeof(char) will always be one and so is redundant in your malloc, although the number of bits in that character may not be 8 (See CHAR_BIT).

Answer 5

If portability is not a concern, you can use 'strcpy_s'.