[iostreams]mapped_file bug on Win32?
Or maybe a feature? Or maybe I'm just using it incorrectly. I'm trying to create a new file of a certain size, regardless of whether or not a file of the same name exists. If I run a test program along the lines of boost::iostreams::mapped_file file; boost::iostreams::mapped_file_params p("test.txt"); p.new_file_size = 1201; p.length = 1; p.mode = ios_base::in | ios_base::out; file.open(p); It fails with the error 'failed setting file size: Cannot create a file when that file already exists.' if file 'test.txt' already exists. It seems to me that this error is a bug caused by incorrect error checking in mapped_file.cpp in function mapped_file_impl::open_file. At the call to CreateFileA, dwCreationDisposition=CREATE_ALWAYS and a valid handle value is returned. The Win32 docs say that in this case the call to CreateFileA has succeeded and the last-error code is set to ERROR_ALREADY_EXISTS. http://msdn.microsoft.com/en-us/library/aa363858%28VS.85%29.aspx Immediately after the call to CreateFileA, the handle is checked for validity but the last-error code is not reset. Shortly thereafter is a call to SetFilePointer. The docs say that the proper way to check for an error from SetFilePointer is to compare its return value to INVALID_SET_VALUE_POINTER. http://msdn.microsoft.com/en-us/library/aa365541%28VS.85%29.aspx This isn't done in the code... Instead, GetLastError() is called, which apparently returns the ERROR_ALREADY_EXISTS code generated by CreateFileA, which wasn't an error at all. It seems to me that a couple of things need to be changed to make this correct- 1) after the check for valid handle creation by CreateFileA, call SetLastError(NO_ERROR) 2) when resizing the file the code if (::GetLastError() != NO_ERROR || !::SetEndOfFile(handle_)) should be something like if (::SetFilePointer(handle_, sizelow, &sizehigh, FILE_BEGIN) == INVALID_SET_VALUE_POINTER || !::SetEndOfFile(handle_)) Does that make sense? Or am I just setting the mapped_file_params incorrectly to force creation of a new file? Erik
On 2/19/2010 10:22 AM, Nelson, Erik - 2 wrote:
If I run a test program along the lines of
boost::iostreams::mapped_file file; boost::iostreams::mapped_file_params p("test.txt"); p.new_file_size = 1201; p.length = 1; p.mode = ios_base::in | ios_base::out; file.open(p);
It fails with the error
'failed setting file size: Cannot create a file when that file already exists.' if file 'test.txt' already exists.
It seems to me that this error is a bug caused by incorrect error checking in mapped_file.cpp in function mapped_file_impl::open_file.
At the call to CreateFileA, dwCreationDisposition=CREATE_ALWAYS and a valid handle value is returned. The Win32 docs say that in this case the call to CreateFileA has succeeded and the last-error code is set to ERROR_ALREADY_EXISTS.
http://msdn.microsoft.com/en-us/library/aa363858%28VS.85%29.aspx
Immediately after the call to CreateFileA, the handle is checked for validity but the last-error code is not reset.
I see that.
Shortly thereafter is a call to SetFilePointer. The docs say that the proper way to check for an error from SetFilePointer is to compare its return value to INVALID_SET_VALUE_POINTER.
http://msdn.microsoft.com/en-us/library/aa365541%28VS.85%29.aspx
According to the docs... that seems to only be true if the 3rd parameter is NULL. Otherwise: "If function succeeds and lpDistanceToMoveHigh is not NULL, the return value is the low-order DWORD of the new file pointer and lpDistanceToMoveHigh contains the high order DWORD of the new file pointer." They show an example how to handle this case in the Remarks section.
This isn't done in the code... Instead, GetLastError() is called, which apparently returns the ERROR_ALREADY_EXISTS code generated by CreateFileA, which wasn't an error at all.
It seems to me that a couple of things need to be changed to make this correct-
1) after the check for valid handle creation by CreateFileA, call SetLastError(NO_ERROR)
Something like?: if (handle_ == INVALID_HANDLE_VALUE) cleanup_and_throw("failed opening file"); +else + SetLastError(NO_ERROR);
2) when resizing the file the code
if (::GetLastError() != NO_ERROR || !::SetEndOfFile(handle_))
should be something like
if (::SetFilePointer(handle_, sizelow,&sizehigh, FILE_BEGIN) == INVALID_SET_VALUE_POINTER || !::SetEndOfFile(handle_))
Interestingly, the following code snippet is what was used prior to changeset 46692 (see: https://svn.boost.org/trac/boost/changeset/46692). This seems to more closely match the MSDN docs: DWORD result = ::SetFilePointer(handle_, sizelow, &sizehigh, FILE_BEGIN); if ( result == INVALID_SET_FILE_POINTER && ::GetLastError() != NO_ERROR || !::SetEndOfFile(handle_) )
Does that make sense? Or am I just setting the mapped_file_params incorrectly to force creation of a new file?
This I am not entirely sure of... it seems okay on the face of it. The test source has something similar (see mapped_file_test.cpp), where it does this: // Test writing to a newly created mapped file. boost::iostreams::test::temp_file first, second; boost::iostreams::test::test_file test; mapped_file_params p(first.name()); p.new_file_size = boost::iostreams::test::data_reps * boost::iostreams::test::data_length(); boost::iostreams::stream<mapped_file_sink> out; out.open(mapped_file_sink(p)); boost::iostreams::test::write_data_in_chars(out); out.close(); BOOST_CHECK_MESSAGE( boost::iostreams::test::compare_files(first.name(), test.name()), "failed writing to newly created mapped file in chars" ); I would recommend creating a new ticket at: https://svn.boost.org/trac/boost
On 2/21/2010 11:05 PM, eg wrote:
Shortly thereafter is a call to SetFilePointer. The docs say that the proper way to check for an error from SetFilePointer is to compare its return value to INVALID_SET_VALUE_POINTER.
http://msdn.microsoft.com/en-us/library/aa365541%28VS.85%29.aspx
According to the docs... that seems to only be true if the 3rd parameter is NULL.
Otherwise: "If function succeeds and lpDistanceToMoveHigh is not NULL, the return value is the low-order DWORD of the new file pointer and lpDistanceToMoveHigh contains the high order DWORD of the new file pointer."
Oops... I forgot to add the next line in the quote, which makes my point more understandable: "If the function fails, the return value is INVALID_SET_FILE_POINTER. To get extended error information, call GetLastError." I was just trying to point out there is a case in which GetLastError() should be called. The remarks section shows sample code such as the following: // Try to move hFile file pointer a huge distance DWORD dwPtrLow = SetFilePointer( hFile, lDistLow, &lDistHigh, FILE_BEGIN ); // Test for failure if ( dwPtrLow == INVALID_SET_FILE_POINTER && GetLastError() != NO_ERROR ) { // Deal with failure // . . . } // End of error handler
eg wrote:
"If the function fails, the return value is INVALID_SET_FILE_POINTER. To get extended error information, call GetLastError."
if ( dwPtrLow == INVALID_SET_FILE_POINTER && GetLastError() != NO_ERROR )
It seems to me that the GetLastError() call as part of the test only makes room for problems... The error test is *defined* as dwPtrLow == INVALID_SET_FILE_POINTER And the GetLastError() should be called only if an error is detected. In fact, if this test had been just the test against INVALID_SET_FILE_POINTER, the code would have worked correctly even without the SetLastError() clearing the error code. Erik
On 2/22/2010 10:28 AM, Nelson, Erik - 2 wrote:
eg wrote:
"If the function fails, the return value is INVALID_SET_FILE_POINTER. To get extended error information, call GetLastError."
if ( dwPtrLow == INVALID_SET_FILE_POINTER&& GetLastError() != NO_ERROR )
It seems to me that the GetLastError() call as part of the test only makes room for problems... The error test is *defined* as
dwPtrLow == INVALID_SET_FILE_POINTER
And the GetLastError() should be called only if an error is detected. In fact, if this test had been just the test against INVALID_SET_FILE_POINTER, the code would have worked correctly even without the SetLastError() clearing the error code.
You may be right as I don't see evidence in the docs of a concreted error case which contradicts this. However, I am just a little worried about the statement in the docs which say "If an error has occurred, the return value of SetFilePointer is INVALID_SET_FILE_POINTER and GetLastError returns a value other than NO_ERROR." Perhaps this is just a case of fuzzy documentation, or I am reading too much into the "...and GetLastError returns a value other than NO_ERROR".
eg wrote:
However, I am just a little worried about the statement in the docs which say "If an error has occurred, the return value of SetFilePointer is INVALID_SET_FILE_POINTER and GetLastError returns a value other than NO_ERROR."
Perhaps this is just a case of fuzzy documentation, or I am reading too much into the "...and GetLastError returns a value other than NO_ERROR".
Not sure if you're reading too much into it, but this bug is currently triggered by GetLastError() returning an error when in fact no error has occurred. The INVALID_SET_FILE_POINTER is the more conservative test, it seems to me, and if SetFilePointer returns it, there *must* have been an error, no? Erik
On 2/22/2010 11:04 AM, Nelson, Erik - 2 wrote:
eg wrote:
However, I am just a little worried about the statement in the docs which say "If an error has occurred, the return value of SetFilePointer is INVALID_SET_FILE_POINTER and GetLastError returns a value other than NO_ERROR."
Perhaps this is just a case of fuzzy documentation, or I am reading too much into the "...and GetLastError returns a value other than NO_ERROR".
Not sure if you're reading too much into it, but this bug is currently triggered by GetLastError() returning an error when in fact no error has occurred.
The INVALID_SET_FILE_POINTER is the more conservative test, it seems to me, and if SetFilePointer returns it, there *must* have been an error, no?
Ah... I see it now, the documentation describes the reason you need to check both: "Note Because INVALID_SET_FILE_POINTER is a valid value for the low-order DWORD of the new file pointer, you must check both the return value of the function and the error code returned by GetLastError to determine whether or not an error has occurred. If an error has occurred, the return value of SetFilePointer is INVALID_SET_FILE_POINTER and GetLastError returns a value other than NO_ERROR. For a code example that demonstrates how to check for failure, see the Remarks section in this topic." We need to rule out the false negative case. Wow... what a nasty API !
Eg wrote:
Ah... I see it now, the documentation describes the reason you need to check both:
"Note Because INVALID_SET_FILE_POINTER is a valid value for the low-order DWORD of the new file pointer, you must check both the return value of the function and the error code returned by GetLastError to determine whether or not an error has occurred. If an error has occurred, the return value of SetFilePointer is INVALID_SET_FILE_POINTER and GetLastError returns a value other than NO_ERROR. For a code example that demonstrates how to check for failure, see the Remarks section in this topic."
We need to rule out the false negative case. Wow... what a nasty API !
Nice find... I put in a ticket for it Erik
On 2/22/2010 1:21 PM, Nelson, Erik - 2 wrote:
Nice find... I put in a ticket for it
I created a patch to mapped_file_test so it triggers this issue. Then created a patch for mapped_file.cpp to correct it. These are now attached to the ticket at: https://svn.boost.org/trac/boost/ticket/3953
participants (2)
-
eg -
Nelson, Erik - 2