Re: [boost] [iostreams]mapped_file bug on Win32?

Subject: Re: [boost] [iostreams]mapped_file bug on Win32?
From: eg (egoots_at_[hidden])
Date: 2010-02-22 02:05:41

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