Improve reliability of System.IO.MemoryMappedFiles

When a MemoryMappedFile is created backed by memory (rather than by a file), in most cases we create a temporary backing store for it to be used by mmap when creating the views.  That backing store may be either a POSIX shared memory object or a temporary file, depending on the platform, and it enables us to better match the exposed API, e.g. enabling multiple views over the same data where some have copy-on-write access, etc.  However, the current implementation only unlinks that backing store when it's Disposed; this means that if the process crashes, the backing store could be left around indefinitely (in the case of a memory object, until restart, and in the case of a temporary file, until tmps are cleared out).

This change moves the unlink up to be done immediately after the backing store is created and opened, and then cleans up a bunch of cruft left over that's no longer necessary.  This has multiple advantages: in particular, we'll clean up the backing store much more reliably in the face of crashes, and the name will only be visible to others on the system for the briefest of moments.

As I was changing these files, I took the opportunity to rename them away from the initial platform they were targeting and instead based on their characteristics.  This should make it easier for other systems, e.g. FreeBSD, to use whichever implementation best suits its need.

Author: stephentoub

src/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs

@@ -13,14 +13,6 @@ namespace Microsoft.Win32.SafeHandles
 {
     public sealed partial class SafeMemoryMappedFileHandle : SafeHandle
     {
-        /// <summary>Indicates where the FileHandle came from, which then controls if/how it should be cleaned up.</summary>
-        internal enum FileStreamSource
-        {
-            Provided,
-            ManufacturedFile,
-            ManufacturedSharedMemory,
-        }
-
         /// <summary>Counter used to produce a unique handle value.</summary>
         private static long s_counter = 0;
 
@@ -31,15 +23,8 @@ internal enum FileStreamSource
         /// </summary>
         internal readonly FileStream _fileStream;
 
-        /// <summary>Indication as to where the file stream came from, if it exists.</summary>
-        internal readonly FileStreamSource _fileStreamSource;
-
-        /// <summary>
-        /// The name of the map, currently used to give internal names to anonymous,
-        /// memory-backed maps.  This will be null if the map is file-backed or in
-        /// some corner cases of memory-backed maps, e.g. read-only.
-        /// </summary>
-        internal readonly string _mapName;
+        /// <summary>Whether this SafeHandle owns the _fileStream and should Dispose it when disposed.</summary>
+        internal readonly bool _ownsFileStream;
 
         /// <summary>The inheritability of the memory-mapped file.</summary>
         internal readonly HandleInheritability _inheritability;
@@ -54,24 +39,23 @@ internal enum FileStreamSource
         internal readonly long _capacity;
 
         /// <summary>Initializes the memory-mapped file handle.</summary>
-        /// <param name="mapName">The name of the map; may be null.</param>
         /// <param name="fileStream">The underlying file stream; may be null.</param>
-        /// <param name="fileStreamSource">The source of the file stream.</param>
+        /// <param name="ownsFileStream">Whether this SafeHandle is responsible for Disposing the fileStream.</param>
         /// <param name="inheritability">The inheritability of the memory-mapped file.</param>
         /// <param name="access">The access for the memory-mapped file.</param>
         /// <param name="options">The options for the memory-mapped file.</param>
         /// <param name="capacity">The capacity of the memory-mapped file.</param>
         internal SafeMemoryMappedFileHandle(
-            string mapName,
-            FileStream fileStream, FileStreamSource fileStreamSource, HandleInheritability inheritability,
+            FileStream fileStream, bool ownsFileStream, HandleInheritability inheritability,
             MemoryMappedFileAccess access, MemoryMappedFileOptions options,
             long capacity)
             : base(new IntPtr(-1), ownsHandle: true)
         {
+            Debug.Assert(!ownsFileStream || fileStream != null, "We can only own a FileStream we're actually given.");
+
             // Store the arguments.  We'll actually open the map when the view is created.
-            _mapName = mapName;
             _fileStream = fileStream;
-            _fileStreamSource = fileStreamSource;
+            _ownsFileStream = ownsFileStream;
             _inheritability = inheritability;
             _access = access;
             _options = options;
@@ -84,26 +68,17 @@ internal SafeMemoryMappedFileHandle(
 
         protected override void Dispose(bool disposing)
         {
-            if (disposing && _fileStream != null && _fileStreamSource != FileStreamSource.Provided)
+            if (disposing && _ownsFileStream)
             {
-                // Clean up the file if we created it
+                // Clean up the file descriptor (either for a file on disk or a shared memory object) if we created it
                 _fileStream.Dispose();
             }
             base.Dispose(disposing);
         }
 
         protected override unsafe bool ReleaseHandle()
         {
-            if (_fileStreamSource == FileStreamSource.ManufacturedSharedMemory)
-            {
-                Debug.Assert(_mapName != null);
-                Debug.Assert(_fileStream != null);
-                return Interop.libc.shm_unlink(_mapName) == 0;
-            }
-
-            // For _fileHandleSource == File, there's nothing to clean up, as it's either the caller's responsibility
-            // or it was created as DeleteOnClose (if it was a temporary backing store).
-
+            // Nothing to clean up.  We unlinked immediately after creating the backing store.
             return true;
         }
 

src/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj

@@ -122,9 +122,6 @@
     <Compile Include="$(CommonPath)\Interop\Unix\Interop.IOErrors.cs">
       <Link>Common\Interop\Unix\Interop.IOErrors.cs</Link>
     </Compile>
-    <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.ftruncate.cs">
-      <Link>Common\Interop\Unix\Interop.ftruncate.cs</Link>
-    </Compile>
     <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.gnu_get_libc_version.cs">
       <Link>Common\Interop\Unix\Interop.gnu_get_libc_version.cs</Link>
     </Compile>
@@ -156,8 +153,11 @@
   </ItemGroup>
   <!-- Linux -->
   <ItemGroup Condition="'$(TargetsLinux)' == 'true'">
-    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.Linux.cs" />
-    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedView.Linux.cs" />
+    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.Unix.BackingObject_Memory.cs" />
+    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedView.Unix.MADV_DONTFORK.cs" />
+    <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.ftruncate.cs">
+      <Link>Common\Interop\Unix\Interop.ftruncate.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Linux\Interop.Errors.cs">
       <Link>Common\Interop\Linux\Interop.Errors.cs</Link>
     </Compile>
@@ -185,7 +185,10 @@
   </ItemGroup>
   <!-- OSX -->
   <ItemGroup Condition="'$(TargetsOSX)' == 'true'">
-    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.OSX.cs" />
+    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.Unix.BackingObject_File.cs" />
+    <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.unlink.cs">
+      <Link>Common\Interop\Unix\Interop.unlink.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\OSX\Interop.Errors.cs">
       <Link>Common\Interop\OSX\Interop.Errors.cs</Link>
     </Compile>
@@ -204,9 +207,6 @@
     <Compile Include="$(CommonPath)\Interop\OSX\libc\Interop.SysConfNames.cs">
       <Link>Common\Interop\OSX\Interop.SysConfNames.cs</Link>
     </Compile>
-    <Compile Include="$(CommonPath)\Interop\OSX\libsystem_kernel\Interop.shm_open.cs">
-      <Link>Common\Interop\OSX\Interop.shm_open.cs</Link>
-    </Compile>
   </ItemGroup>
   <!-- Resource files -->
   <ItemGroup>

src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.OSX.cs

@@ -0,0 +1,34 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using Microsoft.Win32.SafeHandles;
+
+namespace System.IO.MemoryMappedFiles
+{
+    public partial class MemoryMappedFile
+    {
+        private static FileStream CreateSharedBackingObject(Interop.libc.MemoryMappedProtections protections, long capacity)
+        {
+            string path = TmpPathPrefix + Guid.NewGuid().ToString("N") + ".tmp";
+
+            FileAccess access =
+                (protections & (Interop.libc.MemoryMappedProtections.PROT_READ | Interop.libc.MemoryMappedProtections.PROT_WRITE)) != 0 ? FileAccess.ReadWrite :
+                (protections & (Interop.libc.MemoryMappedProtections.PROT_WRITE)) != 0 ? FileAccess.Write :
+                FileAccess.Read;
+
+            // Create the backing file, then immediately unlink it so that it'll be cleaned up when no longer in use.
+            // Then enlarge it to the requested capacity.
+            const int DefaultBufferSize = 0x1000;
+            var fs = new FileStream(path, FileMode.CreateNew, TranslateProtectionsToFileAccess(protections), FileShare.ReadWrite, DefaultBufferSize);
+            Interop.CheckIo(Interop.libc.unlink(path));
+            fs.SetLength(capacity);
+            return fs;
+        }
+
+        // -----------------------------
+        // ---- PAL layer ends here ----
+        // -----------------------------
+
+        private static readonly string TmpPathPrefix = Path.Combine(Path.GetTempPath(), MemoryMapObjectFilePrefix);
+    }
+}

src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.BackingObject_File.cs

@@ -8,12 +8,10 @@ namespace System.IO.MemoryMappedFiles
     public partial class MemoryMappedFile
     {
         private static FileStream CreateSharedBackingObject(
-            Interop.libc.MemoryMappedProtections protections, long capacity,
-            out string mapName, out SafeMemoryMappedFileHandle.FileStreamSource fileStreamSource)
+            Interop.libc.MemoryMappedProtections protections, long capacity)
         {
             // The POSIX shared memory object name must begin with '/'.  After that we just want something short and unique.
-            mapName = "/" + MemoryMapObjectFilePrefix + Guid.NewGuid().ToString("N");
-            fileStreamSource = SafeMemoryMappedFileHandle.FileStreamSource.ManufacturedSharedMemory;
+            string mapName = "/" + MemoryMapObjectFilePrefix + Guid.NewGuid().ToString("N");
 
             // Determine the flags to use when creating the shared memory object
             Interop.libc.OpenFlags flags = (protections & Interop.libc.MemoryMappedProtections.PROT_WRITE) != 0 ?
@@ -30,15 +28,24 @@ private static FileStream CreateSharedBackingObject(
             if ((protections & Interop.libc.MemoryMappedProtections.PROT_EXEC) != 0)
                 perms |= Interop.libc.Permissions.S_IXUSR;
 
-            // Create the shared memory object. Then enlarge it to the requested capacity.
+            // Create the shared memory object.
             int fd;
             Interop.CheckIo(fd = Interop.libc.shm_open(mapName, flags, (int)perms), mapName);
             SafeFileHandle fileHandle = new SafeFileHandle((IntPtr)fd, ownsHandle: true);
 
-            // Wrap the handle in a stream and return it.
-            var fs = new FileStream(fileHandle, TranslateProtectionsToFileAccess(protections));
-            fs.SetLength(capacity);
-            return fs;
+            // Unlink the shared memory object immediatley so that it'll go away once all handles 
+            // to it are closed (as with opened then unlinked files, it'll remain usable via
+            // the open handles even though it's unlinked and can't be opened anew via its name).
+            Interop.CheckIo(Interop.libc.shm_unlink(mapName));
+
+            // Give it the right capacity.  We do this directly with ftruncate rather
+            // than via FileStream.SetLength after the FileStream is created because, on some systems,
+            // lseek fails on shared memory objects, causing the FileStream to think it's unseekable,
+            // causing it to preemptively throw from SetLength.
+            Interop.CheckIo(Interop.libc.ftruncate(fd, capacity));
+
+            // Wrap the file descriptor in a stream and return it.
+            return new FileStream(fileHandle, TranslateProtectionsToFileAccess(protections));
         }
     }
 }

src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Linux.cs renamed to src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.BackingObject_Memory.cs

@@ -21,15 +21,19 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore(
         {
             if (mapName != null)
             {
-                // TODO: We currently do not support named maps.  We could possibly support 
-                // named maps in the future by using shm_open / shm_unlink, as we do for
-                // giving internal names to anonymous maps.  Issues to work through will include 
-                // dealing with permissions, passing information from the creator of the 
-                // map to another opener of it, etc.
+                // Named maps are not supported in our Unix implementation.  We could support named maps on Linux using 
+                // shared memory segments (shmget/shmat/shmdt/shmctl/etc.), but that doesn't work on OSX by default due
+                // to very low default limits on OSX for the size of such objects; it also doesn't support behaviors
+                // like copy-on-write or the ability to control handle inheritability, and reliably cleaning them up
+                // relies on some non-conforming behaviors around shared memory IDs remaining valid even after they've
+                // been marked for deletion (IPC_RMID).  We could also support named maps using the current implementation
+                // by not unlinking after creating the backing store, but then the backing stores would remain around
+                // and accessible even after process exit, with no good way to appropriately clean them up.
+                // (File-backed maps may still be used for cross-process communication.)
                 throw CreateNamedMapsNotSupportedException();
             }
 
-            var fileStreamSource = SafeMemoryMappedFileHandle.FileStreamSource.Provided;
+            bool ownsFileStream = false;
             if (fileStream != null)
             {
                 // This map is backed by a file.  Make sure the file's size is increased to be
@@ -50,15 +54,16 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore(
                 // of an object that has read-only permissions, but we also don't need to worry about sharing
                 // views over a read-only, anonymous, memory-backed map, because the data will never change, so all views
                 // will always see zero and can't change that.  In that case, we just use the built-in anonymous support of
-                // the map by leaving fileHandle as null.
+                // the map by leaving fileStream as null.
                 Interop.libc.MemoryMappedProtections protections = MemoryMappedView.GetProtections(access, forVerification: false);
                 if ((protections & Interop.libc.MemoryMappedProtections.PROT_WRITE) != 0 && capacity > 0)
                 {
-                    fileStream = CreateSharedBackingObject(protections, capacity, out mapName, out fileStreamSource);
+                    ownsFileStream = true;
+                    fileStream = CreateSharedBackingObject(protections, capacity);
                 }
             }
 
-            return new SafeMemoryMappedFileHandle(mapName, fileStream, fileStreamSource, inheritability, access, options, capacity);
+            return new SafeMemoryMappedFileHandle(fileStream, ownsFileStream, inheritability, access, options, capacity);
         }
 
         /// <summary>
@@ -109,7 +114,7 @@ private static Exception CreateNamedMapsNotSupportedException()
             return new PlatformNotSupportedException(SR.PlatformNotSupported_NamedMaps);
         }
 
-        private const string MemoryMapObjectFilePrefix = "CoreFX_MMF_";
+        private const string MemoryMapObjectFilePrefix = "corefx_mmf_";
 
         private static FileAccess TranslateProtectionsToFileAccess(Interop.libc.MemoryMappedProtections protections)
         {