How could this C fragment be written more safely?

Question

Hi,

I have the following C code fragment and have to identify the error and suggest a way of writing it more safely:

char somestring[] = "Send money!\n";
char *copy;

copy = (char *) malloc(strlen(somestring));

strcpy(copy, somestring);
printf(copy);

So the error is that strlen ignores the trailing '\0' of a string and therefore it is not going to be allocated enough memory for the copy but I'm not sure what they're getting at about writing it more safely?

I could just use malloc(strlen(somestring)+1)) I assume but I'm thinking there must be a better way than that?

EDIT: OK, I've accepted an answer, I suspect that the strdup solution would not be expected from us as it's not part of ANSI C. It seems to be quite a subjective question so I'm not sure if what I've accepted is actually the best. Thanks anyway for all the answers.

Thanks,

Adam

Answer 1

char somestring[] = "Send money!\n";
char *copy = strdup(something);

if (copy == NULL) {
    // error
}

or just put this logic in a separate function xstrdup:

char * xstrdup(const char *src) 
{
    char *copy = strdup(src);

    if (copy == NULL) {
       abort();
    }

    return copy;
}

Answer 2

The safer way would be to use strncpy instead of strcpy. That function takes a third argument: the length of the string to copy. This solution doesn't stretch beyond ANSI C, so this will work under all environments (whereas other methods may only work under POSIX-compliant systems).

char somestring[] = "Send money!\n";
char *copy;

copy = (char *) malloc(strlen(somestring));

strncpy(copy, somestring, strlen(somestring));
printf(copy);

Answer 3

1. strlen + 1, for the \0 terminator
2. malloc may fail; always check malloc return value

Answer 4

char   somestring[] = "Send money!\n";
char   *copy;
size_t copysize;

copysize = strlen(somestring)+1;
copy = (char *) malloc(copysize);
if (copy == NULL)
    bail("Oh noes!\n");

strncpy(copy, somestring, copysize);
printf("%s", copy);

Noted differences above:

• Result of malloc() must be checked!
• Compute and store the memory size!
• Use strncpy() because strcpy() is naughty. In this contrived example it won't hurt, but don't get into the habit of using it.

EDIT:

To those thinking I should be using strdup()... that only works if you take the very narrowest view of the question. That's not only silly, it's overlooking an even better answer:

char somestring[] = "Send money!\n";
char *copy = somestring;
printf(copy);

If you're going to be obtuse, at least be good at it.

Answer 5

The best way to write it more safely, if one were to be truly interested in such a thing, would be to write it in Ada.

somestring : constant string := "Send money!";
declare
   copy : constant string := somestring;
begin
   put_line (somestring);
end;

Same result, so what are the differences?

• The whole thing is done on the stack (no pointers). Deallocation is automatic and safe.
• Everything is automaticly range-checked so there's no chance of buffer-overflow exploits
• Both strings are constants, so there's no chance of screwing up and modifying them.
• It will probably be way faster than the C, not only because of the lack of dynamic allocation, but because there isn't that extra scan through the string required by strlen().

Note that in Ada "string" is not some special dynamic construct. It's the built-in array of characters. However, Ada arrays can be sized at declaration by the array you assign into them.

Answer 6

Ick... use strdup() like everyone else said and write it yourself if you have to. Since you have time to think about this now... check out the 25 Most Dangerous Programming Errors at Mitre, then consider why the phrase printf(copy) should never appear in code. That is right up there with malloc(strlen(str)) in terms of utter badness not to mention the headache of tracking down why it causes lots of grief when copy is something like "%s%n"...

Answer 7

I can't comment on the responses above, but in addition to checking the return code and using strncpy, you should never do:

printf(string)

But use:

printf("%s", string);

ref: http://en.wikipedia.org/wiki/Format_string_attack

Answer 8

I would comment to previous solutions but I do not have enough rep. Using strncpy here is as wrong as using strcpy(As there is absolutely no risk of overflow). There is a function called memcpy in < string.h > and it is meant exactly for this. It is not only significantly faster, but also the correct function to use to copy strings of known length in standard C.

From the accepted answer:

char   somestring[] = "Send money!\n";
char   *copy;
size_t copysize;

copysize = strlen(somestring)+1;
copy = (char *) malloc(copysize);
if (copy == NULL)
    bail("Oh noes!\n");

memcpy(copy, somestring, copysize); /* You don't use str* functions for this! */
printf("%s", copy);

Answer 9

Ways to make the code safer (and more correct).

1. Don't make an unnecessary copy. From the example, there's no apparent requirement that you actually need to copy somestring. You can output it directly.
2. If you have to make a copy of a string, write a function to do it (or use strdup if you have it). Then you only have to get it right in one place.
3. Whenever possible, initialize the pointer to the copy immediately when you declare it.
4. Remember to allocate space for the null terminator.
5. Remember to check the return value from malloc.
6. Remember to free the malloc'ed memory.
7. Don't call printf with an untrusted format string. Use printf("%s", copy) or puts(copy).
8. Use an object-oriented language with a string class or any language with built-in string support to avoid most of these problems.

Answer 10

to add more to Adrian McCarthy's ways to make safer code,

Use a static code analyzer, they are very good at finding this kind of errors