Why am I getting a Segmentation Fault when reading in a text file and storing into an array?

Question

I need to read in a line of text, store it into an array. When I compile the program, it works but then when I execute it, I receive a segmentation fault. I have read other questions and tried what they have suggested and nothing seems to be working.

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

void main() {

FILE *file;
char text[10000000], *line[10000000];
int i=0;

file = fopen("/home/Documents/xxxxxx_HW01/text.txt", "r");
if(file == NULL) {
    printf("cannot open file");
    exit(1);
}

while (i< 10000 && fgets(text, sizeof(text), file)!= NULL){
    line[i] = strdup(text);
    i++;
}

for (i=0; text[i] != '\0'; i++)
    printf("%c ", text[i]);

fclose(file);

}

`*file = *fopen(` is not correct, is should be just `file = fopen(`. — Bo Persson, Sep 27 '17 at 02:23
`text[i] = strdup(text);` is overwriting pointer after pointer after pointer. How about `char text[1000], *lines[1000];` and then `while (i < 1000 && fgets(text, sizeof(text), file)!= NULL){ lines[i] = strdup(text); i++; }`? Also See: [**C11 Standard §5.1.2.2.1 Program startup (draft n1570)**](http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf). See also: [**See What should main() return in C and C++?**](http://stackoverflow.com/questions/204476/) — David C. Rankin, Sep 27 '17 at 02:25
You should not declare huge arrays like those on the stack. That alone may give segmentation faults. — Lundin, Sep 27 '17 at 06:44

David C. Rankin · Accepted Answer · 2017-09-27T04:51:48.387

Continuing from my comment,

text[i] = strdup (text);

Is wrong. It attempts to assign a pointer (the result of strdup) to text[i] a signed char value. You need to use a separate array of pointers (or declare a pointer to pointer to char, e.g. char **lines; and then allocate pointers and then for each line).

The most important thing you can do is Listen to what your compiler is telling you. To make sure you are getting the benefit of your compilers help, always compile with warnings enabled and do not accept code until it compiles without warning. For gcc that means adding at minimum -Wall -Wextra to your compile string. For clang, -Weverything will do. For cl.exe (VS) add /Wall. Then read and understand the warnings. The compiler will give you the exact line where any problem occurs.

If you are simply reading lines less than some number, you can avoid allocating pointer and just use an array of pointers, but you must keep track of the index (to avoid writing beyond the last element)

Based on what you are attempting, it looks like you are trying to do something similar to the following:

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

#define MAX 1000

int  main (void) {

    FILE *file;
    char text[MAX] = "", 
        *lines[MAX] = {NULL};
    int i=0, j;

    file = fopen ("/home/Documents/xxxxxx_HW01/text.txt", "r");

    if(file == NULL) {
        printf("cannot open file");
        exit(1);
    }

    while (i < MAX && fgets (text, sizeof(text), file)!= NULL){
        size_t len = strlen (text);        /* get length */
        if (len && text[len-1] == '\n')    /* check if last is '\n' */
            text[--len] = 0;               /* overwrite with '\0' */
        else {  /* line too long - character remain unread */
            fprintf (stderr, "error: line exceeds %d chars.\n", MAX - 2);
            exit (EXIT_FAILURE);
        }
        lines[i] = strdup(text);
        i++;
    }

    for (j = 0; j < i; j++) {
        printf ("line[%3d]  %s\n", j, lines[j]);
        free (lines[j]);    /* don't forget to free memory */
    }

    fclose(file);

    return 0;   /* main() is type int and therefore returns a value */
}

note: you should also remove the trailing '\n' included at the end of text by fgets -- example given above.

Look things over and let me know if you have further questions.

So can you explain why you are using the "if" clause? Like what are you checking for and why do you have to check for it? — Sidney H, Sep 27 '17 at 02:49
When using `fgets` it will read up to and include the line ending (`'\n'`) in `text`. You don't want `'\n'` dangling off the end of each string you store or it will mess of comparisons, etc.. So you trim them by overwriting them with a *nul-terminating* character `'\0'` (which is equivalent to zero) `text[length-1]` will always be `'\n'` if an entire line has been read. If the line is `1025-chars`, then you will not find a `'\n'` and you will know additional character remain in that line unread. — David C. Rankin, Sep 27 '17 at 02:53
@SidneyH, note the change to `if (len && text[len-1] == '\n')`, it was missing `len` at the beginning to make sure length was at least `1` before checking `len - 1` and before `--len`. (otherwise Undefined Behavior will result if you reference a negative array index) — David C. Rankin, Sep 27 '17 at 04:53

score 0 · Answer 2 · answered Sep 27 '17 at 02:49

From what I remember sizeof will give you the size of the object type, and the fgets expects the maximum amount of chars you want to read, so you probably don’t want to use sizeof there. Also you are increasing the index of your array indefinitely which is most likely going to give you a out of bounds exception.

Summing up I think you should try passing directly the size you set on your array on the fgets, and if you dont need the file to be all stored at the same time, just don’t increase i and handle each chunk in one iteration of the loop. If you do need the file all stored at once, make sure you have an array as big as your file size and perhaps use fread like this

Why am I getting a Segmentation Fault when reading in a text file and storing into an array?

2 Answers2