Is this a good way to use strcpy?

Question

I try to use strcpy; is it a good way to do so?

char *stringvar;
stringvar = malloc(strlen("mi string") + 1);
strcpy(stringvar, "mi string");
stringvar[strlen("mi string")] = '\0';

What do you think?

Answer 1

There is exactly one bug in it: You don't check for malloc-failure.

Aside from that, it's repetitive and error-prone.
And overwriting the terminator with the terminator is useless.
Also, the repeated recalculation of the string-length is expensive.
Anyway, as you already have to determine the length, prefer memcpy() over strcpy().

What you should do is extracting it into a function, let's call it strdup() (that is the name POSIX and the next C standard give it):

char* strdup(const char* s) {
    size_t n = strlen(s) + 1;
    char* r = malloc(n);
    if (r)
        memcpy(r, s, n);
    return r;
}

char* stringvar = strdup("mi string");
if (!stringvar)
    handle_error();

Answer 2

You don't need the last line

stringvar[strlen("mi string")] = '\0';

strcpy takes care of that for you.

In real code you absolutely must check malloc's return value to make sure it's not NULL.

Other than that your code is fine. In particular, you've got the vital + 1 in the call to malloc. strlen gives you the length of the string not including the terminating '\0' character, but strcpy is going to add it, so you absolutely need to allocate space for it.

The problem with strcpy -- the fatal problem, some people say -- is that at the moment you call strcpy, you have no way of telling strcpy how big the destination array is. It's your responsibility to make the array big enough, and avoid overflow. strcpy is unable, by itself, to prevent buffer overflow -- and of course if the destination does overflow, that's a big problem.

So then the question is, how can you ensure -- absolutely ensure -- that all the calls to strcpy in all the code you write are correct? And how can you ensure that later, someone modifying your program won't accidentally mess things up?

Basically, if you use strcpy at all, you want to arrange that two things are right next to each other:

1. the code that arranges that the pointer variable points to enough space for the string you're about to copy into it, and
2. the actual call to strcpy that copies the string into that pointer.

So your code

stringvar = malloc(strlen("mi string") + 1);
strcpy(stringvar, "mi string");

comes pretty close to that ideal.

I know your code is only an example, but it does let us explore the concern, what if later, someone modifying your program accidentally messes things up? What if someone changes it to

stringvar = malloc(strlen("mi string") + 1);
strcpy(stringvar, "mi string asombroso");

Obviously we've got a problem. So to make absolutely sure that there's room for the string we're copying, it's even better, I think, if the string we're copying is in a variable, so it's patently obvious that the string we're copying is the same string we allocated space for.

So here's my improved version of your code:

char *inputstring = "mi string";
char *stringvar = malloc(strlen(inputstring) + 1);
if(stringvar == NULL) {
    fprintf(stderr, "out of memory\n");
    exit(1);
}
strcpy(stringvar, inputstring);

(Unfortunately, the check for a NULL return from malloc gets in the way of the goal of having the strcpy call right next to the malloc call.)

Basically your code is an implementation of the C library function strdup, which takes a string you give it and returns a copy in malloc'ed memory.

One more thing. You were worried about the + 1 in the all to malloc, and as I said, it's correct. A pretty common error is

stringvar = malloc(strlen(inputstring));
strcpy(stringvar, inputstring);

This fails to allocate space for the \0, so when strcpy adds the \0 it overflows the allocated space, so it's a problem.

And with that said, make sure you don't accidentally write

stringvar = malloc(strlen("mi string" + 1));

Do you see the error? This is a surprisingly easy mistake to make, but obviously it doesn't do what you want it to do.

Answer 3

There are some issues with the code posted:

• you do not check if malloc() succeeded: if malloc() fails and returns NULL, passing this null pointer to strcpy has undefined behavior.
• the last statement stringvar[strlen("mi string")] = '\0'; is useless: strcpy does copy the null terminator at the end of the source string, making the destination array a proper C string.

Here is a corrected version:

#include <stdlib.h>
#include <string.h>
...
    char *stringvar;
    if ((stringvar = malloc(strlen("mi string") + 1)) != NULL)
        strcpy(stringvar, "mi string");

Note that is would be slightly better to store the allocation size and not use strcpy:

    char *stringvar;
    size_t size = strlen("mi string") + 1;
    if ((stringvar = malloc(size)) != NULL)
        memcpy(stringvar, "mi string", size);

Indeed it would be even simpler and safer to use strdup(), available on POSIX compliant systems, that performs exactly the above steps:

    char *stringvar = strdup("mi string");