Fix: FileStream.Dispose silently fails on Dispose when disk has run out of space...
authorCarlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
Tue, 11 Aug 2020 16:39:56 +0000 (09:39 -0700)
committerGitHub <noreply@github.com>
Tue, 11 Aug 2020 16:39:56 +0000 (09:39 -0700)
* 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 <jkotas@microsoft.com>
Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln
src/libraries/System.IO.FileSystem/tests/ManualTests/ManualTests.cs [new file with mode: 0644]
src/libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj [new file with mode: 0644]
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs

index e18c3a1..3d4bdc1 100644 (file)
@@ -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
new file mode 100644 (file)
index 0000000..0e8ae5b
--- /dev/null
@@ -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 remoteuser@xxx.xxx.xxx.xxx:/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/src/libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj b/src/libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj
new file mode 100644 (file)
index 0000000..3f5205d
--- /dev/null
@@ -0,0 +1,8 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="ManualTests.cs" />
+  </ItemGroup>
+</Project>
index 61b240f..d8f83af 100644 (file)
@@ -92,19 +92,9 @@ namespace Microsoft.Win32.SafeHandles
             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 @@ namespace Microsoft.Win32.SafeHandles
             // 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;
         }
 
index 8d823b5..e7f8852 100644 (file)
@@ -267,6 +267,20 @@ namespace System.IO
                         // 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