From f9d05bd2f36316a0b0f2b946406fdbccad86b08f Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 16 Nov 2021 06:45:31 +1100 Subject: [PATCH] Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix (#52639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Implement most of the fix for #38824 Reverts the changes in the seperate PR https://github.com/dotnet/runtime/commit/a617a01195b4dad3cb5f300962ad843b46d4f175 to fix #38824. Does not re-enable the test because that relies on #49555, will add a seperate commit to whichever is merged last to enable the SettingUpdatesPropertiesOnSymlink test. * Most of the code for PR #52639 to fix #38824 • Deal with merge conflicts • Add FSOPT_NOFOLLOW for macOS and use it in setattrlist • Use AT_SYMLINK_NOFOLLOW with utimensat (note: there doesn't seem to be an equivalent for utimes) • Add SettingUpdatesPropertiesOnSymlink test to test if it actually sets it on the symlink itself (to the best that we can anyway ie. write + read the times) • Specify FILE_FLAG_OPEN_REPARSE_POINT for CreateFile in FileSystem.Windows.cs (is there any other CreateFile calls that need this?) * Remove additional FILE_FLAG_OPEN_REPARSE_POINT I added FILE_FLAG_OPEN_REPARSE_POINT before and it was then added in another PR, this removes the duplicate entry. * Add missing override keywords Add missing override keywords for abstract members in the tests. * Fix access modifiers Fix access modifiers for tests - were meant to be protected rather than public * Add more symlink tests, rearrange files • Move symlink creation api to better spot in non-base files • Add IsDirectory property for some of the new tests • Change abstract symlink api to CreateSymlink that accepts path and target • Create a CreateSymlinkToItem method that creates a symlink to an item that may be relative that uses the new/modified abstract CreateSymlink method • Add SettingUpdatesPropertiesCore to avoid code duplication • Add tests for the following variants: normal/symlink, target exists/doesn't exist, absolute/relative target • Exclude nonexistent symlink target tests on Unix for Directories since they are counted as files * Fix return type of CreateSymlink in File/GetSetTimes.cs * Remove browser from new symlink tests as it doesn't support creation of symlinks Remove browser from new symlink tests as it doesn't support creation of symlinks * Use lutimes, improve code readability, simplify tests • Use lutimes when it's available • Extract dwFlagsAndAttributes to a variable • Use same year for all tests • Checking to delete old symlink is unnecessary, so don't • Replace var with explicit type * Change year in test to 2014 to reduce diff * Rename symlink tests, use 1 core symlink times function, and check that target times don't change Rename symlink tests, use 1 core symlink times function, and check that target times don't change * Inline RunSymlinkTestPart 'function' Inline RunSymlinkTestPart 'function' so that the code can be reordered so the access time test can be valid. * Share CreateSymlinkToItem call in tests and update comment for clarity * Update symlink time tests • Make SettingUpdatesPropertiesOnSymlink a theory • Remove special case for Unix due to https://github.com/dotnet/runtime/pull/52639#discussion_r739009259 (will revert if fails) • Rename isRelative to targetIsRelative for clarity * Remove unnecessary Assert.All * Changes to SettingUpdatesPropertiesOnSymlink test • Rename item field to link field • Don't use if one-liner • Use all time functions since only using UTC isn't necessary • Remove the now-defunct IsDirectory property since we aren't checking it anymore * Remove unnecessary fsi.Refresh() • Remove unnecessary fsi.Refresh() since atime is only updated when reading a file * Updates to test and pal_time.c • Remove targetIsRelative cases • Multi-line if statement • Combine HAVE_LUTIMES and #else conditions to allow more code charing * Remove trailing space --- .../Common/src/Interop/OSX/Interop.libc.cs | 2 + .../Native/Unix/Common/pal_config.h.in | 1 + .../Native/Unix/System.Native/pal_time.c | 10 ++- src/libraries/Native/Unix/configure.cmake | 5 ++ .../tests/Base/BaseGetSetTimes.cs | 64 +++++++++++++++++-- .../tests/Directory/GetSetTimes.cs | 2 + .../tests/DirectoryInfo/GetSetTimes.cs | 2 + .../tests/File/GetSetTimes.cs | 2 + .../tests/FileInfo/GetSetTimes.cs | 2 + .../src/System/IO/FileStatus.SetTimes.OSX.cs | 2 +- .../src/System/IO/FileSystem.Windows.cs | 8 ++- 11 files changed, 92 insertions(+), 8 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index d0b377977a1d6..89ba51305689f 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -25,5 +25,7 @@ internal struct AttrList [DllImport(Libraries.libc, EntryPoint = "setattrlist", SetLastError = true)] internal static unsafe extern int setattrlist(string path, AttrList* attrList, void* attrBuf, nint attrBufSize, CULong options); + + internal const uint FSOPT_NOFOLLOW = 0x00000001; } } diff --git a/src/libraries/Native/Unix/Common/pal_config.h.in b/src/libraries/Native/Unix/Common/pal_config.h.in index 9cffccbb0cc71..00b774b87341b 100644 --- a/src/libraries/Native/Unix/Common/pal_config.h.in +++ b/src/libraries/Native/Unix/Common/pal_config.h.in @@ -112,6 +112,7 @@ #cmakedefine01 HAVE_GETDOMAINNAME #cmakedefine01 HAVE_UNAME #cmakedefine01 HAVE_UTIMENSAT +#cmakedefine01 HAVE_LUTIMES #cmakedefine01 HAVE_FUTIMES #cmakedefine01 HAVE_FUTIMENS #cmakedefine01 HAVE_MKSTEMPS diff --git a/src/libraries/Native/Unix/System.Native/pal_time.c b/src/libraries/Native/Unix/System.Native/pal_time.c index 56e23138a7b77..4f9b5326cb73c 100644 --- a/src/libraries/Native/Unix/System.Native/pal_time.c +++ b/src/libraries/Native/Unix/System.Native/pal_time.c @@ -33,7 +33,7 @@ int32_t SystemNative_UTimensat(const char* path, TimeSpec* times) updatedTimes[1].tv_sec = (time_t)times[1].tv_sec; updatedTimes[1].tv_nsec = (long)times[1].tv_nsec; - while (CheckInterrupted(result = utimensat(AT_FDCWD, path, updatedTimes, 0))); + while (CheckInterrupted(result = utimensat(AT_FDCWD, path, updatedTimes, AT_SYMLINK_NOFOLLOW))); #else struct timeval updatedTimes[2]; updatedTimes[0].tv_sec = (long)times[0].tv_sec; @@ -41,7 +41,13 @@ int32_t SystemNative_UTimensat(const char* path, TimeSpec* times) updatedTimes[1].tv_sec = (long)times[1].tv_sec; updatedTimes[1].tv_usec = (int)times[1].tv_nsec / 1000; - while (CheckInterrupted(result = utimes(path, updatedTimes))); + while (CheckInterrupted(result = +#if HAVE_LUTIMES + lutimes +#else + utimes +#endif + (path, updatedTimes))); #endif return result; diff --git a/src/libraries/Native/Unix/configure.cmake b/src/libraries/Native/Unix/configure.cmake index 1038d1b4c1f2f..57708aff84b65 100644 --- a/src/libraries/Native/Unix/configure.cmake +++ b/src/libraries/Native/Unix/configure.cmake @@ -683,6 +683,11 @@ check_symbol_exists( sys/stat.h HAVE_UTIMENSAT) +check_symbol_exists( + lutimes + sys/time.h + HAVE_LUTIMES) + set (PREVIOUS_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS}) set (CMAKE_REQUIRED_FLAGS "-Werror -Wsign-conversion") diff --git a/src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs b/src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs index 8df736132ea3f..57a63f1c1cca2 100644 --- a/src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs +++ b/src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs @@ -24,6 +24,15 @@ public abstract class BaseGetSetTimes : FileSystemTest protected abstract T GetExistingItem(); protected abstract T GetMissingItem(); + protected abstract T CreateSymlink(string path, string pathToTarget); + + protected T CreateSymlinkToItem(T item) + { + // Creates a Symlink to 'item' (target may or may not exist) + string itemPath = GetItemPath(item); + return CreateSymlink(path: itemPath + ".link", pathToTarget: itemPath); + } + protected abstract string GetItemPath(T item); public abstract IEnumerable TimeFunctions(bool requiresRoundtripping = false); @@ -43,11 +52,8 @@ public static TimeFunction Create(SetTime setter, GetTime getter, DateTimeKind k public DateTimeKind Kind => Item3; } - [Fact] - public void SettingUpdatesProperties() + private void SettingUpdatesPropertiesCore(T item) { - T item = GetExistingItem(); - Assert.All(TimeFunctions(requiresRoundtripping: true), (function) => { // Checking that milliseconds are not dropped after setter. @@ -71,6 +77,56 @@ public void SettingUpdatesProperties() }); } + [Fact] + public void SettingUpdatesProperties() + { + T item = GetExistingItem(); + SettingUpdatesPropertiesCore(item); + } + + [Theory] + [PlatformSpecific(~TestPlatforms.Browser)] // Browser is excluded as it doesn't support symlinks + [InlineData(false)] + [InlineData(true)] + public void SettingUpdatesPropertiesOnSymlink(bool targetExists) + { + // This test is in this class since it needs all of the time functions. + // This test makes sure that the times are set on the symlink itself. + // It is needed as on OSX for example, the default for most APIs is + // to follow the symlink to completion and set the time on that entry + // instead (eg. the setattrlist will do this without the flag set). + // It is also the same case on unix, with the utimensat function. + // It is a theory since we test both the target existing and missing. + + T target = targetExists ? GetExistingItem() : GetMissingItem(); + + // When the target exists, we want to verify that its times don't change. + + T link = CreateSymlinkToItem(target); + if (!targetExists) + { + SettingUpdatesPropertiesCore(link); + } + else + { + // Get the target's initial times + IEnumerable timeFunctions = TimeFunctions(requiresRoundtripping: true); + DateTime[] initialTimes = timeFunctions.Select((funcs) => funcs.Getter(target)).ToArray(); + + SettingUpdatesPropertiesCore(link); + + // Ensure that we have the latest times. + if (target is FileSystemInfo fsi) + { + fsi.Refresh(); + } + + // Ensure the target's times haven't changed. + DateTime[] updatedTimes = timeFunctions.Select((funcs) => funcs.Getter(target)).ToArray(); + Assert.Equal(initialTimes, updatedTimes); + } + } + [Fact] [PlatformSpecific(~TestPlatforms.Browser)] // Browser is excluded as there is only 1 effective time store. public void SettingUpdatesPropertiesAfterAnother() diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/GetSetTimes.cs b/src/libraries/System.IO.FileSystem/tests/Directory/GetSetTimes.cs index 0a23d638c138a..698fbd1d67ce0 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/GetSetTimes.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/GetSetTimes.cs @@ -9,6 +9,8 @@ public class Directory_GetSetTimes : StaticGetSetTimes { protected override string GetExistingItem() => Directory.CreateDirectory(GetTestFilePath()).FullName; + protected override string CreateSymlink(string path, string pathToTarget) => Directory.CreateSymbolicLink(path, pathToTarget).FullName; + public override IEnumerable TimeFunctions(bool requiresRoundtripping = false) { if (IOInputs.SupportsGettingCreationTime && (!requiresRoundtripping || IOInputs.SupportsSettingCreationTime)) diff --git a/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/GetSetTimes.cs b/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/GetSetTimes.cs index 2dad268a79a46..795a8750e49bf 100644 --- a/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/GetSetTimes.cs +++ b/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/GetSetTimes.cs @@ -11,6 +11,8 @@ public class DirectoryInfo_GetSetTimes : InfoGetSetTimes protected override DirectoryInfo GetMissingItem() => new DirectoryInfo(GetTestFilePath()); + protected override DirectoryInfo CreateSymlink(string path, string pathToTarget) => (DirectoryInfo)Directory.CreateSymbolicLink(path, pathToTarget); + protected override string GetItemPath(DirectoryInfo item) => item.FullName; protected override void InvokeCreate(DirectoryInfo item) => item.Create(); diff --git a/src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs b/src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs index 63373576e0be1..50dcf0990a0eb 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs @@ -22,6 +22,8 @@ protected override string GetExistingItem() return path; } + protected override string CreateSymlink(string path, string pathToTarget) => File.CreateSymbolicLink(path, pathToTarget).FullName; + [Fact] [PlatformSpecific(TestPlatforms.Linux)] public void BirthTimeIsNotNewerThanLowestOfAccessModifiedTimes() diff --git a/src/libraries/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs b/src/libraries/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs index d3b9764951cbf..0559edc669e5d 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs @@ -17,6 +17,8 @@ protected override FileInfo GetExistingItem() return new FileInfo(path); } + protected override FileInfo CreateSymlink(string path, string pathToTarget) => (FileInfo)File.CreateSymbolicLink(path, pathToTarget); + private static bool HasNonZeroNanoseconds(DateTime dt) => dt.Ticks % 10 != 0; public FileInfo GetNonZeroMilliseconds() diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs index db485033649ca..23c46bae4a012 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs @@ -47,7 +47,7 @@ private unsafe Interop.Error SetCreationTimeCore(string path, long seconds, long attrList.commonAttr = Interop.libc.AttrList.ATTR_CMN_CRTIME; Interop.Error error = - Interop.libc.setattrlist(path, &attrList, &timeSpec, sizeof(Interop.Sys.TimeSpec), default(CULong)) == 0 ? + Interop.libc.setattrlist(path, &attrList, &timeSpec, sizeof(Interop.Sys.TimeSpec), new CULong(Interop.libc.FSOPT_NOFOLLOW)) == 0 ? Interop.Error.SUCCESS : Interop.Sys.GetLastErrorInfo().Error; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs index 56ca40921ae27..4e90b98a03fd7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs @@ -191,12 +191,18 @@ private static SafeFileHandle OpenHandle(string fullPath, bool asDirectory) throw new ArgumentException(SR.Arg_PathIsVolume, "path"); } + int dwFlagsAndAttributes = Interop.Kernel32.FileOperations.FILE_FLAG_OPEN_REPARSE_POINT; + if (asDirectory) + { + dwFlagsAndAttributes |= Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS; + } + SafeFileHandle handle = Interop.Kernel32.CreateFile( fullPath, Interop.Kernel32.GenericOperations.GENERIC_WRITE, FileShare.ReadWrite | FileShare.Delete, FileMode.Open, - asDirectory ? Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS : 0); + dwFlagsAndAttributes); if (handle.IsInvalid) {