memory error using valgrind, strcpy

Question

I've been using valgrind, but for some reason I keep getting a memory error using a simple string copy with two strings that are the same size, in C.

The Code effected is:

node->entry = (char*)malloc(strlen(string)*sizeof(char));
strcpy(node->entry, string);

With string being: char* string = "Hello There". The Error is: Invalid write of size 2

==2035==    at 0xD494: memmove$VARIANT$sse42 (mc_replace_strmem.c:987)
==2035==    by 0x100001793: __inline_strcpy_chk (_string.h:94)
==2035==    by 0x100001699: createList (main.c:10)
==2035==    by 0x100001BE6: main (main.c:132)
==2035==  Address 0x10000c0fa is 10 bytes inside a block of size 11 alloc'd
==2035==    at 0xB823: malloc (vg_replace_malloc.c:266)
==2035==    by 0x100001635: createList (main.c:9)
==2035==    by 0x100001BE6: main (main.c:132)

Thanks for the Help!

So much noise in your code. It should be `node->entry = malloc(strlen(string) + 1)`. — Kerrek SB, Sep 14 '12 at 14:49

score 5 · Accepted Answer · answered Sep 14 '12 at 14:48

malloc(strlen(string)*sizeof(char));

You are not taking the ending '\0' into account. So it should have been (strlen(string) + 1).

Side note:

type *x;
x = malloc(size * sizeof(*x))

is much more maintainable than

type *x;
x = (type *)malloc(size * sizeof(type));

because you don't repeat yourself 3 times.

score 4 · Answer 2 · edited May 23 '17 at 10:34

Your code is broken:

You need to allocate space for the string termination character, failure to do so will make strcpy() write into non-allocated memory and cause a buffer overflow bug.
Don't cast the return value of malloc(), in C.
You need to check that the allocation succeeds before writing to the memory.

Further, note that sizeof (char) is always 1, so there's no need to involve it. Just use:

node->entry = malloc(strlen(string) + 1);

Or, check if you have strdup() since it combines all of the above.

In the general case, as @Shahbaz noted, it's better to use the sizeof operator on the result of dereferencing the left-hand pointer, to avoid repeating the type name:

type *x;
x = malloc(amount * sizeof *x);

Also, note that sizeof is not a function, the parenthesis are only needed when the argument is a type name and that case, as I just argued, is to be avoided.

memory error using valgrind, strcpy

2 Answers2