Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix (#52639)
authorHamish Arblaster <hamarb123@gmail.com>
Mon, 15 Nov 2021 19:45:31 +0000 (06:45 +1100)
committerGitHub <noreply@github.com>
Mon, 15 Nov 2021 19:45:31 +0000 (11:45 -0800)
* 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

src/libraries/Common/src/Interop/OSX/Interop.libc.cs
src/libraries/Native/Unix/Common/pal_config.h.in
src/libraries/Native/Unix/System.Native/pal_time.c
src/libraries/Native/Unix/configure.cmake
src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
src/libraries/System.IO.FileSystem/tests/Directory/GetSetTimes.cs
src/libraries/System.IO.FileSystem/tests/DirectoryInfo/GetSetTimes.cs
src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs
src/libraries/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

index d0b3779..89ba513 100644 (file)
@@ -25,5 +25,7 @@ internal static partial class Interop
 
         [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;
     }
 }
index 9cffccb..00b774b 100644 (file)
 #cmakedefine01 HAVE_GETDOMAINNAME
 #cmakedefine01 HAVE_UNAME
 #cmakedefine01 HAVE_UTIMENSAT
+#cmakedefine01 HAVE_LUTIMES
 #cmakedefine01 HAVE_FUTIMES
 #cmakedefine01 HAVE_FUTIMENS
 #cmakedefine01 HAVE_MKSTEMPS
index 56e2313..4f9b532 100644 (file)
@@ -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;
index 1038d1b..57708af 100644 (file)
@@ -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")
 
index 8df7361..57a63f1 100644 (file)
@@ -24,6 +24,15 @@ namespace System.IO.Tests
         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<TimeFunction> TimeFunctions(bool requiresRoundtripping = false);
@@ -43,11 +52,8 @@ namespace System.IO.Tests
             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.
@@ -72,6 +78,56 @@ namespace System.IO.Tests
         }
 
         [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<TimeFunction> 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()
         {
index 0a23d63..698fbd1 100644 (file)
@@ -9,6 +9,8 @@ namespace System.IO.Tests
     {
         protected override string GetExistingItem() => Directory.CreateDirectory(GetTestFilePath()).FullName;
 
+        protected override string CreateSymlink(string path, string pathToTarget) => Directory.CreateSymbolicLink(path, pathToTarget).FullName;
+
         public override IEnumerable<TimeFunction> TimeFunctions(bool requiresRoundtripping = false)
         {
             if (IOInputs.SupportsGettingCreationTime && (!requiresRoundtripping || IOInputs.SupportsSettingCreationTime))
index 2dad268..795a875 100644 (file)
@@ -11,6 +11,8 @@ namespace System.IO.Tests
 
         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();
index 6337357..50dcf09 100644 (file)
@@ -22,6 +22,8 @@ namespace System.IO.Tests
             return path;
         }
 
+        protected override string CreateSymlink(string path, string pathToTarget) => File.CreateSymbolicLink(path, pathToTarget).FullName;
+
         [Fact]
         [PlatformSpecific(TestPlatforms.Linux)]
         public void BirthTimeIsNotNewerThanLowestOfAccessModifiedTimes()
index d3b9764..0559edc 100644 (file)
@@ -17,6 +17,8 @@ namespace System.IO.Tests
             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()
index db48503..23c46ba 100644 (file)
@@ -47,7 +47,7 @@ namespace System.IO
             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;
 
index 56ca409..4e90b98 100644 (file)
@@ -191,12 +191,18 @@ namespace System.IO
                 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)
             {