Strcpy changing struct int array

Question

I have a struct in C, and I create an array of this struct

typedef struct {
    int aNum;
    char name[20];
    char sym[10];
    char class[30];
    float weight;
    int shell[SHELLS];
} element_t;
element_t elements[MAX_ELEMENTS];

I am asking the user for an input (1 Hydrogen H other_nonmetals 1.008 1 0 0 0 0 0 0), and I separate it into an array based on the spaces. I get each user input by calling a function,

for(int i=0;i<N;i++) 
    scan_element(i);

function:

void scan_element(int i) {
    char *array[12];
    char str[100];
    int j=0;
    if (fgets(str, 100, stdin)) {
        array[j] = strtok(str," ");
        while(array[j]!=NULL)
        {
            array[++j] = strtok(NULL," ");
        }
    }
    elements[i].aNum = (int) strtol(array[0], NULL, 10);
    if(i>0)
        for(int k=5;k<SHELLS+5;k++)
            printf("%d ",elements[i-1].shell[k]);
    printf("\n");
    strcpy(elements[i].name, array[1]);
    strcpy(elements[i].sym, array[2]);
    strcpy(elements[i].class, array[3]);
    elements[i].weight = (strtod(array[4],NULL));
    for(int k=5;k<SHELLS+5;k++)
        elements[i].shell[k] = (int) strtol(array[k], NULL, 10);
}

If I just input 1 element, the shell int array is fine, but when I input another element, it messes up the previous element's shell array. It should look like 1 0 0 0 0 0 0, and it does for the current element, but when I input another element, the shell array looks like 1 0 82 1684104524 0 0 0. I figured it happens after I call strcpy, as before it prints out fine, but right after the first time I call strcpy, it adds random numbers to the array.

How can I fix it so that so I can enter multiple elements without it messing up the previous element's shell array? It only messes up the shell array, nothing else from the previous struct.

for debugging purposes, strongly suggest the struct definition include a `tag` name so debuggers, like `gdb` can just be given the struct instance and it will output the contents of each field — user3629249, Apr 09 '17 at 02:59
suggest, keep the logic simple, place the field contents directly into the fields if the struct rather than trying to save the individual fields and assigning into the struct instance, later. — user3629249, Apr 09 '17 at 03:01
Please use a debugger and step through your code so you can see what all is going wrong. — user3629249, Apr 09 '17 at 03:10
@user3629249 Regarding your first comment: You're right, but if you continue reading the code, he's *copying* from that array into the struct *before* it becomes overlayed or invalid, so this advice is irrelevant. Second comment: That's not a loop. Fourth comment: That's not an example of portable deserialisation. Fifth comment: Again, that's not a loop. Finally: Please learn C before you try to teach it. — autistic, Apr 09 '17 at 03:35
since I misread the statement: `if( fgets() )`, I have to ask What is the code doing when the code exits the 'if()' code block (but the call to `fgets()` has failed)? setting trash into the elements[i] struct. — user3629249, Apr 09 '17 at 04:26
What happens if the user does not enter enough fields? What happens if the user enters too many fields? — user3629249, Apr 09 '17 at 04:29
@user3629249 That is a legitimate question, and points out some undefined behaviour... However, undefined behaviour is pervasive throughout this code, not just there, as in `strcpy(elements[i].name, array[1]);` there's no guarantee that `array[1]` contains anything non-`NULL`, and the same goes for all of the lines following it. — autistic, Apr 09 '17 at 04:29

score 2 · Accepted Answer · answered Apr 09 '17 at 03:12

The structure element int shell[SHELLS];

has index range from 0 to (SHELLS-1).

So the shell[0] .. shell[SHELLS-1] are valid.

The following assignment is out of bound assignment:

for(int k=5;k<SHELLS+5;k++)
    elements[i].shell[k] = (int) strtol(array[k], NULL, 10);

shell[SHELLS], shell[SHELLS+1], shell[SHELLS+2], shell[SHELLS+3] and shell[SHELLS+4] will be out of bound which causes undefined behaviors. Those numbers you see happened to be numbers in those out of bound memory space.

I think you want to do something like:

  int iShellCnt = 0;

  for(int k=5;k<SHELLS+5;k++)
      elements[i].shell[iShellCnt++] = (int) strtol(array[k], NULL, 10);

Actually, I'd move the `+ 5` into the loop like so: `for (size_t k = 0; k < SHELLS && array[k]; k++) { element[i].shell[k] = strtol(array[k + 5], NULL, 10); }`... but this is okay, gets the +1 from me. — autistic, Apr 09 '17 at 03:41

score 0 · Answer 2 · answered Apr 09 '17 at 04:32

BTW: what is SHELLS defined as?

The following is the root of the problem:

The code block beginning with:

for(int k=5;k<SHELLS+5;k++)

will place the first entry in

elements[i].shell[5]

and continue placing data until long past the end of the shell[] array, This result is undefined behavior and can/will lead to a seg fault event.

Strcpy changing struct int array

2 Answers2