Allow MemoryMappedFile to work on character device

[wfurt]

This change will push two argument checks to the platform part and it will not enforce capacity limits on Unix if the file is a special character device.

I don't know if this is sufficient to make GPIO and possibly other HW working but it is now at least possible to map /dev/* entries and read and write.

added test for /dev/zero

$ stat /dev/zero
  File: '/dev/zero'
  Size: 0         	Blocks: 0          IO Block: 4096   character special file
Device: 6h/6d	Inode: 8           Links: 1     Device type: 1,5
Access: (0666/crw-rw-rw-)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2020-01-03 13:30:23.368000000 -0800
Modify: 2020-01-03 13:30:23.368000000 -0800
Change: 2020-01-03 13:30:23.368000000 -0800
 Birth: -

fixes https://github.com/dotnet/corefx/issues/30686

[carlossanlop]

Left some comments for you to consider.

[wfurt]

I made some updates @carlossanlop and this should be ready for another pass. Touching CreateNew was a mistake and it should be fixed now.
I did more testing around MemoryMappedFileAccess.Write and things are a little bit more tricky. While I don't have a concrete use case for a particular device but my intention was to allow whatever OS can do without adding artificial limitations at .NET layer.

The mmap call will fail on /dev/zero with EACCESS unless fd is opened with O_RDWR.
Touching GetFileAccess() may be risky.
I also did more testing with other devices like /dev/null and the outcome can vary and it can fail with ENOTSUP. When an invalid name is passed in, it is difficult to preserve existing behavior with a deferred check.

So I abandoned the idea that we can open the right stream and pass proper flags for all devices using the file name. If needed, the user can create FileStream as needed and pass more options and we will pass it down to OS for final judgment. I would expect the majority of users to use MemoryMappedFileAccess.ReadWrite

[carlossanlop]

I'm back from vacation. Left a couple of additional comments for you to consider.

[stephentoub]

@wfurt, still working on this?

[wfurt]

I updated tests, changed naming and rill-back write-only support as we discussed while back @carlossanlop. This should be ready for another review round.

[carlossanlop]

I noticed that in MemoryMappedFile.cs, this change removed the code that deletes the file if it didn't use to exist (when the file was created internally via a FileStream). Now if the user calls the CreateFromFile overload that takes a string path, the file that gets created internally would never get deleted.

I also realized that the newly added behavior difference between Windows and Unix would not be obvious to understand to someone reading this in the future.

If you allow me to provide a suggestion, I think that instead of deferring the code, we should create a private method called VerifyMemoryMappedFileAccess, with a Windows version and a Unix version.

The Windows version would contain:

private void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, int capacity, FileStream fileStream, bool existed)
{
    if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length)
    {
        if (!existed)
        {
            CleanupFile(fileStream, true, path);
        }
        throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity);
    }
}

And the Unix methods would contain:

private void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, int capacity, FileStream fileStream, bool existed, bool out isRegularFile)
{
    isRegularFile = true;

    Interop.Sys.FileStatus status = default;

    if (fileStream != null)
    {
        int result = Interop.Sys.FStat(fileStream.SafeFileHandle, out status);
        if (result != 0)
        {
            Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo();
            throw Interop.GetExceptionForIoErrno(errorInfo);
        }

        isRegularFile = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) == 0;

        if (isRegularFile)
        {
            if (access == MemoryMappedFileAccess.Read && capacity > status.Size)
            {
                if (!existed)
                {
                    CleanupFile(fileStream, true, path);
                }
                throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity);
            }

            if (access == MemoryMappedFileAccess.Write)
            {
                throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access));
            }
        }
    }
}

Finally, the new VerifyMemoryMappedFileAccess method would be called right at the top of CreateCore.

What do you think, @wfurt?

cc @jozkee

[jozkee]

Now if the user calls the CreateFromFile overload that takes a string path, the file that gets created internally would never get deleted.

It would be good to add a test for that.

[stephentoub]

Other than Carlos' suggestion, generally looks good.

[wfurt]

I made changes in spirit @carlossanlop suggested and did small updates @stephentoub pointed out. Test seems to be failing for unrelated reasons.

[carlossanlop]

LGTM @wfurt, thanks for making this change. I left one more question in case you want to consider it, but it's not blocking.

[wfurt]

I look again and there are three places where CreateCore() is called - two with FileStream and one without. I moved the remaining cage to VerifyMemoryMappedFileAccess() so we can preserve order of the checks. Also the in the case when CreateFromFile is called with string, the CreateCore() is already wrapped in try/catch and it has cleanup so I don't think we need it in the VerifyMemoryMappedFileAccess() function.