/analyze for Visual Studio—the Ugly (part 4), False Negatives

/Analyze is a powerful Visual C++ feature that can help to find bugs. However it sometimes misses critical issues. In this quick post I describe an amusingly dangerous pattern that /analyze fails to recognize, and explain the one-true-fix.

These tests are all done with Visual Studio 2010 SP1, C/C++ Optimizing Compiler Version 16.00.40219.01 for 80×86.

So you want to copy a string do you?

I think we can all agree that strcpy is dangerous. There are a few cases where it can be used safely, but it’s a bad habit to get into. So consider this code:

void StringCopyTests( const char* pEvil )
{
    char buffer[100];

    // warning C4996: ‘strcpy’: This function or variable may be unsafe.
    // Consider using strcpy_s instead.
    // warning C6204: Possible buffer overrun in call to ‘strcpy’: use
    // of unchecked parameter ‘pEvil’
    strcpy(buffer, pEvil);

    // No warning
    strcpy_s(buffer, strlen(pEvil) + 1, pEvil);
}

VC++ warngs that strcpy is dangerous and that strcpy_s would be a better choice. With /analyze VC++ also warns that this is a specifically dangerous use of strcpy. Also fine.

However it has no comment on the semantically identical use of strcpy_s.

I hope that everyone is horrified by using strcpy_s in that matter. Using the length of the source string to tell strcpy_s the size of the destination buffer means that the copy is unbounded and is truly identical to strcpy. Code like this suggests that the author is not thinking at all clearly about their buffers. I didn’t write this code. But I have seen this pattern a couple of times, and it distresses me that /analyze doesn’t pick it up.

Any calculation of the destination buffer size that in any way uses attributes of the source buffer is broken. I need /analyze to tell me if this pattern exists anywhere else in our code base, and right now it is letting me down.

Copying strings safely

The VC++ deprecation warnings are a good idea in theory, but in the real world they often fail. There are good and bad ways to convert from strcpy/strncpy to strcpy_s/strncpy_s, and experience has shown me that, while most conversions aren’t as bad as this one, the sad fact is that most developers don’t do an effective job of converting to the safer CRT. In this particular case there are exactly two sensible ways of fixing the code:

void StringCopyFix( const char* pEvil )
{
    char buffer[100];

    // Terminate if source is too big
    strcpy_s(buffer, pEvil);

    // Truncate if source is too big
    strncpy_s(buffer, pEvil, _TRUNCATE);
}

Choosing between strcpy_s and strncpy_s with _TRUNCATE is a policy decision – if the string doesn’t fit do you want to continue or abort? What is not up for discussion is the size parameter. It must be specified correctly and, it turns out, the best way to do that is to omit it. The safer CRT functions all have template overloads that will, if you give them a chance, infer the size of arrays. These versions should be preferred because there are a half-dozen different ways that human beings can mess up the destination buffer size*, whereas the compiler always gets it right. If the compiler can’t infer the correct size, it will refuse to compile the code.

* Some of the ways I have seen buffer sizes specified incorrectly include:

Using strcpy to calculate the buffer size
Using a hard-coded number that doesn’t match the buffer size
Using a named constant that doesn’t match the buffer size
Using sizeof when _countof is called for
Using sizeof the wrong array
Using sizeof(buffer-1) when sizeof(buffer)-1 was intended

P.S.

You’d think the name of the source string pointer would have been a give away…