Segmentation Fault: 11 while Reading File

Question

I don't know why but I am not being able to read a txt file and I have been receiving a Segmentation Fault 11 error and I'm not sure why. I have to read a maze that was written previously on a txt file. Apparently everything looks fine for me. Does anyone see anything wrong?

I appreciate it a lot.

enum directions {DIR_UP, DIR_DOWN, DIR_LEFT, DIR_RIGHT};

typedef struct {
int y, x;
enum directions d;

int lins, cols;
char **maze;
} t_maze;



t_maze *read_maze (char *file) {

    FILE *f = fopen (file, "r");

    if (!f) {
        return NULL;
    }

    t_maze *my_maze = (t_maze *) malloc (sizeof (t_maze));
    fscanf (f, "%d %d %d %d\n", &(my_maze->lins), &(my_maze->cols), &(my_maze->x), &(my_maze->y));


    int lin;
    my_maze->maze = (char **) malloc (my_maze->lins * sizeof (char *));
    for (lin = 0; lin < my_maze->lins; lin++) {
        my_maze->maze[lin] = (char *) malloc ((my_maze->cols) * sizeof (char));
    }


    lin = 0;
    while (!feof (f)) {
        int read, col = 0;
        do {
            read = getc (f);

            if (read != '\n') {
                my_maze->maze[lin][col++] = read;
            }
        } while (read != '\n');
        lin++;
    }

    fclose (f);
    return my_maze;
}

int main (int argc, char *argv[]) {
    if (argc > 1) {

        t_maze *m = read_maze (argv[1]);

        return 0;
    }

Answer 1

Given an input file:

4 4 1 1
abcd
efgh
ijkl
mnop

Your code crashes trying to assign to my_maze->maze[4][1] on my machine; YMMV. That's out of bounds of the array, and occurs because you're using feof() — don't!

Your use of feof() is the main culprit, along with not checking that your data is valid. This code fixes the worst problems; it is far from wonderful, but it does read and free the maze successfully.

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

enum directions { DIR_UP, DIR_DOWN, DIR_LEFT, DIR_RIGHT };

typedef struct
{
    int y, x;
    enum directions d;
    int lins, cols;
    char **maze;
} t_maze;

static 
t_maze *read_maze(char *file)
{
    FILE *f = fopen(file, "r");

    if (!f)
    {
        return NULL;
    }

    t_maze *my_maze = (t_maze *) malloc(sizeof(t_maze));
    my_maze->d = DIR_UP;
    fscanf(f, "%d %d %d %d\n", &(my_maze->lins), &(my_maze->cols), &(my_maze->x), &(my_maze->y));
    if (my_maze->lins <= 0 || my_maze->lins >= 100 ||
            my_maze->cols <= 0 || my_maze->cols >= 100 ||
            my_maze->x < 0 || my_maze->x >= my_maze->cols ||
            my_maze->y < 0 || my_maze->y >= my_maze->lins)
    {
        fprintf(stderr, "Bogus maze parameters (%d, %d, %d, %d)\n",
                my_maze->lins, my_maze->cols, my_maze->x, my_maze->y);
        return NULL;
    }

    my_maze->maze = (char **) malloc(my_maze->lins * sizeof(char *));
    if (my_maze->maze == NULL)
        return NULL;
    for (int lin = 0; lin < my_maze->lins; lin++)
    {
        my_maze->maze[lin] = (char *) malloc((my_maze->cols) * sizeof(char));
        if (my_maze->maze[lin] == NULL)
        {
            fprintf(stderr, "Oops! Memory leaked too!\n");
            return NULL;
        }
    }

    int lin = 0;
    int col = 0;
    int read;
    while ((read = getc(f)) != EOF)
    {
        if (read != '\n')
        {
            if (lin >= my_maze->lins)
            {
                fprintf(stderr, "Too many lines (%d)\n", lin);
                return NULL;
            }
            if (col >= my_maze->cols)
            {
                fprintf(stderr, "Too many columns in line %d\n", lin);
                return NULL;
            }
            my_maze->maze[lin][col++] = read;
        }
        else
        {
            lin++;
            col = 0;
        }
    }

    fclose(f);
    return my_maze;
}

static void free_maze(t_maze *maze)
{
    if (maze != 0)
    {
        for (int i = 0; i < maze->lins; i++)
            free(maze->maze[i]);
        free(maze->maze);
        free(maze);
    }
}

int main(int argc, char *argv[])
{
    if (argc > 1)
    {
        t_maze *m = read_maze(argv[1]);
        free_maze(m);

    }
    return 0;
}

Notice how the EOF checking is done — 99.9% of the time, feof() is the wrong function to use (see while (!feof(file)) is always wrong). You can argue that the check for the line number being too large is in the wrong place — you can fix it, too.

Note that your code initialized everything in the maze structure except the d element. This code assigns a value to that, too. It isn't a factor in your crash — though it might have caused problems later.

Answer 2

You can add ferror() after getc to check for reading error. You need to check that read != EOF also. The problem is that even after checking feof(), you may reach end-of-file with getc(). So, inner loop must contain read != EOF condition.

Also, you have to check for lin and col so as not to assign values to unallocated memory.

 lin = 0;
 while (!feof (f) && lin < my_maze->lins) {
   int read, col = 0;
   do {
        read = getc (f);

        if (ferror(f)) {
            perror("Reading error");
            exit (1);
        }

        if (read != '\n') {
            my_maze->maze[lin][col++] = read;
        }

    } while (read != '\n' && read != EOF && col <= my_maze->cols);
    lin++;
 }

Answer 3

The reason for the segfault in the OPs code is tied to the usage of feof() to control a file input loop. There are several great discussions of the issue here, but I want to talk about the particular case at hand.

What the Manual Says About feof()

The GNU C Library Reference Manual says in 12.15:

Many of the functions described in this chapter return the value of the macro EOF to indicate unsuccessful completion of the operation. Since EOF is used to report both end of file and random errors, it's often better to use the feof function to check explicitly for end of file and ferror to check for errors. These functions check indicators that are part of the internal state of the stream object, indicators set if the appropriate condition was detected by a previous I/O operation on that stream.

This means that the end of file indicator is not set until an I/O operation tries to read past the end of file. You have to attempt a file read to find out if it is possible. If you need to distinguish between end of file and an I/O error, after an I/O operation has been attempted, use feof() and ferror().

How feof() Can Cause Problems

In the OPs code we have this loop:

while (!feof (f)) {
    int read, col = 0;
    do {
        read = getc (f);

        if (read != '\n') {
            my_maze->maze[lin][col++] = read;
        }
    } while (read != '\n');
    lin++;
}

Inside of the loop controlled by feof(), there is a loop that reads characters through a newline. When a newline is read, the outer loop condition is revisited. So, when the last newline is read, feof() evaluates to 0 since the end of file indicator has not yet been set, and the body of the outer loop is entered once more. Then the inner loop is entered, and this is where the trouble manifests. There are no more newlines to read, so there is no way to exit the loop. Segmentation fault imminent.

There is another potential problem: what if there is a file read error? This would set the error indicator, but not the end of file indicator, so the loop will continue attempting to read the file until a segfault occurs.

What You Can Do About It

The problem in the OPs code is that the outer loop body is executed once after the last character has been read, but before the end of file indicator has been set. To avoid problems here, you have to make sure that nothing bad happens as a result of the next file read attempt inside of the loop. One solution is to test the next character read for EOF, and to test for file read errors inside the loop. But this seems to defeat the purpose of using feof() in the first place. Controlling the loop with feof() and then needing further tests inside the loop is a good sign that the loop design should be rethought.

More fundamentally, the problem is that the conditional in a while() loop specifies the conditions under which the loop body is executed. Usually it is the case that we want the loop body to execute so long as read attempts are successful. A failed read attempt could mean that end of file was reached, or that an error occurred, but in either event, exiting the loop is probably appropriate. While using feof() to control the loop may sound like you are testing to see if the end of file has been reached, you are really only testing to see if the last read attempted to read past the end of file: reading the last character of a file does not set the end of file indicator. And controlling a loop this way does nothing at all about I/O errors.

The last paragraph hints at issues of code clarity. This code is not clear, in that the conditions under which meaningful code in the loop body executes are unclear from the loop statement:

while (feof()) {
    [ ... do stuff ... ]
    [ escape the loop if an I/O operation returns EOF or NULL ]
    [ oh, and also make sure to test for errors after an I/O operation ]
    [ and escape the loop if there is an I/O error ]
    [ ... do stuff ... ]
}

This code is clear:

wnile ((c = getc(fp)) != EOF) {
    [ ... do stuff ... ]
}

The simplest solution is to control the loop directly with the return value of the input function. For example, getc() returns EOF when it reads past the end of file or when an error occurs, and fgets() returns NULL under the same conditions. This way the loop terminates when an I/O operation fails, and the code can continue appropriately.