Why do my string values get overwritten?

Question

I am trying to accept some values from the user and store them in a char pointer array like so:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char *argv[]) {
    char *names[3];
    char name[20];
    int i;
    for(i = 0; i < 3; i++) {
        printf("Enter your name\n");
        scanf("%s", name);
        names[i] = (char *)malloc(strlen(name));
        names[i] = &name;
        // strcpy(names[i], name);
    }
    printf("Printing the names\n");
    for(i = 0; i < 3; i++) {
        printf("%s\n", names[i]);
    }
}

However, for the following input, I get the following output

Input: Mark Drew Andrew

Output: Andrew Andrew Andrew

Why is this happening? When I use the strcpy function I have commented out instead, it seems to work fine.

Answer 1

The mistake is in the following line ,

names[i] = &name;

it only contact the address of the name string , but it points the last value what the user is input which means it presistant with the last value only .

You must use string copy 'strcpy()' function to copy the input 'name' string in the array of pointer string 'name'.

strcpy(name[i],&name) 

Answer 2

The line names[i]=&name; doesn't make sense. You should be getting a compilation error and if you aren't turn on all warnings and errors on your compiler.

It doesn't make sense because names[i] is a pointer to character (char*) and &name is a pointer to a pointer to character (char** or more accurately char(*)[20]).

But changing that to names[i]=name; won't help. It makes names[i] point to the start of the array name. There is only one instance of name. So all elements of names will point to the same location (name) at the end of the loop.

Here's a version with the basic problems fixed:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char *argv[]) {
    char *names[3];
    char name[20];
    int i;
    for(i = 0; i < 3; i++) {
        printf("Enter your name\n");
        scanf("%s", name);
        names[i] = (char *)malloc(strlen(name)+1);//+1 to include NUL terminator.
        strcpy(names[i], name);//Copy name into the space allocated.
    }
    printf("Printing the names\n");
    for(i = 0; i < 3; i++) {
        printf("%s\n", names[i]);
        free(names[i]);//Release malloc'ed memory after use.
    }
}

There's still a security issue that the user can exceed the name buffer by entering more than 19 characters. You should impose a limit on scanf and/or use scanf_s if available using scanf("%19s",name).

Answer 3

C strings are simple arrays of characters, terminated by a trailing null character. You cannot assign arrays to other arrays, but you can assign them to pointers.

Now by doing names[i] = &name;, you are doing such a pointer assignment. Other than Java or C++ strings, just the address is copied to the pointer, there is no copying of string contents involved (by the way, &name is of bad type: char(*)[20], i. e. a pointer to array of length 20, you need a pointer to char, which you get by simply assigning name directly: names[i] = name;; name decays to a pointer automatically in this case).

The result is that all your string pointers in names point to one and the same character array name, overwriting the pointers to the arrays created by malloc (these are then lost completely, so you cannot free them again either, i. e. you have a memory leak!).

Instead, you have to copy the strings explicitly. However, to not forget the trailing null character:

int len = strlen(name)     + 1;
// trailing null char(!):  ^^^
names[i] = malloc(len);
memcpy(names[i], name, len);

Notice: using memcpy. Alternatives would have been strcpy or strncpy, but as length (including the trailing null character!) is known anyway, memcpy is most efficient...

Alternative could have been:

names[i] = malloc(20);
scanf("%19s", names[i]);

You spare copying for the price of the arrays potentially being too long. Have a close look on the format string: By adding a maximal length (you need to leave space for the terminating null character again, thus one less!) you prevent the user from writing beyond your buffer (which would be undefined behaviour and potentially lead to crash). If you do not return the array anywhere, even nicer:

char names[3][20];

Edit: Nice alternative, too: strdup (see coderredoc's answer); Two other important points:

• always check the result of malloc for being null (memory allocation might have failed! - again see coderredoc's answer).
• avoid (further) memory leaks by freeing the created strings again (not with my very last alternative)!

Answer 4

You have memory leak in the code and also assigning the wrong assignment type . correct type assignment would be names[i]=name but still that won't solve the problem. Then also it wont work. You need to use strcpy to store different names. Here you have assigned to names[i] to the same variable - that's why the same output you got.

Note that &name is of type char(*)[20] which you assigned to char*(Compiler warned about this). And for all of the 3 input you got the pointer to array of char which is always the same - so it is pointing to the same array. And now the last value it contained is the input "Andrew". That's what it printed.

So the thing would be

strcpy(names[i],name);

Also scanf should be

if( scanf("%19s",name)!=1 ){
   fprintf(stderr,"Error in input\n");
   exit(EXIT_FAILURE);
}

malloc's return value should be checked and there is no need for casting.Because void* to char* conversion is implicitly done.

Another easy way would be to use strdup to duplicate the strings.

names[i]=strdup(name);

Also don't forget to free (using free() - this you will have to do in case of strdup also) the dynamically allocated memory when you are done working with it.

---

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main(void) {
    char *names[3];
    char name[20];
    for(size_t i = 0; i < sizeof(names)/sizeof(names[0]); i++) {
        printf("Enter your name\n");
        if(scanf("%19s", name)!= 1){
            fprintf(stderr, "%s\n","Error in input" );
            exit(EXIT_FAILURE);
        }
        names[i] = malloc(strlen(name)+1);
        if( names[i] == NULL){
            perror("malloc");
            exit(EXIT_FAILURE);
        }
        strcpy(names[i],name);
    }
    printf("Printing the names\n");
    for(size_t i = 0; i < sizeof(names)/sizeof(names[0]); i++) {
        printf("%s\n", names[i]);
    }
    for(size_t i = 0; i < sizeof(names)/sizeof(names[0]); i++)
        free(names[i]);
    return 0;
}

Answer 5

You use names[i] = &name; in every loop iteration, however name gets overwritten with different name, meaning at the end of each iteration, you have all your names[i] (up to the number of iterations so far) all point to name which obviously holds one specific name (which is "Andrew" in your case).

Consider how the memory changes while your code runs:

|--name--| ... |--names[0]--|,|--names[1]--|,|--names[2]--| (loop starting)

|--"Mark"--|...|--points to name, i.e. points to "Mark"--|,|--names[1]--|,|--names[2]--| (1st iteration)

|--"Drew "--|...|--points to name, i.e. points to "Drew "--|,|--points to name, i.e. points to "Drew "--|,|--names[2]--| (2nd iteration)

|--"Andrew"--|...|--points to name, i.e. points to "Andrew"--|,|--points to name, i.e. points to "Andrew"--|,|--points to name, i.e. points to "Andrew"--| (3rd iteration)

Fix this by using strcpy as you mentioned, which will result in:

|--name--| ... |--names[0]--|,|--names[1]--|,|--names[2]--| (loop starting)

|--"Mark"--|...|--copied the value of name, i.e. holds "Mark"--|,|--names[1]--|,|--names[2]--| (1st iteration)

|--"Drew "--|...|--copied the value of name, i.e. holds "Mark"--|,|--copied the value of name, i.e. holds "Drew "--|,|--names[2]--| (2nd iteration)

|--"Andrew"--|...|--copied the value of name, i.e. holds "Mark"--|,|--copied the value of name, i.e. holds "Drew "--|,|--copied the value of name, i.e. holds "Andrew"| (3rd iteration)

Basically, you pointed to variable that changed over time (name) instead of saving it's value (using strcpy)

#note: names[i] = (char *)malloc(strlen(name));should benames[i] = (char *)malloc(strlen(name) + 1);

Answer 6

names[0], names[1] and names[2] are pointing to same memory location name, for i=0 Mark will be stored in name for i=1 Drew will be stored and for i=2 Andrew will be stored, so by the end of the loop your array is pointing to name whose value is Andrew

Answer 7

names[i] = &name; is assigning every element of names to the same character buffer, so only the final version of what's in name will persist in the output.

You need to use strcpy (better still, strncpy) to copy the contents of name to names[i].

And don't forget to call free on every element of names when you're done.