Fix: FileStream.Dispose silently fails on Dispose when disk has run o…

…ut of space (dotnet#38742) * Save last error info in SafeFileHandle.ReleaseHandle and read it in FileStream.Dispose * Add manual test * Remove Debug.Fail from SafeFileHandle, automate the drive filling step in the manual test, add suggested comments, make threadstatic field static. * Address suggestions * Update src/libraries/System.IO.FileSystem/tests/ManualTests/ManualTests.cs Co-authored-by: Jan Kotas <[email protected]> Co-authored-by: carlossanlop <[email protected]> Co-authored-by: Jan Kotas <[email protected]>
klogeaage · Aug 11, 2020 · 09048de · 09048de
1 parent 65e51b7
commit 09048de
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 14 deletions.
diff --git a/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln b/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln
@@ -22,6 +22,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{2E666815-2ED
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TestUtilities", "..\Common\tests\TestUtilities\TestUtilities.csproj", "{66810085-C596-4ED4-ACEE-C939CBD55C4E}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "System.IO.FileSystem.ManualTests", "tests\ManualTests\System.IO.FileSystem.Manual.Tests.csproj", "{70FA2031-8D6E-4127-901E-2B0D90420E08}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -44,6 +46,10 @@ Global
 		{66810085-C596-4ED4-ACEE-C939CBD55C4E}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{66810085-C596-4ED4-ACEE-C939CBD55C4E}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{66810085-C596-4ED4-ACEE-C939CBD55C4E}.Release|Any CPU.Build.0 = Release|Any CPU
+		{70FA2031-8D6E-4127-901E-2B0D90420E08}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{70FA2031-8D6E-4127-901E-2B0D90420E08}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{70FA2031-8D6E-4127-901E-2B0D90420E08}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{70FA2031-8D6E-4127-901E-2B0D90420E08}.Release|Any CPU.Build.0 = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE

diff --git a/src/libraries/System.IO.FileSystem/tests/ManualTests/ManualTests.cs b/src/libraries/System.IO.FileSystem/tests/ManualTests/ManualTests.cs
@@ -0,0 +1,79 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.IO;
+using Microsoft.DotNet.XUnitExtensions;
+using Xunit;
+
+namespace System.IO.ManualTests
+{
+    public class FileSystemManualTests
+    {
+        public static bool ManualTestsEnabled => !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MANUAL_TESTS"));
+
+        [ConditionalFact(nameof(ManualTestsEnabled))]
+        [PlatformSpecific(TestPlatforms.AnyUnix)]
+        public static void Throw_FileStreamDispose_WhenRemoteMountRunsOutOfSpace()
+        {
+            /*
+
+            Example of mounting a remote folder using sshfs and two Linux machines:
+
+            In remote machine:
+                - Install openssh-server.
+                - Create an ext4 partition of 1 MB size.
+                
+            In local machine:
+                - Install sshfs and openssh-client.
+                - Create a local folder inside the current user's home, named "mountedremote":
+                    $ mkdir ~/mountedremote
+                - Mount the remote folder into "mountedremote":
+                    $ sudo sshfs -o allow_other,default_permissions [email protected]:/home/remoteuser/share /home/localuser/mountedremote
+                - Set the environment variable MANUAL_TESTS=1
+                - Run this manual test.
+                - Expect the exception.
+                - Unmount the folder:
+                    $ fusermount -u ~/mountedremote
+            */
+
+            string mountedPath = $"{Environment.GetEnvironmentVariable("HOME")}/mountedremote";
+            string largefile = $"{mountedPath}/largefile.txt";
+            string origin = $"{mountedPath}/copyme.txt";
+            string destination = $"{mountedPath}/destination.txt";
+
+            // Ensure the remote folder exists
+            Assert.True(Directory.Exists(mountedPath));
+
+            // Delete copied file if exists
+            if (File.Exists(destination))
+            {
+                File.Delete(destination);
+            }
+
+            // Create huge file if not exists
+            if (!File.Exists(largefile))
+            {
+                File.WriteAllBytes(largefile, new byte[925696]);
+            }
+
+            // Create original file if not exists
+            if (!File.Exists(origin))
+            {
+                File.WriteAllBytes(origin, new byte[8192]);
+            }
+
+            Assert.True(File.Exists(largefile));
+            Assert.True(File.Exists(origin));
+
+            using FileStream originStream = new FileStream(origin, FileMode.Open, FileAccess.Read);
+            Stream destinationStream = new FileStream(destination, FileMode.Create, FileAccess.Write);
+            originStream.CopyTo(destinationStream, 1);
+
+            Assert.Throws<IOException>(() =>
+            {
+                destinationStream.Dispose();
+            });
+        }
+    }
+}
diff --git a/...libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj b/...libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj
@@ -0,0 +1,8 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="ManualTests.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
@@ -92,19 +92,9 @@ private static bool DirectoryExists(string fullPath)
             return ((fileinfo.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR);
         }
 
-        /// <summary>Opens a SafeFileHandle for a file descriptor created by a provided delegate.</summary>
-        /// <param name="fdFunc">
-        /// The function that creates the file descriptor. Returns the file descriptor on success, or an invalid
-        /// file descriptor on error with Marshal.GetLastWin32Error() set to the error code.
-        /// </param>
-        /// <returns>The created SafeFileHandle.</returns>
-        internal static SafeFileHandle Open(Func<SafeFileHandle> fdFunc)
-        {
-            SafeFileHandle handle = Interop.CheckIo(fdFunc());
-
-            Debug.Assert(!handle.IsInvalid, "File descriptor is invalid");
-            return handle;
-        }
+        // Each thread will have its own copy. This prevents race conditions if the handle had the last error.
+        [ThreadStatic]
+        internal static Interop.ErrorInfo? t_lastCloseErrorInfo;
 
         protected override bool ReleaseHandle()
         {
@@ -120,7 +110,10 @@ protected override bool ReleaseHandle()
             // to retry, as the descriptor could actually have been closed, been subsequently reassigned, and
             // be in use elsewhere in the process.  Instead, we simply check whether the call was successful.
             int result = Interop.Sys.Close(handle);
-            Debug.Assert(result == 0, $"Close failed with result {result} and error {Interop.Sys.GetLastErrorInfo()}");
+            if (result != 0)
+            {
+                t_lastCloseErrorInfo = Interop.Sys.GetLastErrorInfo();
+            }
             return result == 0;
         }
 

diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs
@@ -267,6 +267,20 @@ protected override void Dispose(bool disposing)
                         // name will be removed immediately.
                         Interop.Sys.Unlink(_path); // ignore errors; it's valid that the path may no longer exist
                     }
+
+                    // Closing the file handle can fail, e.g. due to out of disk space
+                    // Throw these errors as exceptions when disposing
+                    if (_fileHandle != null && !_fileHandle.IsClosed && disposing)
+                    {
+                        SafeFileHandle.t_lastCloseErrorInfo = null;
+
+                        _fileHandle.Dispose();
+
+                        if (SafeFileHandle.t_lastCloseErrorInfo != null)
+                        {
+                            throw Interop.GetExceptionForIoErrno(SafeFileHandle.t_lastCloseErrorInfo.GetValueOrDefault(), _path, isDirectory: false);
+                        }
+                    }
                 }
             }
             finally