freading from a file that was fclosed before reading

user_8732793 0
Viewed 102

I spend a sleepless night yesterday trying to track down a bug in my test case. My interface looks something like this:

image read_image(FILE *file) {
  if (file == nullptr) {
    //throw exception 
  }
  //call ftell and fread on the file
  //but not fclose
  ...
  //return an image
}

Turns out one of my test cases tested whether my code can handle reading from a file that was first opened (so the file pointer was not nullptr), but closed before I pass it to my function, something like this:

FILE *img_file = fopen("existing_image.png", "r");
REQUIRE(img_file != nullptr); //this passes!
fclose(img_file);
auto my_image = image_read(file);

//... then somewhere down in completely
//unrelated test cases I get segfaults,
//double free errors and the like

Then I spent hours trying to track down segfaults, double frees in completely unrelated parts of my code until I removed that particular test case. This seemed to solve it.

My questions are:

  1. I know calling fread/ftell on a closed file is a dumb idea but could it really cause that kind of memory corruption? I looked around on e.g. cppreference but it was never explicitly specified that passing a closed stream is undefined behavior...
  2. Is there any way of finding out if a file was closed before reading from it? (I looked on SO, but the answer seems: no.)

Additional Info

I am using C++17 and gcc 9.3.0 to compile. The reason I have to deal with FILE * at all is because I am receiving these pointers from an external C API.

fclose fread fopen c++
3 Answers

Yes it can cause memory corruption because a FILE * might have allocated memory. Probably using malloc.

What happens to your program if you try to use a pointer from malloc after you used free on it?

Yes, everything breaks. Do not do that.

Zan_Lynx_134220

The power and efficiency of the C and C++ languages come with great responsibility: the programmer must be cautious about the life cycle or every object.

C++ make this easier with smart pointers and RAII, but C lacks these paradigms so every pointer is a potential source of undefined behavior. Pointers received from C APIs are a good example.

You could set the FILE * to NULL after every fclose but this will not solve the problem if the FILE pointer was received as an argument or duplicated some other way.

There is no standard API to check if a pointer is valid, nor in this particular case if a FILE * refers to an open stream. To make things worse, FILE pointers are usually recycled quickly, so a stale FILE * may very well refer to a newly open file, different from the one for which it was originally received.

user_45932670
  • I know calling fread/ftell on a closed file is a dumb idea but could it really cause that kind of memory corruption? I looked around on e.g. cppreference but it was never explicitly specified that passing a closed stream is undefined behavior...

Trying fread or ftell on a FILE* that's been closed will make both functions return -1 and set errno to an appropriate value on many systems - but you can usually avoid this by checking if the FILE* is valid.

  • Is there any way of finding out if a file was closed before reading from it? (I looked on SO, but the answer seems: no.)

In Posix systems and Windows (and possibly others), yes. Posix fileno() and Windows _fileno() returns -1 if the argument isn't a valid stream, like after it's been closed.

You could therefore create a RAII wrapper that takes ownership of the FILE* and checks if it's valid at construction. If it passes this test, the risk of anything in your code closing it when it's not supposed to will be very low.

Here's an outline of such a wrapper:

class File {
public:
    File(std::FILE* fp) : file(validate(fp)) {
        if(!file) throw std::runtime_error("I don't like nullptr");
    }

    template<typename T, std::size_t N>
    auto read(T(&buf)[N], std::size_t nmemb = N) {
        if(N < nmemb) throw std::runtime_error("reading out of bounds");
        return fread(buf, sizeof(T), nmemb, file.get());
    }

    template<typename T, std::size_t N>
    auto write(const T(&buf)[N], std::size_t nmemb = N) {
        if(N < nmemb) throw std::runtime_error("writing out of bounds");
        return fwrite(buf, sizeof(T), nmemb, file.get());
    }

private:
    std::FILE* validate(std::FILE* fp) {
#if defined(_POSIX_C_SOURCE)
        if(::fileno(fp) == -1) throw std::runtime_error(std::strerror(errno));
#elif defined(_WIN32)
        if(::_fileno(fp) == -1) throw std::runtime_error(std::strerror(errno));
#endif
        return fp;
    }
    struct fcloser {
        auto operator()(std::FILE* fp) const {
            return std::fclose(fp); 
        } 
    };

    std::unique_ptr<FILE, fcloser> file;
};

It'd need seeking / telling member functions etc. too, but this should keep your pointer reasonably safe.

Demo

user_75822470
Related