Why is my nested array of Structs overwriting previously stored values?

Question

So basically here I am just seeing whether my previously stored input, that is temp->voterDetails[0].name, has its value restored after another input.
Unfortunately it does not seem to store its value. Could you help me understanding why?

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


struct Voter {
    char *name;
};

struct Block{
    // Creating a block containing 5 voter details
    struct Voter voterDetails[5];
};

int main() {
    int choice;
    struct Block *Genesis = (struct Block *)malloc(sizeof(struct Block));
    struct Block *temp = Genesis;
    struct Block *newnode = (struct Block *)malloc(sizeof(struct Block));
    char *nm;
    int voteNum = 0;
    int blockNumber = 1;
    do{
        printf("What do you want to do\n1. Cast a vote\n2. Check your vote\n3. Exit\n");
        scanf("%d", &choice);
    
        switch(choice){
            case 1:
                printf("Enter name");
                fgets(nm, 30, stdin);
                fgets(nm, 30, stdin);
                voteNum += 1;
                temp->voterDetails[voteNum-1].name = nm;
                puts(Genesis->voterDetails[0].name);

                break;
        }
    } while (choice != 3);
    return 0;
}

Because you keep assigning the same variable `nm` to the stored values. You must make a copy of the string (and release it when you are done with it). — Jonathan Leffler, Dec 28 '22 at 21:12
sorry to bother but how would I release it. I have used strcpy to copy the string. What next? — Syed Muhammad Taha, Dec 28 '22 at 21:17
Furthermore apparently no allocation is done for the `nm` temporary char pointer. — Roberto Caboni, Dec 28 '22 at 21:17
Use `strdup(nm)` to make a copy of the string. When you're done with them, do `free(Genesis->voterDetails[i].name)` — Barmar, Dec 28 '22 at 21:31

Roberto Caboni · Answer 1 · 2022-12-30T18:52:51.310

Your program has a lot of issues:

nm temporary pointer (that in your snippet isn't even necessary, but I assume it can be useful in the whole program) is not initialized. It should be allocated, instead
Each element of the array temp->voterDetails[voteNum].name has to be allocated as well
Strings cannot be assigned like variables belonging to base types such as integers (str1 = str2; make those strings actually point to the same data). They can be copied using strcpy function
Whenever you perform a choice the newline remains in the input buffer, making the first fgets return immediately with no data. That's what you "solved" calling fgets twice, but it's not a real solution
Data read with fgets will have trailing newline, that will have to be removed from the name buffer

Here it is the working code:

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

#define MAX_VOTERS 5

struct Voter {
    char *name;
};

struct Block{
    // Creating a block containing 5 voter details
    struct Voter voterDetails[MAX_VOTERS];
};

void flushInput(void)
{
    int c;
    while ((c = getchar()) != '\n' && c != EOF) { }
}

int main() {
    int choice;
    struct Block *Genesis = (struct Block *)malloc(sizeof(struct Block));
    struct Block *temp = Genesis;
    struct Block *newnode = (struct Block *)malloc(sizeof(struct Block));
    char *nm = malloc(30 * sizeof(char)); // nm temporary pointer allocation
    int voteNum = 0;
    int blockNumber = 1;
    do{
        printf("What do you want to do\n1. Cast a vote\n2. Check your vote\n3. Exit\n");
        scanf("%d", &choice);
        flushInput();
        switch(choice){
            case 1:
                printf("Enter name\n"); // Better with a newline
                fgets(nm, 30, stdin);   // Useless extra fgets removed
                nm[strcspn(nm, "\n")] = 0;
                temp->voterDetails[voteNum].name = malloc(30*sizeof(char));
                strcpy(temp->voterDetails[voteNum].name, nm);   // Strings cannot be assigned, but are copied with strcpy
                                                                // voteNum incremented later, so '-1' can be avoided. Isn't it more readable?
                printf("Name: %s..\n", Genesis->voterDetails[voteNum].name); // printf used instead of puts, in order to append a newline
                voteNum += 1;
                
                break;
        }
    } while (choice != 3 && voteNum < MAX_VOTERS); // The program is safer if you check that votes don't exceed the number of location in your array

    free(nm);
    /* ... remember to free all the remaining pointers as soon as you don't need them anymore */
    return 0;
}

Output:

What do you want to do
1. Cast a vote
2. Check your vote
3. Exit
1
Enter name
Roberto
Name: Roberto
What do you want to do
1. Cast a vote
2. Check your vote
3. Exit
...

Some extra comments:

Pending data in stdin is flushed using the custom function flushInput() (consult this answer for more details)
After flushing correctly the input buffer, the extra fgets can be removed
It's always better to make sure that the user doesn't insert data we don't have room for. In this case, since the voterDetails array has 5 elements, we make sure to exit the loop as soon as the fifth name has been inserted
Since the constant '5' is now used several times, it is better to avoid using "magic numbers" in the code. Just define a precompiler constant, instead (MAX_VOTERS)

score 0 · Answer 2 · answered Dec 28 '22 at 21:47

0

At least these problems:

Uninitialized/assign value

Calling fgets(nm, 30, stdin); yet nm is not yet assigned. UB

answered Dec 28 '22 at 21:47

chux - Reinstate Monica

143,097
13
135
256

score 0 · Accepted Answer · answered Dec 28 '22 at 22:15

You're not allocating any space for your pointers. This doesn't happen "automatically" in C, you have to manually do it. From your fgets argument of 30, it seems you want to accept names with a max of 30 chars. If that's the case, then the easiest thing to do is simply use char arrays instead of pointers:

struct Voter {
    char name[30]; // note because of the required NUL terminator, this
                   // will only fit 29 printable characters
};

and the same for nm

char nm[30] = { 0 };  // initializes the array to 0s, altho fgets
                      // will always add a NUL terminator

But even with those fixes, you still need to copy the string data entered in nm to your names:

#include <string.h>
...
strcpy(temp->voterDetails[voteNum-1].name, nm);

Or IMHO, there's no point in using the temp nm anyway, just plug temp->voterDetails[voteNum].name directly into fgets. Also note that fgets will add a newline to the array if you don't max out the buffer size. You can search for ways to remove that if you don't want it.

If you want to use pointers instead of arrays, you must malloc space for name and nm as you have for your struct Block pointers.

Why is my nested array of Structs overwriting previously stored values?

3 Answers3