calling CloseHandle in destructor causes runtime memory error, how to properly close a file handle in a struct/class?

Question

Error message is: The instruction at "0x7c810eac" referenced memory at "0x00000000". The memory could not be "written".

If I remove the destructor everything is fine. But I do not understand what is happening here. Everywhere I read that I should close handles, but here code won't let me. (Yes I know that I can do it manually... but that is a unnecessary line in client code that I think should be handled by object.)

#include <windows.h>
#include <iostream>
#include <string>

struct fileHandle {
  HANDLE hFile;
  fileHandle(std::string path) {
    hFile = CreateFile(path.c_str(), GENERIC_WRITE, FILE_SHARE_READ, NULL, CREATE_NEW, FILE_ATTRIBUTE_ARCHIVE, NULL);
    if (hFile == INVALID_HANDLE_VALUE) {
      printf("error: INVALID_HANDLE_VALUE");
    }
  }
  ~fileHandle() {
    CloseHandle(hFile);
  }
  void save(std::string data) {
    if (!WriteFile(hFile, data.c_str(), data.size(), NULL, NULL)) {
      printf("WriteFile failed: ", GetLastError());
    }
  }
};

int main() {
  fileHandle my_handle("test_file.txt");
  my_handle.save("some text");
}

Update: this happens when file doesn't exist. When file do exist program print errors, but this is intended. I'm here asking co cover only this case when file is created (I know how to rewrite handle creating to cover existing file.)

compiler: http://sourceforge.net/projects/mingwbuilds/files/host-windows/releases/4.7.2/32-bit/threads-posix/sjlj/x32-4.7.2-release-posix-sjlj-rev7.7z

Update 2: I didn't mention that this code works and writes to file. Memory error is triggered at the very end.

Answer 1

Please compile all your code with -Wall. Saves a whole lot amount of time.

In your code printf which has an invalid format string. Correct solution for printf would be (notice the %lu):

void save(std::string data) {
  if (!WriteFile(hFile, data.c_str(), data.size(), NULL, NULL)) {
    printf("WriteFile failed: %lu", GetLastError());
  }
}

If you would have compiled with -Wall, your code would have given the warning:

filehandle.cpp: In member function 'void fileHandle::save(std::string)':
filehandle.cpp:18:50: warning: too many arguments for format [-Wformat-extra-args]

Also you should print errors to stderr as it is unbuffered. printf uses buffers and that's why you did not get any output. Also a good idea to add a \n after the errors.

Also read the other answers to improve your code. Use the rule of three.

---

After reading the comment I realized that this is not the segfault reason indeed. (Also look at Ron Burk's solution to see what went wrong.)

According to the Windows API documentation, lpNumberOfBytesWritten parameter can be NULL only when the lpOverlapped parameter is not NULL.

So you have to give a pointer to a DWORD where WriteFile can store how many bytes it actually read. The final save would be:

void save(std::string data) {
  DWORD writtenBytes;
  if (!WriteFile(hFile, data.c_str(), data.size(), &writtenBytes, NULL)) {
    printf("WriteFile failed: %lu", GetLastError());
  }
}

If the file exists, the error does not pop up, because passing INVALID_HANDLE_VALUE to WriteFile seems to make WriteFile return earlier than it uses your pointer.

Answer 2

Wrong args to WriteFile(). 2nd to last argument can't be NULL. Change it to address of DWORD that you've set to 0 and all will work. It was dying in the kernel in WriteFile() as it tried to write back the # of bytes written.

Answer 3

The documentation for CloseHandle makes it clear why this happens:

If the application is running under a debugger, the function will throw an exception if it receives either a handle value that is not valid or a pseudo-handle value.

So, when your call to CreateFile fails, the subsequent call to CloseHandle will raise an SEH exception.

The solution is that your code must only call CloseHandle if the call to CreateFile succeeded.

As others point out, your use of WriteFile is wrong. I won't repeat the details here.

Answer 4

~fileHandle() {
    if(hFile != INVALID_HANDLE_VALUE)
        CloseHandle(hFile);
  }

Answer 5

You're not following the rule of three - so ownership of hFile is shared between copies of fileHandle because of the shallow, compiler generated copy-constructor and copy-assignment operator.

For example:

fileHandle my_handle("test_file.txt");
fileHandle my_second_handle = my_handle;

Which one of these should actually close the handle on destruction? The first? What will happen when the second one goes out of scope and calls the destructor?

In this case, I'd say you should disallow copying or assignment, or pinpoint ownership.

EDIT: In your snippet, the problem is probably the one pointed out by Ron, but this is still an important point.

That's not how I'd treat it though. I think if the file couldn't be opened, fileHandle shouldn't exist. What's the point of having an invalid object? I'd simply throw an exception in the constructor to disallow invalid objects, as opposed to doing the check in the destructor.

fileHandle(std::string path) {
    hFile = CreateFile(path.c_str(), GENERIC_WRITE, FILE_SHARE_READ, NULL, CREATE_NEW, FILE_ATTRIBUTE_ARCHIVE, NULL);
    if (hFile == INVALID_HANDLE_VALUE) {
       throw std::exception("file not found");
    }
  }

Answer 6

You need to check that the file was correctly opened.

 if (hFile != INVALID_HANDLE_VALUE) CloseFile(hFile);

Answer 7

to be on the safe side, use

~fileHandle() 
{

            if (hfile != INVALID_HANDLE_VALUE)
            {
                CloseHandle(hfile);
                hfile = INVALID_HANDLE_VALUE;
            }
}