Valgrind - Invalid write of size 1 for strcpy

Question

My swapData function basically swaps data between two node of type char*

17 void swapData(struct Node *node1, struct Node *node2)
18 {
19     // Create a new node "temp" that stores the data of node2
20     struct Node *temp = (struct Node *)malloc(sizeof(struct Node));
21     temp->data = malloc(strlen(node2->data));
22    
23     strcpy(temp->data,node2->data);
24    
25     // Copy data from node1 to node2
26     strcpy(node2->data,node1->data);
27    
28     // Copy data from temp to node1
29     strcpy(node1->data,temp->data);
30    
31     free(temp->data);
32     free(temp);
33  }

Whenever I run valgrind, it keeps giving me this output:

==27570== Invalid write of size 1
==27570==    at 0x4C2C00F: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27570==    by 0x400794: swapData (test4.c:23)
==27570==    by 0x400A9C: sort (list2.c:20)
==27570==    by 0x40086B: main (test4.c:49)
==27570==  Address 0x51f11dd is 0 bytes after a block of size 13 alloc'd
==27570==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27570==    by 0x40076B: swapData (test4.c:21)
==27570==    by 0x400A9C: sort (list2.c:20)
==27570==    by 0x40086B: main (test4.c:49)
==27570==
==27570== Source and destination overlap in strcpy(0x51f1130, 0x51f1130)
==27570==    at 0x4C2C085: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27570==    by 0x4007B2: swapData (test4.c:26)
==27570==    by 0x400A9C: sort (list2.c:20)
==27570==    by 0x40086B: main (test4.c:49)
==27570==
==27570== Invalid read of size 1
==27570==    at 0x4C2C002: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27570==    by 0x4007D0: swapData (test4.c:29)
==27570==    by 0x400A9C: sort (list2.c:20)
==27570==    by 0x40086B: main (test4.c:49)
==27570==  Address 0x51f11dd is 0 bytes after a block of size 13 alloc'd
==27570==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27570==    by 0x40076B: swapData (test4.c:21)
==27570==    by 0x400A9C: sort (list2.c:20)
==27570==    by 0x40086B: main (test4.c:49)
==27570==

I believe the problem is in strcpy in swapData. Can someone tell me what's going on?

Answer 1

You need to malloc one more byte for temp->data

temp->data = malloc(strlen(node2->data)+1);

This is because you need the final byte to store then '\0' indicating the end of the string.

Answer 2

Not only do you need to add one to your malloc length, but also you can't swap strings using strcpy like you are doing. What if the first string was malloced with 10 bytes and the second with 29 bytes? When you copy to swap, you will overrun the first string's buffer. It would be best to swap the pointers. If data is defined as a fixed length array, then what you are doing is ok, but then temp could also be the same sized array instead of a node.