Reading from file to struct into terminal, segmentation fault

Question

I'm trying to read from a text file with similar lines (about 200 of them with some empty lines now and then). I know that something can be uninitialized, but I'm blank as to what it is. What I'm trying to do right now is just read from a file and print it into the terminal window.

I really hope you can help me! Thank you.

Example of a few lines of text file:

Fre     19/07 18.30     AGF - FCM      0 - 2     9.364   
Lor     20/07 17.00     VFF - RFC      2 - 2     4.771   
Son     21/07 14.00     OB - SDR       1 - 1     7.114

My struct looks like this:

struct match {
    char weekday[MAX_NAME_LEN];
    int day;
    int month;
    int hours;
    int minutes;
    char home_team[MAX_NAME_LEN];
    char away_team[MAX_NAME_LEN];
    int home_team_goals;
    int away_team_goals;
    int spectators_thds;
    int spectators_hdrs;
};

My read_file function looks like this:

void read_file(struct match *match) {
    FILE *fp;
    int i;
    fp = fopen("superliga-2013-2014", "r"); 
    while (!fp) {
        error("Cannot open file.");
    }

    struct match *matches = (struct match *) malloc(sizeof(match) * MAX_MATCHES);
    while(!feof(fp)) {
        fscanf(fp, " %s %i/%i %i.%i %s - %s %i - %i %i.%i \n", 
               matches[i].weekday, &matches[i].day, &matches[i].month, &matches[i].hours,
               &matches[i].minutes, matches[i].home_team, matches[i].away_team,
               &matches[i].home_team_goals, &matches[i].away_team_goals,
               &matches[i].spectators_thds, &matches[i].spectators_hdrs);
    }
    fclose(fp);
    free(fp);
}

And my main function looks like this:

int main(int argc, char *argv[]) {
    int i;
    struct match *matches;
    void read_file(struct match *matches);
    for (i = 0; i < MAX_MATCHES; ++i) {
        printf(" %s %i/%i %i.%i %s - %s %i - %i %i.%i ",
               matches[i].weekday, matches[i].day, matches[i].month, matches[i].hours,
               matches[i].minutes, matches[i].home_team, matches[i].away_team,
               matches[i].home_team_goals, matches[i].away_team_goals,
               matches[i].spectators_thds, matches[i].spectators_hdrs);
    }

    return 0;
}

It compiles with no problems, but I get a Segmentation fault when I try to run it. I then use Valgrind, and it gives me this message:

==12799== Use of uninitialised value of size 8
==12799==    at 0x4007B8: main (in /home/dradee/Dropbox/UNI/Programmering/C/readfile)
==12799== 
==12799== Invalid read of size 4
==12799==    at 0x4007B8: main (in /home/dradee/Dropbox/UNI/Programmering/C/readfile)
==12799==  Address 0x34 is not stack'd, malloc'd or (recently) free'd
==12799== 
==12799== 
==12799== Process terminating with default action of signal 11 (SIGSEGV)
==12799==  Access not within mapped region at address 0x34
==12799==    at 0x4007B8: main (in /home/dradee/Dropbox/UNI/Programmering/C/readfile)

http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong — Tim Čas, Feb 10 '15 at 22:17

score 3 · Accepted Answer · edited May 23 '17 at 12:23

Don't cast malloc()

struct match *matches = (struct match *) malloc(sizeof(match) * MAX_MATCHES);

is better as

struct match *matches = malloc(sizeof(struct match) * MAX_MATCHES);

Check the result of malloc(), it returns NULL on failure.

Don't check !feof() because it's always wrong, so change this

while(!feof(fp))

with

while (fscanf(fp, " %s %i/%i %i.%i %s - %s %i - %i %i.%i \n", 
       matches[i].weekday, &matches[i].day, &matches[i].month, &matches[i].hours,
       &matches[i].minutes, matches[i].home_team, matches[i].away_team,
       &matches[i].home_team_goals, &matches[i].away_team_goals,
       &matches[i].spectators_thds, &matches[i].spectators_hdrs) == 11)

And this is the most important: you iterate until MAX_MATCHES in the printf() loop, when you don't necessarily have read MAX_MATCHES. So you need to keep a count of the successfuly read items, and use that in the second for loop.

You can for example return the count of read items from read_file to main, and use it.

You already have a counter in read_file() but read the next point.
You never increment i in the while loop in read_file().

This is very weired

void read_file(struct match *matches);

do you mean

read_file(matches);

This is incorrect
```
free(fp);
```
you already did fclose(fp) which is the correct way to release a FILE *'s resources, the free() there will cause undefined behavior, which could be a segmentation fault, or any thing else, since it's undefined.
This makes no sense
```
while (!fp) { ... }
```
you meant
```
if (!fp) { ... }
```

Your read_file() function only localy allocates the structs and then you have no access to them from main(), what you can do is this

int read_file(struct match **matches)
{
    FILE         *fp;
    int           i;
    struct match *match;        
    if (matches == NULL)
        return 0;
    *matches = NULL;
    fp       = fopen("superliga-2013-2014", "r");
    if (!fp)
    {
        error("Cannot open file.");
        return 0;
    }        
    match = malloc(sizeof(struct match) * MAX_MATCHES);
    if (match == NULL)
        return 0;
    while ((fscanf(fp, " %s %i/%i %i.%i %s - %s %i - %i %i.%i \n",
               match[i].weekday, &match[i].day, &match[i].month, &match[i].hours,
               &match[i].minutes, match[i].home_team, match[i].away_team,
               &match[i].home_team_goals, &match[i].away_team_goals,
               &match[i].spectators_thds, &match[i].spectators_hdrs) == 11) && (i < MAX_MATCHES))
    {
        i++;
    }
    fclose(fp);

    *matches = match;
    return i;
}

and then your main()

int main(int argc, char *argv[]) 
{
    int           i;
    struct match *matches;
    int           count;
    count = read_file(&matches);
    for (i = 0 ; i < MAX_count ; ++i) 
    {
        printf(" %s %i/%i %i.%i %s - %s %i - %i %i.%i ",
               matches[i].weekday, matches[i].day, matches[i].month, matches[i].hours,
               matches[i].minutes, matches[i].home_team, matches[i].away_team,
               matches[i].home_team_goals, matches[i].away_team_goals,
               matches[i].spectators_thds, matches[i].spectators_hdrs);
    }
    free(matches);        
    return 0;
}

Thank you very much for the answer! I didn't expect such a thorough answer, and I'm grateful for that! If I could upvote you I would, but I don't have 9 reputation yet. Thank you! — jmkjaer, Feb 11 '15 at 00:50

Reading from file to struct into terminal, segmentation fault

1 Answers1