From 09048ded3891d3ebf0a396049286d2fa4cee0eff Mon Sep 17 00:00:00 2001 From: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> Date: Tue, 11 Aug 2020 09:39:56 -0700 Subject: [PATCH] Fix: FileStream.Dispose silently fails on Dispose when disk has run out of space (#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 Co-authored-by: carlossanlop Co-authored-by: Jan Kotas --- .../System.IO.FileSystem/System.IO.FileSystem.sln | 6 ++ .../tests/ManualTests/ManualTests.cs | 79 ++++++++++++++++++++++ .../System.IO.FileSystem.Manual.Tests.csproj | 8 +++ .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 21 ++---- .../src/System/IO/FileStream.Unix.cs | 14 ++++ 5 files changed, 114 insertions(+), 14 deletions(-) create mode 100644 src/libraries/System.IO.FileSystem/tests/ManualTests/ManualTests.cs create mode 100644 src/libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj diff --git a/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln b/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln index e18c3a1..3d4bdc1 100644 --- 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 new file mode 100644 index 0000000..0e8ae5b --- /dev/null +++ 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 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(() => + { + 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 index 0000000..3f5205d --- /dev/null +++ b/src/libraries/System.IO.FileSystem/tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj @@ -0,0 +1,8 @@ + + + $(NetCoreAppCurrent) + + + + + 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 index 61b240f..d8f83af 100644 --- 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 @@ namespace Microsoft.Win32.SafeHandles return ((fileinfo.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR); } - /// Opens a SafeFileHandle for a file descriptor created by a provided delegate. - /// - /// 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. - /// - /// The created SafeFileHandle. - internal static SafeFileHandle Open(Func 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; } 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 index 8d823b5..e7f8852 100644 --- 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 @@ 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 -- 2.7.4