Leave well enough alone

  • Article

I was reviewing a check-in the other day, and one of the changes was to fix a line of code which resulted in one of the new "safe CRT" warnings:

 

warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead.

 

Here is the original code:

HRESULT GetDefaultList(char** tests)

    {

    if(!tests) return E_INVALIDARG;

 

    char *testList = "item1, item2, item3";

 

    *tests = new(std::nothrow) char[strlen(testList) + 1];

 

    if(!*tests) return E_OUTOFMEMORY;

 

    strcpy(*tests, testList);

    return S_OK;

    }

 

Here is the updated code that was checked in:

HRESULT GetDefaultList(char** tests)

    {

    if(!tests) return E_INVALIDARG;

 

    char *testList = "item1, item2, item3";

 

    *tests = new(std::nothrow) char[strlen(testList) + 1];

 

    if(!*tests) return E_OUTOFMEMORY;

 

    strcpy_s(*tests, strlen(*tests), testList);

 

    return S_OK;

    }

 

The problem is that we took perfectly working and valid code, and because of a compiler warning, we introduced a run-time bug: strlen of uninitialized data. The obvious take away from this is to ignore the compiler, but a better approach is to actually think about what you are doing.

 

1. Think about and review every line of code you write. Even (especially?) if it is just a "one line fix".

2. Introducing code churn always introduces risk. If your code isn't broken to begin with, "fixing" various warnings (either from a compiler or static analysis tool) might not be the best use of everybody's time.

 

I'm a big proponent of getting projects to build "clean" on the most restrictive warning level. But when tools encourage you to iterate over valid code, it might be better to just disable the warnings over a specific block of code.