CODEWITHSUNDEEP
clinuxvalgrind
Lipika Deka
Lipika Deka

Reputation: 3884

How serious is the following Memory leak shown by Valgrind

After writing thousands of lines of code I use valgrind and am horrified to see the amount of errors. Was just using GDB before. Most of my errors are with string functions. I post a portion. I understand the error is happening because strlen does not count the trailing NULL whereas strcpy adds it. How serious is it? Do I really need to fix them? I can fix it but worry if that may lead to more errors as my code did not not keep that in mind when I as wriitng it.

Does the strcpy copy the trailing NULL even if the is no space reserved for it?

t.write_length = (strlen("NA\n");/*Line number 116*/
t.data = malloc(strlen("NA\n");/*117*/
strcpy(t.data,"NA\n");/*118*/

Valgrind:

==3287== Invalid write of size 1
==3287==    at 0x400764E: memcpy (mc_replace_strmem.c:497)
==3287==    by 0x804A714: log_txn_commit (Log_manager.c:118)
==3287==    by 0x8049D3C: on_txn_commit (TxFS_manager.c:85)
==3287==    by 0x804939E: handler (Reader.c:139)
==3287==    by 0xBF5F18: start_thread (in /lib/libpthread-2.12.90.so)
==3287==    by 0xB37A2D: clone (in /lib/libc-2.12.90.so)
==3287==  Address 0x403282b is 0 bytes after a block of size 3 alloc'd
==3287==    at 0x4005BDC: malloc (vg_replace_malloc.c:195)
==3287==    by 0x804A6F5: log_txn_commit (Log_manager.c:117)
==3287==    by 0x8049D3C: on_txn_commit (TxFS_manager.c:85)
==3287==    by 0x804939E: handler (Reader.c:139)
==3287==    by 0xBF5F18: start_thread (in /lib/libpthread-2.12.90.so)
==3287==    by 0xB37A2D: clone (in /lib/libc-2.12.90.so)

Upvotes: 4

Views: 574

Answers (4)

Mitch Wheat
Mitch Wheat

Reputation: 300579

It is a serious memory overwrite problem. Your code should be

t.write_length = strlen("NA\n");/*Line number 116*/
t.data = malloc(t.write_length + 1);
strcpy(t.data,"NA\n");

and needs fixing for sure. strcpy() will append the termininating '\0' which there isn't room for.

To avoid overflows, the size of the array pointed by destination shall be long enough to contain the same C string as source (including the terminating null character), and should not overlap in memory with source.

Always take Valgrind's advice seriously!

Upvotes: 8

David Stone
David Stone

Reputation: 28803

You always want to fix errors reported by Valgrind. Invalid writes lead to unexpected behavior, which is by definition not what your program should do. Depending on how your program is laid out in memory, you could be overwriting other important variables, or not fully writing what you expect.

If fixing this leads to more errors in your code, that means that other parts of your code are in error, not Valgrind's report. You should fix this bug, and if that leads to further errors being reported, you fix those, too. Ignore invalid read / write errors at your own peril.

Upvotes: 5

clstrfsck
clstrfsck

Reputation: 14839

Potentially quite serious, and you should fix it. Consider using either strdup() if you need the trailing null or memcpy() if you don't need the trailing null.

Upvotes: 2

Chris Eberle
Chris Eberle

Reputation: 48785

Yes, you always need to malloc(strlen(str) + 1) bytes for a string (for that pesky null terminator). Or the easier way would be using strdup.

Upvotes: 5

Related Questions