Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix #52639

Merged
merged 24 commits into from
Nov 15, 2021
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
270c526
Implement most of the fix for #38824
hamarb123 May 12, 2021
50d0b47
Merge remote-tracking branch 'upstream/main' into fixfor38824
hamarb123 Oct 20, 2021
952a168
Most of the code for PR #52639 to fix #38824
hamarb123 Oct 20, 2021
abdbbcb
Remove additional FILE_FLAG_OPEN_REPARSE_POINT
hamarb123 Oct 21, 2021
7dc1875
Add missing override keywords
hamarb123 Oct 21, 2021
41d23e2
Fix access modifiers
hamarb123 Oct 21, 2021
f603c3c
Merge remote-tracking branch 'upstream/main' into fixfor38824
hamarb123 Oct 21, 2021
0a87835
Add more symlink tests, rearrange files
hamarb123 Oct 26, 2021
606130b
Merge remote-tracking branch 'upstream/main' into fixfor38824
hamarb123 Oct 26, 2021
f8c4fd6
Fix return type of CreateSymlink in File/GetSetTimes.cs
hamarb123 Oct 26, 2021
060bf1c
Remove browser from new symlink tests as it doesn't support creation …
hamarb123 Oct 26, 2021
f4bac0c
Use lutimes, improve code readability, simplify tests
hamarb123 Oct 28, 2021
97d1ed3
Change year in test to 2014 to reduce diff
hamarb123 Oct 28, 2021
fd9d2d5
Rename symlink tests, use 1 core symlink times function, and check th…
hamarb123 Oct 28, 2021
1b7b868
Inline RunSymlinkTestPart 'function'
hamarb123 Oct 28, 2021
b8460ff
Share CreateSymlinkToItem call in tests and update comment for clarity
hamarb123 Oct 28, 2021
9bf86db
Update symlink time tests
hamarb123 Oct 29, 2021
5713e27
Remove unnecessary Assert.All
hamarb123 Oct 29, 2021
d50be1b
Changes to SettingUpdatesPropertiesOnSymlink test
hamarb123 Oct 30, 2021
d61060b
Merge remote-tracking branch 'upstream/main' into fixfor38824
hamarb123 Oct 30, 2021
a894d87
Remove unnecessary fsi.Refresh()
hamarb123 Oct 31, 2021
d8bff21
Updates to test and pal_time.c
hamarb123 Nov 5, 2021
3b37666
Merge remote-tracking branch 'upstream/main' into fixfor38824
hamarb123 Nov 5, 2021
569a24f
Remove trailing space
hamarb123 Nov 5, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libraries/Common/src/Interop/OSX/Interop.libc.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
1 change: 1 addition & 0 deletions src/libraries/Native/Unix/Common/pal_config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion src/libraries/Native/Unix/System.Native/pal_time.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,15 @@ 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)));
#elif HAVE_LUTIMES
struct timeval updatedTimes[2];
updatedTimes[0].tv_sec = (long)times[0].tv_sec;
updatedTimes[0].tv_usec = (int)times[0].tv_nsec / 1000;

updatedTimes[1].tv_sec = (long)times[1].tv_sec;
updatedTimes[1].tv_usec = (int)times[1].tv_nsec / 1000;
while (CheckInterrupted(result = lutimes(path, updatedTimes)));
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
#else
struct timeval updatedTimes[2];
updatedTimes[0].tv_sec = (long)times[0].tv_sec;
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/Native/Unix/configure.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
66 changes: 62 additions & 4 deletions src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ public abstract class BaseGetSetTimes<T> : FileSystemTest
protected abstract T GetExistingItem();
protected abstract T GetMissingItem();

protected abstract T CreateSymlink(string path, string pathToTarget);

protected T CreateSymlinkToItem(T item, bool isRelative)
{
// Creates a Symlink to 'item' (may or may not exist) that is optionally
// a relative path rather than an absolute path.
string itemPath = GetItemPath(item);
string target = isRelative ? Path.GetFileName(itemPath) : itemPath;
return CreateSymlink(itemPath + ".link", target);
}

protected abstract string GetItemPath(T item);

public abstract IEnumerable<TimeFunction> TimeFunctions(bool requiresRoundtripping = false);
Expand All @@ -43,11 +54,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.
Expand All @@ -71,6 +79,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, false)]
[InlineData(false, true)]
[InlineData(true, false)]
[InlineData(true, true)]
public void SettingUpdatesPropertiesOnSymlink(bool targetIsRelative, bool targetExists)
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
{
// 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 there are variants which have a relative target
// or absolute target, and whether the target exists or not.

T target = targetExists ? GetExistingItem() : GetMissingItem();

// When the target exists, we want to verify that its times don't change.

T link = CreateSymlinkToItem(target, targetIsRelative);
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();
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved

// Ensure the target's times haven't changed.
DateTime[] updatedTimes = timeFunctions.Select((funcs) => funcs.Getter(target)).ToArray();
Assert.Equal(initialTimes, updatedTimes);
}
}

hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
[Fact]
[PlatformSpecific(~TestPlatforms.Browser)] // Browser is excluded as there is only 1 effective time store.
public void SettingUpdatesPropertiesAfterAnother()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<TimeFunction> TimeFunctions(bool requiresRoundtripping = false)
{
if (IOInputs.SupportsGettingCreationTime && (!requiresRoundtripping || IOInputs.SupportsSettingCreationTime))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ public class DirectoryInfo_GetSetTimes : InfoGetSetTimes<DirectoryInfo>

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();
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
Interop.Error.SUCCESS :
Interop.Sys.GetLastErrorInfo().Error;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,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)
{
Expand Down