Why is this use of strcpy considered bad?

user_12675559 0
Viewed 169

I've spotted the following piece of C code, marked as BAD (aka buffer overflow bad). The problem is I don't quite get why? The input string length is captured before the allocation etc.

char *my_strdup(const char *s)
{
    size_t len = strlen(s) + 1;
    char *c = malloc(len);
    if (c) {
        strcpy(c, s);  // BAD
    }
    return c;
}

Update from comments:

  • the 'BAD' marker is not precise, the code is not bad, not efficient yes, risky (below) yes,
  • why risky? +1 after the strlen() call is required to safely allocate the space on heap that also will keep the string terminator ('\0')
strcpy buffer c
3 Answers

There is no bug in your sample function.

However, to make it obvious to future readers (both human and mechanical) that there is no bug, you should replace the strcpy call with a memcpy:

char *my_strdup(const char *s)
{
    size_t len = strlen(s) + 1;
    char *c = malloc(len);
    if (c) {
        memcpy(c, s, len);
    }
    return c;
}

Either way, len bytes are allocated and len bytes are copied, but with memcpy that fact stands out much more clearly to the reader.

user_3885200

There's no problem with this code.

While it's possible that strcpy can cause undefined behavior if the destination buffer isn't large enough to hold the string in question, the buffer is allocated to be the correct size. This means there is no risk of overrunning the buffer.

You may see some guides recommend using strncpy instead, which allows you to specify the maximum number of characters to copy, but this has its own problems. If the source string is too long, only the specified number of characters will be copied, however this also means that the string isn't null terminated which requires the user to do so manually. For example:

char src[] = "test data";
char dest[5];

strncpy(dest, src, sizeof dest);  // dest holds "test " with no null terminator
dest[sizeof(dest) - 1] = 0;       // manually null terminate, dest holds "test"

I tend towards the use of strcpy if I know the source string will fit, otherwise I'll use strncpy and manually null-terminate.

user_16871190

I cannot see any problem with the code when it comes to the use of strcpy

But you should be aware that it requires s to be a valid C string. That is a reasonable requirement, but it should be specified.

If you want, you could put in a simple check for NULL, but I would say that it's ok to do without it. If you're about to make a copy of a "string" pointed to by a null pointer, then you probably should check either the argument or the result. But if you want, just add this as the first line:

if(!s) return NULL;

But as I said, it does not add much. It just makes it possible to change

if(!str) {
    // Handle error
} else {
    new_str = my_strdup(str);
}

to:

new_str = my_strdup(str);
if(!new_str) {
    // Handle error
}

Not really a huge gain

user_66994330
Related