File handle is closed after establishing file memory mapping on Windows

[IvanPizhenko]

I've had some issue with Aeron on Windows and as result have been looking at this code (in MemoryMappedFile.cpp):

MemoryMappedFile::ptr_t MemoryMappedFile::createNew(const char *filename, size_t offset, size_t size)
{
    FileHandle fd;
    fd.handle = CreateFile(filename, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

    if (fd.handle == INVALID_HANDLE_VALUE)
    {
        throw IOException(std::string("Failed to create file: ") + filename + " " + toString(GetLastError()), SOURCEINFO);
    }

    OnScopeExit tidy(
        [&]()
        {
            CloseHandle(fd.handle);
        });

    if (!fill(fd, size, 0))
    {
        throw IOException(std::string("Failed to write to file: ") + filename + " " + toString(GetLastError()), SOURCEINFO);
    }

    return MemoryMappedFile::ptr_t(new MemoryMappedFile(fd, offset, size, false));
}

While similar thing on the Linux is OK (mmap man says file handle does not need to remain open after mapping is etablished), Microsoft Docs for Windows say right the opposite (https://docs.microsoft.com/en-us/windows/win32/memory/creating-a-file-mapping-object):

The first step in mapping a file is to open the file by calling the CreateFile function. 
To ensure that other processes cannot write to the portion of the file that is mapped,
you should open the file with exclusive access.
In addition, the file handle should remain open until the process no longer needs
the file mapping object.

But what we can see here, file handle is unconditionally closed with this one:

    OnScopeExit tidy(
        [&]()
        {
            CloseHandle(fd.handle);
        });

Instead, on success it should have been saved inside MemoryMappedFile object and closed only after mapping is closed. But here is also error in the MemoryMappedFile::cleanUp() - file handle is closed first:

void MemoryMappedFile::cleanUp()
{
    if (m_file)
    {
        CloseHandle(m_file);
    }

    if (m_memory)
    {
        UnmapViewOfFile(m_memory);
    }

    if (m_mapping)
    {
        CloseHandle(m_mapping);
    }
}

while it must be closed last, after mapping is closed, i.e. move first if to the end of this function.

[tmontgomery]

Looks like the fix for #737 was added in the wrong spot.

[IvanPizhenko]

I'd say this OnScopeExit tidy ... still needed on Windows because this operator new new MemoryMappedFile(fd, offset, size, false) may throw std::bad_alloc, and then the handle will be leaked. But if new succeeded to allocate memory, constructor must have file handle inside the object, and outside we need to prevent closing file.

This might be solved like this:

OnScopeExit tidy(
        [&]()
        {
            if (fd.handle != INVALID_FILE_HANDLE) CloseHandle(fd.handle);
        });

and then

auto obj = MemoryMappedFile::ptr_t(new MemoryMappedFile(fd, offset, size, false));
fd.fileHandle = INVALID_FILE_HANDLE;
return obj;

and MemoryMappedFile constructor then has to do:

m_file = fd.handle;

in the beginning.

[tmontgomery]

Take a look at 7e2be01 and see if that handles everything. Aside from std::bad_alloc, that is. Which if that happens, leaking a HANDLE is probably the least of your worries.

[IvanPizhenko]

Yes, I would agree that if you've got std::bad_alloc, it is likely already fatal situation for the application.

[IvanPizhenko]

One more remark there.
7e2be01#r36852514

[tmontgomery]

Feel free to try latest set of changes.

[IvanPizhenko]

Thank you, this seems to solve my original issue. Still not ideal from the point of handle management, if we more closely look at the constructor of the MemoryMappedFile there still may be cases when you will leak handle under std::bad_alloc. To have that completelty solved you likely need to wrap each handle (file, mapping, view address) into a small guard class.

[tmontgomery]

If it is simple, then probably fine to handle locally. But in general, doing too much to handle std::bad_alloc or malloc failures in general just leads to more issues, IMO. Better to fail fast, bubble up and suggest the app to exit ASAP instead.