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:
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...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.
- 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.