From 96921262e28b2f377d79e783e72cb903253f07e7 Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 24 Jan 2020 17:19:12 -0800 Subject: [PATCH 1/9] allow MemoryMeppedFile to work on character device --- .../MemoryMappedFile.Unix.cs | 41 +++++++++++++++++-- .../MemoryMappedFile.Windows.cs | 12 ++++++ .../IO/MemoryMappedFiles/MemoryMappedFile.cs | 21 ---------- .../MemoryMappedFile.CreateFromFile.Tests.cs | 36 ++++++++++++++++ 4 files changed, 86 insertions(+), 24 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index da7fbb0795dd85..c4c0be2b3eb92a 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -18,6 +18,39 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( HandleInheritability inheritability, MemoryMappedFileAccess access, MemoryMappedFileOptions options, long capacity) { + bool isSpecial = false; + + if (fileStream != null) + { + if (fileStream.Length == 0) + { + // If the file does not have length, it may be special unix device. + Interop.Sys.FileStatus status; + if (Interop.Sys.FStat(fileStream.SafeFileHandle, out status) == 0) + { + if ((status.Mode & Interop.Sys.FileTypes.S_IFCHR) != 0) + { + isSpecial = true; + } + } + } + + // perform deferred argument check. + if (!isSpecial) + { + // perform defered argument check. + if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) + { + throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); + } + + if (access == MemoryMappedFileAccess.Write) + { + throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); + } + } + } + if (mapName != null) { // Named maps are not supported in our Unix implementation. We could support named maps on Linux using @@ -35,10 +68,12 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( bool ownsFileStream = false; if (fileStream != null) { - // This map is backed by a file. Make sure the file's size is increased to be - // at least as big as the requested capacity of the map. - if (fileStream.Length < capacity) + + + if (capacity > fileStream.Length && !isSpecial) { + // This map is backed by a file. Make sure the file's size is increased to be + // at least as big as the requested capacity of the map for Write* access. try { fileStream.SetLength(capacity); diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs index c2399181c5a562..f84df392889c32 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs @@ -24,6 +24,18 @@ private static SafeMemoryMappedFileHandle CreateCore( SafeFileHandle? fileHandle = fileStream != null ? fileStream.SafeFileHandle : null; Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = GetSecAttrs(inheritability); + if (fileStream != null) + { + if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) + { + throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); + } + + if (access == MemoryMappedFileAccess.Write) + { + throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); + } + } SafeMemoryMappedFileHandle handle = fileHandle != null ? Interop.CreateFileMapping(fileHandle, ref secAttrs, GetPageAccess(access) | (int)options, capacity, mapName) : diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs index f743f1c49d6e0d..2d79f3ebbf7d2a 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs @@ -154,12 +154,6 @@ public static MemoryMappedFile CreateFromFile(string path, FileMode mode, string throw new ArgumentException(SR.Argument_EmptyFile); } - if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) - { - CleanupFile(fileStream, existed, path); - throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); - } - if (capacity == DefaultSize) { capacity = fileStream.Length; @@ -219,16 +213,6 @@ public static MemoryMappedFile CreateFromFile(FileStream fileStream, string? map throw new ArgumentOutOfRangeException(nameof(access)); } - if (access == MemoryMappedFileAccess.Write) - { - throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); - } - - if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) - { - throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); - } - if (inheritability < HandleInheritability.None || inheritability > HandleInheritability.Inheritable) { throw new ArgumentOutOfRangeException(nameof(inheritability)); @@ -293,11 +277,6 @@ public static MemoryMappedFile CreateNew(string? mapName, long capacity, MemoryM throw new ArgumentOutOfRangeException(nameof(access)); } - if (access == MemoryMappedFileAccess.Write) - { - throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); - } - if (((int)options & ~((int)(MemoryMappedFileOptions.DelayAllocatePages))) != 0) { throw new ArgumentOutOfRangeException(nameof(options)); diff --git a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs index 3f5e9645f95343..7f4c3e5a303845 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs @@ -5,6 +5,7 @@ using Microsoft.Win32.SafeHandles; using System.Collections.Generic; using System.Runtime.InteropServices; +using Microsoft.DotNet.XUnitExtensions; using Xunit; namespace System.IO.MemoryMappedFiles.Tests @@ -868,5 +869,40 @@ public void ReusingNames_Windows(string name) } } + /// + /// Test that we can map special character devices on Unix. + /// + [ConditionalTheory] + [InlineData(MemoryMappedFileAccess.Read)] + [InlineData(MemoryMappedFileAccess.ReadWrite)] + [InlineData(MemoryMappedFileAccess.Write)] + [PlatformSpecific(TestPlatforms.AnyUnix)] + public void OpenCharacterDevice(MemoryMappedFileAccess access) + { + string device = "/dev/zero"; + if (!File.Exists(device)) + { + throw new SkipTestException($"'{device}' is not available."); + } + + long viewCapacity = 0xFF; + + using (FileStream fs = new FileStream(device, FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite)) + using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(fs, null, viewCapacity, access, HandleInheritability.None, false)) + using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) + { + if (access != MemoryMappedFileAccess.Write) + { + byte b = view.ReadByte(0); + // /dev/zero return zeroes. + Assert.Equal(0, b); + } + + if (access != MemoryMappedFileAccess.Read) + { + view.Write(0, (byte)1); + } + } + } } } From 5154299cf0097c6d556d7b3d2e544989c85e409b Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 27 Jan 2020 14:34:50 -0800 Subject: [PATCH 2/9] feedback from review --- .../src/System.IO.MemoryMappedFiles.csproj | 3 + .../MemoryMappedFile.Unix.cs | 7 +- .../IO/MemoryMappedFiles/MemoryMappedFile.cs | 5 ++ .../MemoryMappedFile.CreateFromFile.Tests.cs | 67 ++++++++++++++++--- 4 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj b/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj index ed1097e9af5a2c..26ab1aa0bec62c 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj @@ -129,6 +129,9 @@ Common\Interop\Unix\Interop.Permissions.cs + + Common\Interop\Unix\Interop.Stat.cs + Common\Interop\Unix\Interop.SysConf.cs diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index c4c0be2b3eb92a..e3977c6ebfc495 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -19,7 +19,6 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( MemoryMappedFileOptions options, long capacity) { bool isSpecial = false; - if (fileStream != null) { if (fileStream.Length == 0) @@ -28,17 +27,13 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( Interop.Sys.FileStatus status; if (Interop.Sys.FStat(fileStream.SafeFileHandle, out status) == 0) { - if ((status.Mode & Interop.Sys.FileTypes.S_IFCHR) != 0) - { - isSpecial = true; - } + isSpecial = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) != 0; } } // perform deferred argument check. if (!isSpecial) { - // perform defered argument check. if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) { throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs index 2d79f3ebbf7d2a..4fcbc7c6c451fc 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs @@ -277,6 +277,11 @@ public static MemoryMappedFile CreateNew(string? mapName, long capacity, MemoryM throw new ArgumentOutOfRangeException(nameof(access)); } + if (access == MemoryMappedFileAccess.Write) + { + throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); + } + if (((int)options & ~((int)(MemoryMappedFileOptions.DelayAllocatePages))) != 0) { throw new ArgumentOutOfRangeException(nameof(options)); diff --git a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs index 7f4c3e5a303845..55ca60e1e8b853 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs @@ -870,14 +870,14 @@ public void ReusingNames_Windows(string name) } /// - /// Test that we can map special character devices on Unix. + /// Test that we can map special character devices on Unix using FileStream. /// [ConditionalTheory] [InlineData(MemoryMappedFileAccess.Read)] [InlineData(MemoryMappedFileAccess.ReadWrite)] [InlineData(MemoryMappedFileAccess.Write)] [PlatformSpecific(TestPlatforms.AnyUnix)] - public void OpenCharacterDevice(MemoryMappedFileAccess access) + public void OpenCharacterDeviceAsStream(MemoryMappedFileAccess access) { string device = "/dev/zero"; if (!File.Exists(device)) @@ -887,22 +887,67 @@ public void OpenCharacterDevice(MemoryMappedFileAccess access) long viewCapacity = 0xFF; - using (FileStream fs = new FileStream(device, FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite)) - using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(fs, null, viewCapacity, access, HandleInheritability.None, false)) - using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) + try { - if (access != MemoryMappedFileAccess.Write) + using (FileStream fs = new FileStream(device, FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite)) + using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(fs, null, viewCapacity, access, HandleInheritability.None, false)) + using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) { - byte b = view.ReadByte(0); - // /dev/zero return zeroes. - Assert.Equal(0, b); + if (access != MemoryMappedFileAccess.Write) + { + byte b = view.ReadByte(0); + // /dev/zero return zeroes. + Assert.Equal(0, b); + } + + if (access != MemoryMappedFileAccess.Read) + { + view.Write(0, (byte)1); + } } + } + catch (UnauthorizedAccessException) { } + // ENODEV Operation not supported by device. + catch (IOException ex) when (ex.HResult == 19) { }; + } - if (access != MemoryMappedFileAccess.Read) + /// + /// Test that we can map special character devices on Unix using file name. + /// + [ConditionalTheory] + [InlineData(MemoryMappedFileAccess.Read)] + [InlineData(MemoryMappedFileAccess.ReadWrite)] + [PlatformSpecific(TestPlatforms.AnyUnix)] + public void OpenCharacterDeviceAsFile(MemoryMappedFileAccess access) + { + string device = "/dev/zero"; + if (!File.Exists(device)) + { + throw new SkipTestException($"'{device}' is not available."); + } + + long viewCapacity = 0xFF; + + try { + using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(device, FileMode.Open, null, viewCapacity, access)) + using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) { - view.Write(0, (byte)1); + if (access != MemoryMappedFileAccess.Write) + { + byte b = view.ReadByte(0); + // /dev/zero return zeroes. + Assert.Equal(0, b); + } + + if (access != MemoryMappedFileAccess.Read) + { + view.Write(0, (byte)1); + } } } + catch (UnauthorizedAccessException) { } + // ENODEV Operation not supported by device. + catch (IOException ex) when (ex.HResult == 19) { }; } } } From ad283b9608d3b3b01497912e36626befb1c1502f Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 10 Feb 2020 11:04:37 -0800 Subject: [PATCH 3/9] feedback from review --- .../MemoryMappedFile.Unix.cs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index e3977c6ebfc495..1459573c5d1fae 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -19,22 +19,23 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( MemoryMappedFileOptions options, long capacity) { bool isSpecial = false; + Interop.Sys.FileStatus status = default; + if (fileStream != null) { - if (fileStream.Length == 0) + int result = Interop.Sys.FStat(fileStream.SafeFileHandle, out status); + if (result != 0) { - // If the file does not have length, it may be special unix device. - Interop.Sys.FileStatus status; - if (Interop.Sys.FStat(fileStream.SafeFileHandle, out status) == 0) - { - isSpecial = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) != 0; - } + Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo(); + throw Interop.GetExceptionForIoErrno(errorInfo); } + isSpecial = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) != 0; + // perform deferred argument check. if (!isSpecial) { - if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) + if (access == MemoryMappedFileAccess.Read && capacity > status.Size) { throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); } @@ -65,7 +66,7 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( { - if (capacity > fileStream.Length && !isSpecial) + if (capacity > status.Size && !isSpecial) { // This map is backed by a file. Make sure the file's size is increased to be // at least as big as the requested capacity of the map for Write* access. From b51a4077c067e5986b56ece35e1e02c6bd6460d6 Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 16 Mar 2020 21:23:32 -0700 Subject: [PATCH 4/9] feedback from review --- .../MemoryMappedFile.Unix.cs | 8 ++-- .../MemoryMappedFile.CreateFromFile.Tests.cs | 44 +++++++++---------- 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index 1459573c5d1fae..c06339d1834a67 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -18,7 +18,7 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( HandleInheritability inheritability, MemoryMappedFileAccess access, MemoryMappedFileOptions options, long capacity) { - bool isSpecial = false; + bool isRegularFile = true; Interop.Sys.FileStatus status = default; if (fileStream != null) @@ -30,10 +30,10 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( throw Interop.GetExceptionForIoErrno(errorInfo); } - isSpecial = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) != 0; + isRegularFile = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) == 0; // perform deferred argument check. - if (!isSpecial) + if (isRegularFile) { if (access == MemoryMappedFileAccess.Read && capacity > status.Size) { @@ -66,7 +66,7 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( { - if (capacity > status.Size && !isSpecial) + if (capacity > status.Size && isRegularFile) { // This map is backed by a file. Make sure the file's size is increased to be // at least as big as the requested capacity of the map for Write* access. diff --git a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs index 55ca60e1e8b853..3742d9a09e28a0 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs @@ -869,6 +869,24 @@ public void ReusingNames_Windows(string name) } } + private void ValidateDeviceAccess(MemoryMappedFile memMap, long viewCapacity, MemoryMappedFileAccess access) + { + using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) + { + if (access != MemoryMappedFileAccess.Write) + { + byte b = view.ReadByte(0); + // /dev/zero return zeroes. + Assert.Equal(0, b); + } + + if (access != MemoryMappedFileAccess.Read) + { + view.Write(0, (byte)1); + } + } + } + /// /// Test that we can map special character devices on Unix using FileStream. /// @@ -891,19 +909,8 @@ public void OpenCharacterDeviceAsStream(MemoryMappedFileAccess access) { using (FileStream fs = new FileStream(device, FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite)) using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(fs, null, viewCapacity, access, HandleInheritability.None, false)) - using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) { - if (access != MemoryMappedFileAccess.Write) - { - byte b = view.ReadByte(0); - // /dev/zero return zeroes. - Assert.Equal(0, b); - } - - if (access != MemoryMappedFileAccess.Read) - { - view.Write(0, (byte)1); - } + ValidateDeviceAccess(memMap, viewCapacity, access); } } catch (UnauthorizedAccessException) { } @@ -930,19 +937,8 @@ public void OpenCharacterDeviceAsFile(MemoryMappedFileAccess access) try { using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(device, FileMode.Open, null, viewCapacity, access)) - using (MemoryMappedViewAccessor view = memMap.CreateViewAccessor(0, viewCapacity, access)) { - if (access != MemoryMappedFileAccess.Write) - { - byte b = view.ReadByte(0); - // /dev/zero return zeroes. - Assert.Equal(0, b); - } - - if (access != MemoryMappedFileAccess.Read) - { - view.Write(0, (byte)1); - } + ValidateDeviceAccess(memMap, viewCapacity, access); } } catch (UnauthorizedAccessException) { } From 06c00dc7f14992490bfec7cde4ca7b3db5f1b3ca Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 17 Mar 2020 10:33:54 -0700 Subject: [PATCH 5/9] roll-back write only access --- .../System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs | 5 ----- .../src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs | 5 +++++ .../tests/MemoryMappedFile.CreateFromFile.Tests.cs | 1 - 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs index f84df392889c32..ed3dc24ae3d556 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs @@ -30,11 +30,6 @@ private static SafeMemoryMappedFileHandle CreateCore( { throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); } - - if (access == MemoryMappedFileAccess.Write) - { - throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); - } } SafeMemoryMappedFileHandle handle = fileHandle != null ? diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs index 4fcbc7c6c451fc..4effc6734e31ce 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs @@ -213,6 +213,11 @@ public static MemoryMappedFile CreateFromFile(FileStream fileStream, string? map throw new ArgumentOutOfRangeException(nameof(access)); } + if (access == MemoryMappedFileAccess.Write) + { + throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); + } + if (inheritability < HandleInheritability.None || inheritability > HandleInheritability.Inheritable) { throw new ArgumentOutOfRangeException(nameof(inheritability)); diff --git a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs index 3742d9a09e28a0..21e275a1b5a033 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs @@ -893,7 +893,6 @@ private void ValidateDeviceAccess(MemoryMappedFile memMap, long viewCapacity, Me [ConditionalTheory] [InlineData(MemoryMappedFileAccess.Read)] [InlineData(MemoryMappedFileAccess.ReadWrite)] - [InlineData(MemoryMappedFileAccess.Write)] [PlatformSpecific(TestPlatforms.AnyUnix)] public void OpenCharacterDeviceAsStream(MemoryMappedFileAccess access) { From e81ffc1e5a14c7c02f41431fa05e8b4af52eb186 Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 28 Apr 2020 18:01:47 -0700 Subject: [PATCH 6/9] feedback from review --- .../MemoryMappedFile.Unix.cs | 40 ++++++++++++------- .../MemoryMappedFile.Windows.cs | 18 ++++++++- .../IO/MemoryMappedFiles/MemoryMappedFile.cs | 2 +- .../MemoryMappedFile.CreateFromFile.Tests.cs | 7 ++-- 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index c06339d1834a67..5d1c29db5c8135 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -8,21 +8,18 @@ namespace System.IO.MemoryMappedFiles { public partial class MemoryMappedFile { - /// - /// Used by the 2 Create factory method groups. A null fileHandle specifies that the - /// memory mapped file should not be associated with an existing file on disk (i.e. start - /// out empty). - /// - private static unsafe SafeMemoryMappedFileHandle CreateCore( - FileStream? fileStream, string? mapName, - HandleInheritability inheritability, MemoryMappedFileAccess access, - MemoryMappedFileOptions options, long capacity) + + // This will verify file access and return file size. fileSize will return -1 for special devices. + private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream? fileStream, string? createdFile, out long fileSize) { bool isRegularFile = true; - Interop.Sys.FileStatus status = default; + fileSize = -1; if (fileStream != null) { + + Interop.Sys.FileStatus status = default; + int result = Interop.Sys.FStat(fileStream.SafeFileHandle, out status); if (result != 0) { @@ -32,11 +29,15 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( isRegularFile = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) == 0; - // perform deferred argument check. if (isRegularFile) { + fileSize = status.Size; if (access == MemoryMappedFileAccess.Read && capacity > status.Size) { + if (createdFile != null) + { + CleanupFile(fileStream, true, createdFile); + } throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); } @@ -46,6 +47,19 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( } } } + } + + /// + /// Used by the 2 Create factory method groups. A null fileHandle specifies that the + /// memory mapped file should not be associated with an existing file on disk (i.e. start + /// out empty). + /// + private static unsafe SafeMemoryMappedFileHandle CreateCore( + FileStream? fileStream, string? mapName, + HandleInheritability inheritability, MemoryMappedFileAccess access, + MemoryMappedFileOptions options, long capacity, string? createdFile = null) + { + VerifyMemoryMappedFileAccess(access, capacity, fileStream, createdFile, out long fileSize); if (mapName != null) { @@ -64,9 +78,7 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore( bool ownsFileStream = false; if (fileStream != null) { - - - if (capacity > status.Size && isRegularFile) + if (fileSize >= 0 && capacity > fileSize) { // This map is backed by a file. Make sure the file's size is increased to be // at least as big as the requested capacity of the map for Write* access. diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs index ed3dc24ae3d556..0c36a10cdbc9cd 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs @@ -11,15 +11,29 @@ namespace System.IO.MemoryMappedFiles { public partial class MemoryMappedFile { + + // This will verify file access and return file size. fileSize will -1 for special devices. + private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream? fileStream, string? createdFile, out long fileSize) + { + if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) + { + if (createdFile != null) + { + CleanupFile(fileStream, true, createdFile); + } + + throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); + } + } + /// /// Used by the 2 Create factory method groups. A null fileHandle specifies that the /// memory mapped file should not be associated with an existing file on disk (i.e. start /// out empty). /// - private static SafeMemoryMappedFileHandle CreateCore( FileStream? fileStream, string? mapName, HandleInheritability inheritability, - MemoryMappedFileAccess access, MemoryMappedFileOptions options, long capacity) + MemoryMappedFileAccess access, MemoryMappedFileOptions options, long capacity, string? createdFile = null) { SafeFileHandle? fileHandle = fileStream != null ? fileStream.SafeFileHandle : null; Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = GetSecAttrs(inheritability); diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs index 4effc6734e31ce..2769807fa10c6a 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs @@ -170,7 +170,7 @@ public static MemoryMappedFile CreateFromFile(string path, FileMode mode, string try { handle = CreateCore(fileStream, mapName, HandleInheritability.None, - access, MemoryMappedFileOptions.None, capacity); + access, MemoryMappedFileOptions.None, capacity, existed ? null : path); } catch { diff --git a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs index 21e275a1b5a033..2dd5b6303bcceb 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs @@ -896,7 +896,7 @@ private void ValidateDeviceAccess(MemoryMappedFile memMap, long viewCapacity, Me [PlatformSpecific(TestPlatforms.AnyUnix)] public void OpenCharacterDeviceAsStream(MemoryMappedFileAccess access) { - string device = "/dev/zero"; + const string device = "/dev/zero"; if (!File.Exists(device)) { throw new SkipTestException($"'{device}' is not available."); @@ -926,7 +926,7 @@ public void OpenCharacterDeviceAsStream(MemoryMappedFileAccess access) [PlatformSpecific(TestPlatforms.AnyUnix)] public void OpenCharacterDeviceAsFile(MemoryMappedFileAccess access) { - string device = "/dev/zero"; + const string device = "/dev/zero"; if (!File.Exists(device)) { throw new SkipTestException($"'{device}' is not available."); @@ -934,7 +934,8 @@ public void OpenCharacterDeviceAsFile(MemoryMappedFileAccess access) long viewCapacity = 0xFF; - try { + try + { using (MemoryMappedFile memMap = MemoryMappedFile.CreateFromFile(device, FileMode.Open, null, viewCapacity, access)) { ValidateDeviceAccess(memMap, viewCapacity, access); From e08caa9e7e1d8919008ac1881fd825d75432b5a0 Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 29 Apr 2020 12:27:59 -0700 Subject: [PATCH 7/9] fix windows --- .../IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs index 0c36a10cdbc9cd..bc5cca65b67794 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs @@ -13,7 +13,7 @@ public partial class MemoryMappedFile { // This will verify file access and return file size. fileSize will -1 for special devices. - private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream? fileStream, string? createdFile, out long fileSize) + private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream fileStream, string? createdFile) { if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) { @@ -38,12 +38,10 @@ private static SafeMemoryMappedFileHandle CreateCore( SafeFileHandle? fileHandle = fileStream != null ? fileStream.SafeFileHandle : null; Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = GetSecAttrs(inheritability); + if (fileStream != null) { - if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) - { - throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); - } + VerifyMemoryMappedFileAccess(access, capacity, fileStream, createdFile); } SafeMemoryMappedFileHandle handle = fileHandle != null ? From ebcc4c0fbbad9ebbe96339585785b886c41df602 Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Mon, 18 May 2020 16:47:14 -0700 Subject: [PATCH 8/9] avoid exception reordering --- .../MemoryMappedFile.Unix.cs | 16 +++++++++------- .../MemoryMappedFile.Windows.cs | 19 ++++++++++--------- .../IO/MemoryMappedFiles/MemoryMappedFile.cs | 15 +-------------- 3 files changed, 20 insertions(+), 30 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index 5d1c29db5c8135..ebb1a345247d66 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -10,7 +10,7 @@ public partial class MemoryMappedFile { // This will verify file access and return file size. fileSize will return -1 for special devices. - private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream? fileStream, string? createdFile, out long fileSize) + private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream? fileStream, out long fileSize) { bool isRegularFile = true; fileSize = -1; @@ -34,13 +34,15 @@ private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, fileSize = status.Size; if (access == MemoryMappedFileAccess.Read && capacity > status.Size) { - if (createdFile != null) - { - CleanupFile(fileStream, true, createdFile); - } throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); } + // one can always create a small view if they do not want to map an entire file + if (fileStream.Length > capacity) + { + throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_CapacityGEFileSizeRequired); + } + if (access == MemoryMappedFileAccess.Write) { throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access)); @@ -57,9 +59,9 @@ private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, private static unsafe SafeMemoryMappedFileHandle CreateCore( FileStream? fileStream, string? mapName, HandleInheritability inheritability, MemoryMappedFileAccess access, - MemoryMappedFileOptions options, long capacity, string? createdFile = null) + MemoryMappedFileOptions options, long capacity) { - VerifyMemoryMappedFileAccess(access, capacity, fileStream, createdFile, out long fileSize); + VerifyMemoryMappedFileAccess(access, capacity, fileStream, out long fileSize); if (mapName != null) { diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs index bc5cca65b67794..400555d6417748 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs @@ -12,18 +12,19 @@ namespace System.IO.MemoryMappedFiles public partial class MemoryMappedFile { - // This will verify file access and return file size. fileSize will -1 for special devices. - private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream fileStream, string? createdFile) + // This will verify file access and return file size. + private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream fileStream) { if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length) { - if (createdFile != null) - { - CleanupFile(fileStream, true, createdFile); - } - throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity); } + + // one can always create a small view if they do not want to map an entire file + if (fileStream.Length > capacity) + { + throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_CapacityGEFileSizeRequired); + } } /// @@ -33,7 +34,7 @@ private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, /// private static SafeMemoryMappedFileHandle CreateCore( FileStream? fileStream, string? mapName, HandleInheritability inheritability, - MemoryMappedFileAccess access, MemoryMappedFileOptions options, long capacity, string? createdFile = null) + MemoryMappedFileAccess access, MemoryMappedFileOptions options, long capacity) { SafeFileHandle? fileHandle = fileStream != null ? fileStream.SafeFileHandle : null; Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = GetSecAttrs(inheritability); @@ -41,7 +42,7 @@ private static SafeMemoryMappedFileHandle CreateCore( if (fileStream != null) { - VerifyMemoryMappedFileAccess(access, capacity, fileStream, createdFile); + VerifyMemoryMappedFileAccess(access, capacity, fileStream); } SafeMemoryMappedFileHandle handle = fileHandle != null ? diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs index 2769807fa10c6a..0f6552df57bd03 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs @@ -159,18 +159,11 @@ public static MemoryMappedFile CreateFromFile(string path, FileMode mode, string capacity = fileStream.Length; } - // one can always create a small view if they do not want to map an entire file - if (fileStream.Length > capacity) - { - CleanupFile(fileStream, existed, path); - throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_CapacityGEFileSizeRequired); - } - SafeMemoryMappedFileHandle? handle = null; try { handle = CreateCore(fileStream, mapName, HandleInheritability.None, - access, MemoryMappedFileOptions.None, capacity, existed ? null : path); + access, MemoryMappedFileOptions.None, capacity); } catch { @@ -231,12 +224,6 @@ public static MemoryMappedFile CreateFromFile(FileStream fileStream, string? map capacity = fileStream.Length; } - // one can always create a small view if they do not want to map an entire file - if (fileStream.Length > capacity) - { - throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_CapacityGEFileSizeRequired); - } - SafeMemoryMappedFileHandle handle = CreateCore(fileStream, mapName, inheritability, access, MemoryMappedFileOptions.None, capacity); From a74f600fe2ef02dddb8e82feb52bcb4291187dda Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 1 Jun 2020 13:09:51 -0700 Subject: [PATCH 9/9] feedback from review --- .../System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs | 7 ++----- .../IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs | 3 +-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index ebb1a345247d66..16bb696a017985 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -8,17 +8,14 @@ namespace System.IO.MemoryMappedFiles { public partial class MemoryMappedFile { - // This will verify file access and return file size. fileSize will return -1 for special devices. private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream? fileStream, out long fileSize) { - bool isRegularFile = true; fileSize = -1; if (fileStream != null) { - - Interop.Sys.FileStatus status = default; + Interop.Sys.FileStatus status; int result = Interop.Sys.FStat(fileStream.SafeFileHandle, out status); if (result != 0) @@ -27,7 +24,7 @@ private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, throw Interop.GetExceptionForIoErrno(errorInfo); } - isRegularFile = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) == 0; + bool isRegularFile = (status.Mode & Interop.Sys.FileTypes.S_IFCHR) == 0; if (isRegularFile) { diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs index 400555d6417748..11ffb4ecb75c86 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs @@ -11,8 +11,7 @@ namespace System.IO.MemoryMappedFiles { public partial class MemoryMappedFile { - - // This will verify file access and return file size. + // This will verify file access. private static void VerifyMemoryMappedFileAccess(MemoryMappedFileAccess access, long capacity, FileStream fileStream) { if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length)