From 77a38131c481e2d847ca17ce595ffddc0920bfff Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 7 Oct 2021 08:27:27 +0200 Subject: [PATCH 1/2] SafeFileHandle.Unix: don't DeleteOnClose when lock is not succesful. In https://github.com/dotnet/runtime/pull/55153 DeleteOnClose handling moved from FileStream to SafeFileHandle. This unintentionally caused DeleteOnClose to be applied even when FileShare locking fails. As on Windows, DeleteOnClose should not take effect when sharing prevents the file from being opened. This also swaps the order of unlink and LOCK_UN in Dispose as it was prior to https://github.com/dotnet/runtime/pull/55153. Either order should be fine. --- .../tests/FileStream/SafeFileHandle.cs | 12 +++++++++ .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 27 ++++++++++--------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs index 0740eed02f0d8..f6b46a585e9ed 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs @@ -78,6 +78,18 @@ public async Task ThrowWhenHandlePositionIsChanged_async() await ThrowWhenHandlePositionIsChanged(useAsync: true); } + [Fact] + public void DeleteOnClose_FailedShareDoesNotDeleteFile() + { + string fileName = GetTestFilePath(); + + using SafeFileHandle handle = File.OpenHandle(fileName, FileMode.OpenOrCreate, FileAccess.Write, FileShare.None); + + Assert.Throws(() => File.OpenHandle(fileName, FileMode.Open, FileAccess.Write, FileShare.None, FileOptions.DeleteOnClose)); + + Assert.True(File.Exists(fileName)); + } + private async Task ThrowWhenHandlePositionIsChanged(bool useAsync) { string fileName = GetTestFilePath(); 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 1edb65fe58afb..d6822efe41189 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 @@ -121,17 +121,6 @@ private static bool DirectoryExists(string fullPath) protected override bool ReleaseHandle() { - // When the SafeFileHandle was opened, we likely issued an flock on the created descriptor in order to add - // an advisory lock. This lock should be removed via closing the file descriptor, but close can be - // interrupted, and we don't retry closes. As such, we could end up leaving the file locked, - // which could prevent subsequent usage of the file until this process dies. To avoid that, we proactively - // try to release the lock before we close the handle. - if (_isLocked) - { - Interop.Sys.FLock(handle, Interop.Sys.LockOperations.LOCK_UN); // ignore any errors - _isLocked = false; - } - // If DeleteOnClose was requested when constructed, delete the file now. // (Unix doesn't directly support DeleteOnClose, so we mimic it here.) if (_deleteOnClose) @@ -143,6 +132,17 @@ protected override bool ReleaseHandle() Interop.Sys.Unlink(_path); // ignore errors; it's valid that the path may no longer exist } + // When the SafeFileHandle was opened, we likely issued an flock on the created descriptor in order to add + // an advisory lock. This lock should be removed via closing the file descriptor, but close can be + // interrupted, and we don't retry closes. As such, we could end up leaving the file locked, + // which could prevent subsequent usage of the file until this process dies. To avoid that, we proactively + // try to release the lock before we close the handle. + if (_isLocked) + { + Interop.Sys.FLock(handle, Interop.Sys.LockOperations.LOCK_UN); // ignore any errors + _isLocked = false; + } + // Close the descriptor. Although close is documented to potentially fail with EINTR, we never want // 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. @@ -274,7 +274,6 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo private void Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize) { IsAsync = (options & FileOptions.Asynchronous) != 0; - _deleteOnClose = (options & FileOptions.DeleteOnClose) != 0; // Lock the file if requested via FileShare. This is only advisory locking. FileShare.None implies an exclusive // lock on the file and all other modes use a shared lock. While this is not as granular as Windows, not mandatory, @@ -293,6 +292,10 @@ private void Init(string path, FileMode mode, FileAccess access, FileShare share } } + // Enable DeleteOnClose when we've succesfully locked the file. + // On Windows, the locking happens atomically as part of opening the file. + _deleteOnClose = (options & FileOptions.DeleteOnClose) != 0; + // These provide hints around how the file will be accessed. Specifying both RandomAccess // and Sequential together doesn't make sense as they are two competing options on the same spectrum, // so if both are specified, we prefer RandomAccess (behavior on Windows is unspecified if both are provided). From bb68d384b1ee1fc68b9e1d89af9a4b1c205a0b9d Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 7 Oct 2021 15:00:59 +0200 Subject: [PATCH 2/2] test: add ConditionalFact IsFileLockingEnabled --- .../System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs index f6b46a585e9ed..0c49a52abbcf1 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs @@ -78,7 +78,7 @@ public async Task ThrowWhenHandlePositionIsChanged_async() await ThrowWhenHandlePositionIsChanged(useAsync: true); } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsFileLockingEnabled))] public void DeleteOnClose_FailedShareDoesNotDeleteFile() { string fileName = GetTestFilePath();