CODEWITHSUNDEEP
carraysstructsegmentation-fault
user7544211
user7544211

Reputation:

Segmentation fault as a result of using fgets() to read lines from a file into a struct array

I am currently learning C and need a little bit of help with my code.

Say there is a file called "books.txt" that contains the names of several books each on a new line of the file. I am trying to grab the names of each book to use for the rest of my program.

To do this I have created the following struck:

struct bookData { // This is my struct to encapsulate book information
     char name[50]; // Name of book
     // Other struct variable
     // Other struct variable
};

Now I need to get the names of each book and put them into a struct array. Below is how I've gone about doing so.

struct bookData booksList[numBooks]; // numBooks is the number of books in "books.txt"

int i;
for(i = 0; i < numBooks; i++) {
     fgets(booksList[i].name, 50, books);
     // Books is the "books.txt" file that was opened for reading
}

When I run this code I encounter a Segmentation fault. I believe the problem lies with the use of the for-loop. However I am not sure how to correct this problem or even why the loop is causing the Segmentation fault to occur. When I simply place the line,

fgets(booksList[0].name, 50, books);

without the for-loop, no error occurs and the code runs and prints the name of the book just fine.

I am trying to understand why an error is occurring in my code. I would be very grateful if anyone could give me advice on how to fix the error. Thank you in advance for taking the time to read/answer my question!

EDIT: numBooks is essentially the number of lines in the "books.txt" files. Which translates into the number of books for this particular problem. numBooks was calculated with the following code: 

char c; 
int numBooks; 
while((c = fgetc(books)) != EOF) { 
    if(c == '\n') { 
         numBooks++; 
    } 
}

EDIT#2: Thank you everyone for your help!!!

Upvotes: 2

Views: 236

Answers (1)

autistic
autistic

Reputation: 15642

The following code is erroneous, and could be reasonably expected to cause problems:

char c; 
int numBooks; 
while((c = fgetc(books)) != EOF) { 
    if(c == '\n') { 
         numBooks++; 
    } 
}

Firstly, numBooks is uninitialised, and later use of that is likely to invoke undefined behaviour.

Secondly, though much less likely to cause problems on most systems, fgetc returns int, which typically has a wider domain (can represent more values than unsigned char). This is done for a reason. Any actual character values returned by fgetc will be as an unsigned char (i.e. positive only) value. Failure will cause EOF (negative only). This means fgetc could typically return one of 257 values, and by converting straight to char you're discarding one of those values: the error-handling one. In other words, you can no longer tell if fgetc succeeded or not. What happens when you reach EOF? You convert it to a char, treat it as a character value (when it isn't) and then try again..? Wrong answer!

In summary, fgetc returns int, so store the return value in int...

Another problem arises when numBooks reaches INT_MAX, and numBooks++; will cause an overflow. Technically this is undefined behaviour, and could theoretically cause segmentation faults... but I've never personally seen that. Nonetheless, you should probably use an unsigned type, as it wouldn't make sense to have a negative number of entries in a file, would it?

Come to think about it, if numBooks in struct bookData booksList[numBooks]; were negative (or a large enough number), you might start to see segmentation violations when you access the higher elements.

In summary: Use an unsigned type when you only expect to see positive numbers, and use dynamic allocation (e.g. malloc) for large arrays.

Note that this doesn't cover all possibilities, as you haven't provided an MCVE so it's not possible/practical to do so; there's a big possibility that your segfault is caused by other code which you've not provided. Please let me know if you update this question, so I can update this answer and keep the world spinning :)

Upvotes: 2

Related Questions