2

Can the following code result in undefined behavior?

FILE *fp;
fopen_s(&fp, "abc.bin", "rb");
fclose(fp);
fclose(fp); // accidentally closed an already closed file.

I know that calling free on an already freed up array results in UB. Hence I ask.

The Vivandiere
  • 3,059
  • 3
  • 28
  • 50
  • It's undefined behavior of course. – Iharob Al Asimi Apr 01 '15 at 20:08
  • 1
    In a complex situation that is hard to keep track of, just as with `free()` if you set the pointer to `NULL` at the point of declaration, and after closing or freeing, you can then check it before the `fclose()` or `free()` call to save a lot of grief. – Weather Vane Apr 01 '15 at 20:12
  • @WeatherVane: But unfortunately, in difference to `free`, it is not well defined to call `fclose` with a `NULL` pointer. I would suggest "checking" for `NULL` before `free` is a bad thing. Checking for `NULL` before calling `fclose` is definitely a good thing. – Mats Petersson Apr 01 '15 at 20:18
  • Also see [Fclose a file that is already fclose](http://stackoverflow.com/q/24555980/1708801) looks like a duplicate – Shafik Yaghmour Apr 01 '15 at 20:19
  • @MatsPetersson How about `if (!nullptr == fp) fclose(fp); ` ? – The Vivandiere Apr 01 '15 at 20:21
  • @MatsPetersson I didn't say to call `fclose(NULL)` or `free(NULL)`. I said it allows you to check the pointer status and ***not*** call those functions. but with a file, you need to be careful to distinguish that a file handle can be `0`. – Weather Vane Apr 01 '15 at 20:21
  • Right, and my point is that `free`, since about 1990 [or before], tolerates calls with a `NULL` pointer - so you should NOT check for `NULL` before calling `free` (because it's extra code that doesn't do anything meaningful), but you should before calling `fclose`. Which is rather confusing. Better to avoid both and use `fstream`. – Mats Petersson Apr 01 '15 at 20:23
  • @MatsPetersson I don't understand your quibble. Is my suggestion that you set a freed memory pointer, or a closed file pointer to `NULL` wrong, so that in the first case it is benign to `free()`, and in the second, checkable to `fclose()`? – Weather Vane Apr 01 '15 at 20:27
  • No, but your suggestion to "check for `NULL` before calling `free` leads to extra code for no good reason, wheras you NEED to do that in the case of `fclose`. – Mats Petersson Apr 01 '15 at 20:29
  • That's what I originally said. Check for `NULL`, sorry I mentioned `free()` now, but you went right off topic. – Weather Vane Apr 01 '15 at 20:30

1 Answers1

6

Quote from man fclose:

The behaviour of fclose() is undefined if the stream parameter is an illegal pointer, or is a descriptor already passed to a previous invocation of fclose().

So yes, it is undefined behavior.

Shade
  • 775
  • 3
  • 16
  • POSIX also says it's UB: http://pubs.opengroup.org/onlinepubs/009695399/functions/fclose.html – Brian Bi Apr 01 '15 at 20:10
  • Agreed. An implementation could always `new/malloc` the `FILE` in `fopen(_s)()` and `delete/free` it in `fclose()`,or it could use a private cache, and both would be legal. The caller should not make any assumptions about the validity of a `FILE*` pointer after passing it to `fclose()`, just NULL it and move it. – Remy Lebeau Apr 01 '15 at 20:10
  • Why does it say *stream parameter*? Not using streams here – The Vivandiere Apr 01 '15 at 20:10
  • 1
    @ShafikYaghmour: ISO C11 Draft N1570, 7.21.3 par. 4: `The value of a pointer to a FILE object is indeterminate after the associated file is closed (including the standard text streams).` – cremno Apr 01 '15 at 20:13