3

Here is my relatively simple scenario, and I'm wondering if I can save myself a conditional and tidy the code up a bit. Its production code that someone else wrote and I'm just tidying up. It had no fclose at all, so all I have added is the conditional fclose() lines:

  FILE *fp;
  struct stat sb;
  /* snipped code */
  if (((fp = fopen (config_file, "r+")) == NULL) || (fstat (fileno (fp), &sb))) {
      syslog (LOG_ERR, "Fault. Unable to read config file");
      if (fp != NULL) {
          fclose (fp);
      }
      return -1;
  }
  /* code carries on after this */

The question is, do I really need to have the if(fp != null) in my code? What are the implications of just doing an fclose(fp) without checking? I had a read of the C89 standard but it wasn't clear to me from that what the results would be.

Cheers in advance

Steve

Steve
  • 1,439
  • 2
  • 14
  • 27
  • 5
    No, you don't, because `fp` is *guaranteed* to be null at that point. Did you forget your earlier check? `if (((fp = fopen (config_file, "r+")) == NULL)`. Your if statement is useless and will never be entered – Ed S. Feb 04 '13 at 03:35
  • 1
    You may have intended to put `||` instead of `&&` in the first `if` statement's condition. – John Colanduoni Feb 04 '13 at 03:36
  • Sorry, yes that should have been || not && – Steve Feb 04 '13 at 03:42

2 Answers2

4

fclose on a null pointer has undefined behaviour. This means it could segfault or cause you problems. I would stick with your checks as they are good practise and make the code easier to read.

Fantastic Mr Fox
  • 32,495
  • 27
  • 95
  • 175
  • Thanks for the clarification... I'll leave the conditional where it is. Cheers – Steve Feb 04 '13 at 03:50
  • @Steve: The second check (`if (fp != NULL)`) along with the call to `fclose()` it protects is **not** necessary! As already mentioned by *Ed. S*'s comment on the OP. – alk Feb 04 '13 at 07:40
1

In BSD, fclose( NULL ) will segfault. I believe that the classic description of what may happen is that nasal demons may fly out of your nose, but am uncertain if the behavior is undefined or unspecified. I've never considered it a terribly important distinction: just don't do it.

William Pursell
  • 204,365
  • 48
  • 270
  • 300
  • Thanks, the idea of anything flying out of any orifice is probably enough for me to keep the conditional in place :) – Steve Feb 04 '13 at 03:50