C malloc() with structs advice

Question

I have two structs and I have an array of 30 pointer StudentType. I have a problem with malloc(). When I compile it it's ok. But when I try to debug it, it shows "Segmentation Fault" in Dev c++.

In Eclipse, it shows up anything on console. I think that my mistakes are on these lines of code:

students[0]=(StudentType *)malloc(sizeof(StudentType)*NumOfStudents);
(*students[NumOfStudents]).firstName=(char*)malloc(sizeof(char[30]));
(*students[NumOfStudents]).lastName=(char*)malloc(sizeof(char[30]));

That's a part of my code.

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

typedef struct{
    float firstAssignment;
    float secondAssignment;
    float midterm;
    float final;
    float finalMark;
}StudentRecordType;

typedef struct{
    char *firstName;
    char *lastName;
    int idNumber;
    StudentRecordType marks;
}StudentType;
StudentType *students[30];
char firstName[30];
char lastName[30];

int ReadFromFile();
int PrintAll();
int NumOfStudents;
int i;

int main(void)
{
    ReadFromFile();
}

int ReadFromFile()
{
    FILE *fp;
    fp=fopen("project2-askhsh2.dat","r");
    if(fp==NULL)
    {
        printf("Error opening file.\n");
    }
    else
    {
        printf("Successful open of project2-askhsh2.dat\n");
    }
    fscanf(fp,"%d",&NumOfStudents);
    printf("%d\n",NumOfStudents);
    students[0]=(StudentType *)malloc(sizeof(StudentType)*NumOfStudents);
    (*students[NumOfStudents]).firstName=(char*)malloc(sizeof(char[30]));
    (*students[NumOfStudents]).lastName=(char*)malloc(sizeof(char[30]));

    for(i=0;i<NumOfStudents;i++)
    {
        (*students[i]).idNumber=i;
        fscanf(fp,"%s %s", (*students[i]).firstName,(*students[i]).lastName);
        fscanf(fp,"%f %f %f %f",(*students[i]).marks.firstAssignment,(*students[i]).marks.secondAssignment,(*students[i]).marks.midterm,(*students[i]).marks.final);
        printf("%s",(*students[i]).firstName);//, students[i].lastName);
    }

}

Answer 1

(*students[NumOfStudents]).firstName=(char)malloc(sizeof(char[30]));

First, the cast is just plain wrong. Casting to char loses information.
Second, in C don't cast the return value from malloc(). It is, at best, redundant, and may hide an error the compiler would have caught without the cast.

^{If you're going to cast anyway, cast to char*.}

Answer 2

You seem to have an extra level of pointers that you don't need causing some confusion, when using the students pointer. You also access past the end of the array that you've allocated.

So instead of

StudentType *students[30];

Which gives you an array of 30 pointers to StudentType I guess you probably just wanted:

StudentType *students;

Which is just a plain pointer to StudentType and can be used as the base of your dynamically allocated array. Then when you do the allocations you'd do this:

students = malloc(sizeof(*students) * NumOfStudents);

And you'd have to initialise each of those StudentTypes before you use them.

for(i=0;i<NumOfStudents;i++)
{
    students[i].firstname = malloc(30);
    students[i].lastname = malloc(30);
}

Notice that each StudentType is now accessed directly as an element from the students array as student[i], rather than the *students[i] which you had that was wrong. You can expand this to the rest of your code. Remember that you can only access from index 0 to NumOfStudents-1, so do not use students[NumOfStudents].

The other problem you will have is that when you use fscanf() you need to pass the address of the variable to store the result in using the ampersand operator. Currently you are only passing the value, e.g. you should use &students[i].marks.firstAssignment instead of (*students[i]).marks.firstAssignment, assuming you also fix the pointer errors.

Answer 3

These two statements are wrong:

(*students[NumOfStudents]).firstName=(char*)malloc(sizeof(char[30]));
(*students[NumOfStudents]).lastName=(char*)malloc(sizeof(char[30]));

C-arrays are zero-indexed, and so you are trying to dereference one past the size of the array. Assuming that you want to set the first and last name of the last element of students[], then you need to dereference index NumOfStudents - 1.

You don't need to cast the result of malloc() to char * (assuming you are writing a C application).

The sizeof(char) is 1, and so you just need to write malloc(30).

Answer 4

(*students[NumOfStudents]).firstName=(char*)malloc(sizeof(char[30]));

I think you meant:

students[some_index]->firstName = malloc(30);

With some_index something below NumOfStudents.

So your loop becomes:

for(i=0; i < NumOfStudents ; i++)
    {
        /* students[] is an array of pointers
        ** students[i] is a pointer
        */
        students[ i ] = malloc(sizeof *students[ i ]);
        students[ i ]->idNumber=i;

        students[ i ]->firstName = malloc(30);
        students[ i ]->lastName= malloc(30);

        fscanf(fp,"%s %s"
            , students[i]->firstName
            , students[i]->lastName
            );
        fscanf(fp,"%f %f %f %f"
            , &students[i]->marks.firstAssignment
            , &students[i]->marks.secondAssignment
            , &students[i]->marks.midterm
            , &students[i]->marks.final
            );
        printf("%s",students[i]->firstName);//, students[i].lastName);
    }

And using a global index named "i" can be considered bad style.