I have this code:
static void foo(char *string1, char *string2)
{
char *string1_copy= malloc(strlen(string1));
strcpy(string1_copy, haystack);
char *string2_copy = malloc(strlen(string2));
strcpy(string2_copy, needle);
}
I have to copy string1 and string2 to modify their copies and maintain the originals. This does what it should and compiles without an error, but when I run:
valgrind --leak-check=full -v ./myProgram
I get this:
==20595== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
==20595==
==20595== 1 errors in context 1 of 3:
==20595== Invalid read of size 1
==20595== at 0x4C376F4: strstr (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20595== by 0x108CED: grep (myProgram.c:87)
==20595== by 0x109023: main (myProgram.c:214)
==20595== Address 0x522e3b3 is 0 bytes after a block of size 3 alloc'd
==20595== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20595== by 0x108CA5: grep (myProgram.c:77)
==20595== by 0x109023: main (myProgram.c:214)
==20595==
==20595==
==20595== 1 errors in context 2 of 3:
==20595== Invalid write of size 1
==20595== at 0x4C32E0D: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20595== by 0x108CBC: grep (myProgram.c:78)
==20595== by 0x109023: main (myProgram.c:214)
==20595== Address 0x522e3b3 is 0 bytes after a block of size 3 alloc'd
==20595== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20595== by 0x108CA5: grep (myProgram.c:77)
==20595== by 0x109023: main (myProgram.c:214)
==20595==
==20595==
==20595== 1 errors in context 3 of 3:
==20595== Invalid write of size 1
==20595== at 0x4C32E0D: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20595== by 0x108C91: grep (myProgram.c:75)
==20595== by 0x109023: main (myProgram.c:214)
==20595== Address 0x522e362 is 0 bytes after a block of size 18 alloc'd
==20595== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20595== by 0x108C7A: grep (myProgram.c:74)
==20595== by 0x109023: main (myProgram.c:214)
==20595==
==20595== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
which is exactly where I create those 2 copies with strcpy() and where I read them with strstr().
Is there a way to avoid this or am I not supposed to use strcpy() here? Is the strlen(string) not correct size of the string that I am passing?
Your allocate presents the classic Off-By-One problem. A string in C is always terminated with the nul-character. That is what distinguishes a string from a character array. To correctly allocate storage for a copy of the source string src, you must allocate strlen(src) + 1 bytes.
Your foo function makes little sense. Within foo, you allocate storage, e.g. char *string1_copy= malloc(strlen(string1)); but provide no way that your program can make use of the allocated memory after the function returns. A void function returns no value eliminating any way to determine success/failure of the copy, and there are no additional pointer-to-pointer parameters to provide any way to update the pointer at an original address. Further, after allocating storage, the function returns and you lose the pointers that hold the starting address for each allocation creating a memory leak.
When looking to duplicate two-strings, it makes no sense to write a function that duplicates both. That single-use function has little re-usability. Instead simply write a function that duplicates a single string, provides a meaningful return to allow a determination of success/failure and then call that function once for each string you need to duplicate.
That re-factoring of your function will not only allow you to adequately validate each allocation, but will also be reusable any time you need to duplicate a string. In fact POSIX provides a strdup() function that does just that, but you can easily write your own to ensure strict C standard compliance.
A reasonable implementation of such a function could be written as:
/* returns pointer to allocated copy of src, or NULL on failure */
char *dupstr (const char *src)
{
size_t len = strlen (src); /* get length of src */
char *dest = malloc (len + 1); /* allocate length + 1 bytes */
if (!dest) { /* validate EVERY allocation */
perror ("dupstr() malloc-dest");
return NULL;
}
return memcpy (dest, src, len + 1); /* copy src to dest, return ptr */
}
(note: you can also add a check on if (!src) to insure the pointer passed is not NULL -- that is left to you)
It is a simple function that obtains the length of the original string (passed as const char*) and then allocates len + 1 bytes to provide adequate storage validating the allocation and providing an error on failure and returning NULL. The function then copies src to the destination string dest using memcpy() returning a pointer to dest
(note: there is no need to use strcpy(). At that point you have already computed the length of src and there is no need to scan for end-of-string again with strcpy()).
A simple implementation that duplicates and outputs all program arguments could be:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* returns pointer to allocated copy of src, or NULL on failure */
char *dupstr (const char *src)
{
size_t len = strlen (src); /* get length of src */
char *dest = malloc (len + 1); /* allocate length + 1 bytes */
if (!dest) { /* validate EVERY allocation */
perror ("dupstr() malloc-dest");
return NULL;
}
return memcpy (dest, src, len + 1); /* copy src to dest, return ptr */
}
int main (int argc, char **argv) {
char *copies[argc]; /* VLA of argc pointers to char */
for (int i = 0; i < argc; i++) { /* loop over each argument */
if ((copies[i] = dupstr (argv[i]))) { /* duplicate in copies[i] */
puts (copies[i]); /* output copy */
free (copies[i]); /* free copy */
}
}
}
Example Use/Output
$ ./bin/dupstr my dog has fleas and my cat has none - lucky cat
./bin/dupstr
my
dog
has
fleas
and
my
cat
has
none
-
lucky
cat
Memory Use/Error Check
In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.
It is imperative that you use a memory error checking program to ensure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.
For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.
$ valgrind ./bin/dupstr my dog has fleas and my cat has none - lucky cat
==6014== Memcheck, a memory error detector
==6014== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6014== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==6014== Command: ./bin/dupstr my dog has fleas and my cat has none - lucky cat
==6014==
./bin/dupstr
my
dog
has
fleas
and
my
cat
has
none
-
lucky
cat
==6014==
==6014== HEAP SUMMARY:
==6014== in use at exit: 0 bytes in 0 blocks
==6014== total heap usage: 14 allocs, 14 frees, 1,086 bytes allocated
==6014==
==6014== All heap blocks were freed -- no leaks are possible
==6014==
==6014== For counts of detected and suppressed errors, rerun with: -v
==6014== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Always confirm that you have freed all memory you have allocated and that there are no memory errors.
Look things over and let me know if you have further questions.