eygilbert / egdb_intl

Functions to access the kingsrow international draughts endgame database
Boost Software License 1.0
4 stars 3 forks source link

problem with error return from open_file() #2

Closed eygilbert closed 8 years ago

eygilbert commented 8 years ago

When there is an error in open_file(), it returns a nullptr. This is a problem when USE_WIN_API is true because 0 is a valid HANDLE value that can be returned by CreateFile.

A solution is to add a second argument to open_file(), passed by reference, to return the FILE_HANDLE value, and use the return value of the open_file() function to indicate error status.

rhalbersma commented 8 years ago

I don't see the problem, open_file first checks the return value from CreateFile equality to INVALID_HANDLE_VALUE before returning nullptr. The MSDN docs say about the return value from CreateFile that its either INVALID_HANDLE_VALUE or a valid handle to the underlying device. Since FILE_HANDLE is simply a void*, this valid pointer cannot be equal to nullptr then.

IMO, all callers of open_file should be checking against nullptr, not against INVALID_HANDLE_VALUE. This is consistent with the std::fopen interface.

eygilbert commented 8 years ago

The value returned by CreateFile may be a pointer type, but it is not a pointer value. I looked at the value of the first handle returned by CreateFile using the debugger and it was 0xa0, which is not a pointer value. In comparison, the first FILE * returned by fopen was 0x4d4e70. In this MSDN description of HANDLE (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724457%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396), it says, "Each handle has an entry in an internally maintained table. These entries contain the addresses of the resources and the means to identify the resource type." So it's a little vague, but the wording suggests that the handle is something like a descriptor, or an index into the table, and the table has the actual pointer to the resource object. The fact that they do not use nullptr to represent an invalid handle, plus the values that we see, give weight to that interpretation. I think it's risky to assume that 0 is not a valid HANDLE value. We have to assume that they defined -1 to be an invalid handle for a good reason.

rhalbersma commented 8 years ago

Seems safe enough: http://stackoverflow.com/questions/8240660/can-createfile-ever-return-null The argument is that CloseHandle does not know which function opened the handle, so NULL is assumed never to a valid handle.

eygilbert commented 8 years ago

Although I think it is unlikely that CreateFile will ever return 0 as a valid handle, I'm not convinced by the speculation at stackoverflow. User Peter Chen argues that CloseHandle has no requirement to do something intelligent with an error return value from CreateMutex (which is null). He has 5 or 6 posts where he continues this argument, which I think are rational and reasonable. It's not a big deal, but it is relying on unspecified behavior, and there's no need to because a fix is quite simple.

rhalbersma commented 8 years ago

I think the easiest fix is to place an assert(return_value != NULL) right after the call to CreateFile. This should catch any sneaky behavior.

Alternatively, we can define a macro INVALID_FILE_HANDLE, that is either nullptr or INVALID_HANDLE_VALUE, depending on USE_WIN_API, and let all clients test the open_file return value against that.

I really don't want to cripple open_file() with extra parameters and certainly not use any WIn32 macros on Linux.

eygilbert commented 8 years ago

I think the assert is an acceptable solution. I wasn't proposing anything drastic or crippling, only to make the function look like this:

/* Return 0 for success, non-zero on error. / `int open_file(char const name, FILE_HANDLE *handle);`

But the assert is fine.