Reputation: 23
I have a relatively simple program that keeps crashing after reading integers from a file. It crashes upon execution of the fclose line. I have localized the error to be in this function.
// Read array from text file
int * fileRead() {
FILE *file;
file = fopen("test.txt", "r");
// Check if the file exists
if(file == NULL){
printf("There was a problem opening the file");
exit(1);
}
// Count number of lines in file
int count = 0;
char c;
for (c = getc(file); c != EOF; c = getc(file)){
if (c == '\n') {
count = count + 1;
}
}
// Reset to top of file
int t = fseek(file, 0, SEEK_SET);
// Read each line and save it to temp
int *temp = malloc(sizeof(int)*count);
int num, i;
for (i = 0; i<=count; i++){
fscanf(file, "%d\n", &temp[i]);
printf("%d\n", temp[i]);
}
fclose(file);
printf("Hello World\n");
return temp;
}
The Hello World is to prove to myself it crashes at fclose exactly. The function reads ints from a file which contains only ints on separate lines (of unknown quantity) and saves them to an array and then returns that array. Thank you in advance for the help, this is my first time using c so I am not sure where to start looking for the bug.
Upvotes: 2
Views: 2478
Reputation: 144520
There are multiple problems in your code:
fgetc(fp)
returns an int
value that can have all the values of type unsigned char
and the special value EOF
. You must store this into a variable of type int
for the end of file test to be reliable.
in the loop for (i = 0; i<=count; i++){
you cause a buffer overflow as the maximum allowed index into the allocated block is count-1
. This is definitely causing undefined behavior and may well explain the observed behavior.
Use this instead:
for (i = 0; i < count; i++) ...
you must pass the length of the allocated array back to the caller.
counting the number of lines does not accurately determine the number of entries in the file. It would be better to parse the file twice or to parse it just once and reallocate the array dynamically. The latter approach is necessary for non seekable input streams such as terminals and pipes.
Here is a modified version of your code:
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int *fileRead(int *countp) {
// Check if the file exists
FILE *file = fopen("PATH.txt", "r");
if (file == NULL) {
fprintf(stderr, "Cannot open input file PATH.txt: %s\n",
strerror(errno));
exit(1);
}
// Count number of lines in file
int num, count = 0;
while (fscanf(file, "%d", &num) == 1) {
count++;
}
// Reset to top of file
fseek(file, 0, SEEK_SET);
// Read each line and save it to temp
int *temp = calloc(sizeof(int), count);
if (temp != NULL) {
int i;
for (i = 0; i < count; i++) {
if (fscanf(file, "%d", &temp[i]) != 1) {
fprintf(stderr, "error reading element number %d\n", i);
break;
}
}
*countp = i; // number of entries successfully converted
}
fclose(file);
return temp;
}
int main(void) {
int count;
int *t = fileRead(&count);
if (t != NULL) {
for (int i = 0; i < count; i++) {
printf("%d\n", t[i]);
}
free(t);
}
return 0;
}
Upvotes: 1
Reputation: 14856
NOTES:
Indices in c starts from 0. So if you want to save count
integers, you have to iterate till count-1
.
i.e.
i < count
i <= count-1
Your reading is wrong, as you assume your integers are of one digit.
#include<stdio.h>
#include<stdlib.h>
int * fileRead() {
FILE *file;
file = fopen("PATH.txt", "r");
// Check if the file exists
if(file == NULL){
printf("There was a problem opening the file");
exit(1);
}
// Count number of lines in file
int count = 0;
char pc = '\n';
char c;
while (c = fgetc(file), c != EOF)
{
if (c == '\n' && pc != '\n')
count++;
pc = c;
}
// Reset to top of file
fseek(file, 0, SEEK_SET);
// Read each line and save it to temp
int *temp = malloc(sizeof(int)*count);
int num, i;
for (i=0; i<count; i++)
{
fscanf (file, "%d", &temp[i]);
}
fclose(file);
return temp;
}
int main()
{
int *t = fileRead();
printf("%d\n", t[0]);
printf("%d\n", t[1]);
}
FILE:
452
55
OUTPUT:
542
55
To sum it up:
fclose() crashing my program.
No. It wasn't fclose
it was you trying to access memory not allocated.
Upvotes: 1