Adding a file close statement causes a seg fault

Question

When trying to close the file after reading it, I get a seg fault on running the program.

    int inputDirectory()
    {
    char fileName[64];char directoryBuffer[256];FILE *fp;
    printf("\n> Please type the filename containing the list of directories. >");
    inputFix(fileName, sizeof(fileName));
    fp = fopen(fileName,"r");
    if(access(fileName, F_OK) == 0)
    {
        if (fp == 0)
        {
            printf("> Error opening file.");
            return 1;
        }
        else
        {
            if (access(fileName, R_OK) == 0)
            {
                while (fgets(directoryBuffer, sizeof(directoryBuffer), (FILE*)fp))
                {       
                    readCheck(directoryBuffer);
                    printf("%s \n", directoryBuffer);
                    getInode(directoryBuffer);
                }
            }
            else
            {
                printf("\n> File can't be read.");
            }
        }
    }
    else
    {
        printf("\n> File %s does not exist ", fileName); 
    }
    fclose(fp);
    return 0;
}

void inputFix(char string[],int length)
{
int ch, len = 0;
fgets(string, length, stdin);
string[strcspn(string, "\r\n")] = '\0';
len = strlen(string);
if (len == length - 1)
{
     while((ch = getchar()) != '\n' && ch != EOF);
}
}

void readCheck(char string[])
{
    string[strcspn(string, "\r\n")] = '\0';
}

Ive been reading into race conditions, but from my understanding there isn't one? Is there a need to check to see if the file exists before trying to open it? Is there a need to include some of the checks that I'm using?

"Ive been reading into race conditions" ... what lead you down that path? Is this part of a thread-proc or invoked somewhere on a free-threaded COM server or some-such? — WhozCraig, Apr 27 '14 at 04:12
Was reading into possible seg faults from File I/O. It didn't make much sense. — choczy, Apr 27 '14 at 04:16
I think it has to do with that if the file doesn't exist, it skips over and then fclose(fp) is called, where no file is opened. hence the 0x0 point of fp when an invalid file name is given. — choczy, Apr 27 '14 at 04:20
So ... this makes me wonder: `(FILE*)fp` ? That thing is *not* already a `FILE*` ?? — WhozCraig, Apr 27 '14 at 04:21
And *what* while-loop? There is **no** `fclose()` in the only while-loop of the code posted. That `fclose()` should be immediately after the `else { printf("\n> File can't be read."); }` block. There is nowhere else in this code where it makes sense to have it (and even there, it is wrong, as you should be checking access *prior* to opening the file, or testing `fp` prior to `fclose()`-ing it). Why are you invoking `access` after `fopen()`, but *before* you test the success of the `fopen()` anyway? — WhozCraig, Apr 27 '14 at 04:26
I fixed it placing a fclose() after the while loop. The code has been edited as I saw that the fopen() should be after the access call. — choczy, Apr 27 '14 at 04:58
If you open a file successfully for reading, there's no need to use `access()` too. The `access()` function is interesting. It is designed primarily for use by SUID programs to see whether the real UID (rather than the effective UID normally used for checking permissions) can access the file. In a non-SUID program, it works fine, but you're better off trying the open and spotting failure (Easier to Ask for Forgiveness than Permission: EAFP) than risking a TOCTOU (Time of Check, Time of Use) failure (Look Before You Leap: LBYL). See [SO 404795](http://stackoverflow.com/q/404795/). — Jonathan Leffler, Apr 27 '14 at 07:03

score 2 · Answer 1 · answered Apr 27 '14 at 03:31

2

Looking at these lines.

    if (fp == 0)
    {
        printf("> Error opening file.");
        fclose(fp);   // NOT NEEDED. REMOVE THE LINE
    }

It seems you don't need to call fclose when you were not able to open the file.

Remove the line.

answered Apr 27 '14 at 03:31

R Sahu

204,454
14
159
270

score 0 · Answer 2 · answered Apr 27 '14 at 03:29

0

If fp is null (equal to 0), you do not need to close it, the file was never opened to begin with. You should close fp after you are done successfully reading from it.

answered Apr 27 '14 at 03:29

Daniel

1,920
4
17
35

I've made the change to place it at the end. It still appears to seg fault. – choczy Apr 27 '14 at 03:31
What line? If you don't know how to fix a segfault, I suggest you [do some research](http://www.cprogramming.com/debugging/segfaults.html) – Daniel Apr 27 '14 at 03:32
Running it through gdb tells me its caused by fclose(fp) even after moving it. Even if it doesn't read from the file, I've opened it before going through the checks. – choczy Apr 27 '14 at 03:42
And what is the value of `fp` according to gdb? – Daniel Apr 27 '14 at 03:43
0xb7e852f2, which is weird because the program starts at 0x80486a0. 0xb7e852f2 would be a library reference right? I've used -ggdb but gdb doesn't see the debugging symbols. – choczy Apr 27 '14 at 03:45
`-g` is the flag to compile with debug info. Use `print` to print a variable. – Daniel Apr 27 '14 at 03:56
I've placed a breakpoint before the final fclose(fp). FP is the value of 0xb7fffad0. which holds another address 0xb7fffa74, which points to 0xb7fdcb18, which points to 0xb7fff918 = NULL. – choczy Apr 27 '14 at 04:10

Saba · Answer 3 · 2014-04-27T07:10:47.953

You are closing the file at the end regardless of whether the file ever opened or not. Calling fclose on an unopened file can cause a crash. Try this instead. I have moved the fclose statement to be called only when fp is not NULL.

  int inputDirectory()
    {
    char fileName[64];char directoryBuffer[256];FILE *fp;
    printf("\n> Please type the filename containing the list of directories. >");
    inputFix(fileName, sizeof(fileName));
    if(access(fileName, F_OK) == 0)
    {
        fp = fopen(fileName,"r");

        if (fp == NULL)
        {
            printf("> Error opening file.");
            return 1;
        }
        else
        {
            if (access(fileName, R_OK) == 0)
            {
                while (fgets(directoryBuffer, sizeof(directoryBuffer), (FILE*)fp))
                {       
                    readCheck(directoryBuffer);
                    printf("%s \n", directoryBuffer);
                    getInode(directoryBuffer);
                }
            }
            else
            {
                printf("\n> File can't be read.");
            }
            fclose(fp);
        }
    }
    else
    {
        printf("\n> File %s does not exist ", fileName); 
    }
    return 0;
}

alk · Answer 4 · 2014-04-27T09:32:32.350

Only call fclose() on a FILE* which had been returned by a successful call to fopen().

To test wether fopen() had been successful compare its result against NULL. If this test succeeds the call had not been successful:

#include <stdio.h>

int main(void)
{
  char filename[] = "myfile";
  FILE * fp = fopen(filename, "r");
  if (NULL == fp)
  {
    fprintf(stderr, "fopen(\"%s\", ...) failed.\n", filename);
  }
  else
  {
    fprintf(stderr, "fopen(\"%s\", ...) succeeded.\n", filename);

    /* Perform operation on fp here. */

    fclose(fp);
  }

  return 0;
}

Adding a file close statement causes a seg fault

4 Answers4