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

SetFileInformationByHandle uses incorrect parameters on Win32 #95096

Closed
smx-smx opened this issue Nov 22, 2023 · 7 comments
Closed

SetFileInformationByHandle uses incorrect parameters on Win32 #95096

smx-smx opened this issue Nov 22, 2023 · 7 comments

Comments

@smx-smx
Copy link
Contributor

smx-smx commented Nov 22, 2023

Description

System.IO.File.SetLastWriteTime throws an exception on NFS, without setting the modification time.
NFS does however supports setting the last modification time (the touch command from Cygwin works, for example)

This breaks MSBuild, among other things, as seen here: dotnet/sdk#13808

Reproduction Steps

mount NFS

mount [remote] T:
T: is now successfully connected to [remote]

source code

Console.WriteLine("Hello, dotnet " + Environment.Version);

var t = DateTime.Now.Subtract(TimeSpan.FromDays(1));
System.IO.File.SetLastWriteTime(@"T:\thefile.txt", t);

output

Hello, dotnet 8.0.0
Unhandled exception. System.IO.IOException: The parameter is incorrect. : 'T:\thefile.txt'
   at System.IO.FileSystem.SetFileTime(SafeFileHandle fileHandle, String fullPath, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetFileTime(String fullPath, Boolean asDirectory, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetLastWriteTime(String fullPath, DateTimeOffset time, Boolean asDirectory)
   at System.IO.File.SetLastWriteTime(String path, DateTime lastWriteTime)
   at Program.<Main>$(String[] args) in G:\projects\nfs-issue\Program.cs:line 4

Expected behavior

Last modification time should be set correctly

Actual behavior

An unhandled exception is thrown

Unhandled exception. System.IO.IOException: The parameter is incorrect. : 'T:\thefile.txt'
   at System.IO.FileSystem.SetFileTime(SafeFileHandle fileHandle, String fullPath, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetFileTime(String fullPath, Boolean asDirectory, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetLastWriteTime(String fullPath, DateTimeOffset time, Boolean asDirectory)
   at System.IO.File.SetLastWriteTime(String path, DateTime lastWriteTime)
   at Program.<Main>$(String[] args) in G:\projects\nfs-issue\Program.cs:line 4

Regression?

It appears to be an old standing issue, present at least since 2019.

Known Workarounds

None

Configuration

.NET 8.0.100, on Windows 10 21H2 x64.

Other information

I found the following linked issues. It's been observed in mono too

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 22, 2023
@ghost
Copy link

ghost commented Nov 22, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

System.IO.File.SetLastWriteTime throws an exception on NFS, without setting the modification time.
NFS does however supports setting the last modification time (the touch command from Cygwin works, for example)

This breaks MSBuild, among other things, as seen here: dotnet/sdk#13808

Reproduction Steps

mount NFS

mount [remote] T:
T: is now successfully connected to [remote]

source code

Console.WriteLine("Hello, dotnet " + Environment.Version);

var t = DateTime.Now.Subtract(TimeSpan.FromDays(1));
System.IO.File.SetLastWriteTime(@"T:\thefile.txt", t);

output

Hello, dotnet 8.0.0
Unhandled exception. System.IO.IOException: The parameter is incorrect. : 'T:\thefile.txt'
   at System.IO.FileSystem.SetFileTime(SafeFileHandle fileHandle, String fullPath, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetFileTime(String fullPath, Boolean asDirectory, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetLastWriteTime(String fullPath, DateTimeOffset time, Boolean asDirectory)
   at System.IO.File.SetLastWriteTime(String path, DateTime lastWriteTime)
   at Program.<Main>$(String[] args) in G:\projects\nfs-issue\Program.cs:line 4

Expected behavior

Last modification time should be set correctly

Actual behavior

An unhandled exception is thrown

Unhandled exception. System.IO.IOException: The parameter is incorrect. : 'T:\thefile.txt'
   at System.IO.FileSystem.SetFileTime(SafeFileHandle fileHandle, String fullPath, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetFileTime(String fullPath, Boolean asDirectory, Int64 creationTime, Int64 lastAccessTime, Int64 lastWriteTime, Int64 changeTime, UInt32 fileAttributes)
   at System.IO.FileSystem.SetLastWriteTime(String fullPath, DateTimeOffset time, Boolean asDirectory)
   at System.IO.File.SetLastWriteTime(String path, DateTime lastWriteTime)
   at Program.<Main>$(String[] args) in G:\projects\nfs-issue\Program.cs:line 4

Regression?

It appears to be an old standing issue, present at least since 2019.

Known Workarounds

None

Configuration

.NET 8.0.100, on Windows 10 21H2 x64.

Other information

I found the following linked issues. It's been observed in mono too

Author: smx-smx
Assignees: -
Labels:

area-System.IO, untriaged

Milestone: -

@adamsitnik
Copy link
Member

I took a quick look at the implementation:

private static void SetFileTime(
string fullPath,
bool asDirectory,
long creationTime = -1,
long lastAccessTime = -1,
long lastWriteTime = -1,
long changeTime = -1,
uint fileAttributes = 0)
{
using SafeFileHandle handle = OpenHandleToWriteAttributes(fullPath, asDirectory);
SetFileTime(handle, fullPath, creationTime, lastAccessTime, lastWriteTime, changeTime, fileAttributes);
}
private static unsafe void SetFileTime(
SafeFileHandle fileHandle,
string? fullPath = null,
long creationTime = -1,
long lastAccessTime = -1,
long lastWriteTime = -1,
long changeTime = -1,
uint fileAttributes = 0)
{
var basicInfo = new Interop.Kernel32.FILE_BASIC_INFO
{
CreationTime = creationTime,
LastAccessTime = lastAccessTime,
LastWriteTime = lastWriteTime,
ChangeTime = changeTime,
FileAttributes = fileAttributes
};
if (!Interop.Kernel32.SetFileInformationByHandle(fileHandle, Interop.Kernel32.FileBasicInfo, &basicInfo, (uint)sizeof(Interop.Kernel32.FILE_BASIC_INFO)))

Basically all .NET does is calling SetFileInformationByHandle sys-call provided by Windows OS. The sys-call doc mentions some file systems that are not supported:

image

NFS is not listed, but my guess is that it's not supported. We should find out whether on purpose or by accident (a bug).

However a touch command from msys/cygwin on NFS works,

It would be great to find out which sys-call touch is using and consider switching to it or using it as a fallback.

@adamsitnik adamsitnik added os-windows and removed untriaged New issue has not been triaged by the area owner labels Nov 22, 2023
@adamsitnik adamsitnik added this to the Future milestone Nov 22, 2023
@smx-smx
Copy link
Contributor Author

smx-smx commented Nov 22, 2023

Here's a trace of touch on a file on NFS

#	Time of Day	Thread	Module	API	Return Value	Duration
110	6:11:58.166 PM	1	cygwin1.dll	NtCreateFile ( 0x00000007ffffc808, FILE_READ_ATTRIBUTES | FILE_READ_EA | FILE_WRITE_EA | GENERIC_WRITE | READ_CONTROL | SYNCHRONIZE, 0x00000007ffffc850, 0x00000007ffffc810, NULL, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, FILE_OPEN_IF, FILE_OPEN_FOR_BACKUP_INTENT | FILE_SYNCHRONOUS_IO_NONALERT, 0x00000007ffffc750, 116 )	STATUS_SUCCESS	0.0007311
111	6:11:58.167 PM	1	cygwin1.dll	NtSetInformationFile ( 0x0000000000000220, 0x00000007ffffc930, 0x00000007ffffc960, 40, FileBasicInformation )	STATUS_SUCCESS	0.0003514

FileInformation is the following:

0000  00 00 00 00 00 00 00 00 52 55 1e 00  ........RU..
000c  67 1d da 01 52 55 1e 00 67 1d da 01  g...RU..g...
0018  00 00 00 00 00 00 00 00 00 00 00 00  ............
0024  00 00 00 00                          ....         

And can be interpreted as:
CreationTime: 0
LastAccessTime: non-zero
LastWriteTime: non-zero
ChangeTime: 0
FileAttributes: 0

If i do the same with the .NET sample:

#	Time of Day	Thread	Module	API	Return Value	Duration
144	6:13:32.378 PM	1	KERNELBASE.dll	NtCreateFile ( 0x000000f92877e7d0, FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES | SYNCHRONIZE, 0x000000f92877e830, 0x000000f92877e7d8, NULL, 0, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, FILE_OPEN, FILE_NON_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT, NULL, 0 )	STATUS_SUCCESS	0.0008612
145	6:13:32.379 PM	1	KERNELBASE.dll	NtSetInformationFile ( 0x00000000000002a8, 0x000000f92877e940, 0x000000f92877ea70, 40, FileBasicInformation )	STATUS_INVALID_PARAMETER	0.0000060

And the information being set is:

0000  ff ff ff ff ff ff ff ff ff ff ff ff  ............
000c  ff ff ff ff a4 89 da 0d 9e 1c da 01  ............
0018  ff ff ff ff ff ff ff ff 00 00 00 00  ............
0024  f9 00 00 00                          ....         

So CreationTime is now -1, and so is LastAccessTime and ChangeTime

I could try to make a basic program doing NtSetInformationFile later, to see which of those fields triggers the problem

@smx-smx
Copy link
Contributor Author

smx-smx commented Nov 22, 2023

Ok, i broke down the issue in the following C code.
Setting LastAccessTime to -1 triggers the error. However, setting it to 0 or a valid FILETIME works correctly.

#include <stdio.h>
#include <windows.h>
#include <winnt.h>
#include <winternl.h>

int main(int argc, char *argv[]){
	HANDLE hFile;
	
	OBJECT_ATTRIBUTES attribs;
	UNICODE_STRING filePath;
	RtlInitUnicodeString(&filePath, L"\\??\\T:\\thefile.txt");
	InitializeObjectAttributes(&attribs, &filePath, OBJ_CASE_INSENSITIVE, 0, 0);

	IO_STATUS_BLOCK stb, stb2;
	NTSTATUS status = NtCreateFile(
		&hFile,
		FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES | SYNCHRONIZE,
		&attribs, &stb,
		NULL, 0,
		FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, FILE_OPEN, FILE_NON_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT,
		NULL, 0);
	if(!NT_SUCCESS(status)){
		printf("fail open\n");
		return EXIT_FAILURE;
	}

	FILETIME ft;
	GetSystemTimeAsFileTime(&ft);

	FILE_BASIC_INFO info = {
		.CreationTime = -1,
		.ChangeTime = -1,
		.LastAccessTime = 0,
		//.LastAccessTime = -1 // gives 0xC000000D (STATUS_INVALID_PARAMETER)
		//.LastAccessTime = {.HighPart = ft.dwHighDateTime, .LowPart = ft.dwLowDateTime }, // works
		.LastWriteTime = {
			.HighPart = ft.dwHighDateTime,
			.LowPart = ft.dwLowDateTime
		},
		.FileAttributes = 0xF9
	};	

	status = NtSetInformationFile(hFile, &stb2, &info, sizeof(info), FileBasicInformation);
	CloseHandle(hFile);
	if(!NT_SUCCESS(status)){
		printf("fail set (0x%08X)\n", status);
		return EXIT_FAILURE;
	}

	return EXIT_SUCCESS;
}

@smx-smx
Copy link
Contributor Author

smx-smx commented Nov 22, 2023

Is there any reason why -1 is used instead of 0?
According to MS-FSCC:

  • a value of 0 indicates to the server that it MUST NOT change this attribute.
  • a value of -1 indicates to the server that it MUST NOT change this attribute for all subsequent operations
    on the same file handle.
  • a value of -2 indicates to the server that it MUST change this attribute for all subsequent operations on the same file handle.

So i'd assume 0 would make more sense in this context (and perhaps the NFS implementation doesn't support tracking of subsequent operations on the same file handle?)

@smx-smx
Copy link
Contributor Author

smx-smx commented Nov 22, 2023

This is actually a bug in the Dotnet Windows filesystem implementation, which has implications for NTFS too.

These functions update only the specified component of the open file handle, leaving the other parameters to the default -1.
Therefore, it's possible to instruct (for example) the NTFS driver to ignore subsequent write operations by setting the last access time to anything, then executing other write operations on the open handle.

Update: it looks like this behavior has existed from the very beginning (at least it was present in December 2014): https://github.com/dotnet/corefx/blob/fda256397e23e60a15a4f0639783d99bd55069b4/src/System.IO.FileSystem/src/Interop/Interop.Windows.cs#L308

The following C# POC can be used to perform a "shadow write", i.e. opening a file and writing something to it without updating the Last modification time of the file

Console.WriteLine("Hello, dotnet " + Environment.Version);

using var fh = File.OpenHandle(@"D:\tmp\thefile.txt",
	FileMode.OpenOrCreate,
	FileAccess.ReadWrite,
	FileShare.None,
	FileOptions.None, 0);

// set last access time (and trigger the -1 LastWriteTime)
File.SetLastAccessTime(fh, DateTime.Now.Subtract(TimeSpan.FromDays(1)));

Console.WriteLine("before:" + File.GetLastWriteTime(fh));

var fs = new FileStream(fh, FileAccess.ReadWrite);
var length = fs.Length;

var wr = new StreamWriter(new FileStream(fh, FileAccess.ReadWrite));
wr.BaseStream.Seek(0, SeekOrigin.Begin);

for(int i=0; i<20; i++){
	wr.WriteLine("now: " + DateTime.Now);
	Thread.Sleep(1000);
}
Console.WriteLine("after: " + File.GetLastWriteTime(fh));

wr.Close();
fh.Close();

Output

@smx-smx smx-smx changed the title SetLastWriteTime fails on NFS SetFileInformationByHandle uses incorrect parameters on Win32 Nov 23, 2023
@adamsitnik
Copy link
Member

@smx-smx big thanks for digging so deep into the issue!

Is there any reason why -1 is used instead of 0?

I suspect that it was an oversight (the docs for the sys-call and FILE_BASIC_INFO don't mention the meaning of these values) or a performance optimization (OS has less work to do on every file write). The perf optimization would be acceptable if the API was only internal or the name would imply that. But anyway it's a bug and it needs to get fixed. Would you like to send a PR with a fix?

it looks like this behavior has existed from the very beginning

FWIW in .NET Framework we were using a different sys-call: SetFileTime

https://referencesource.microsoft.com/#mscorlib/system/io/file.cs,497
https://referencesource.microsoft.com/#mscorlib/microsoft/win32/win32native.cs,1181

@adamsitnik adamsitnik added the bug label Nov 23, 2023
@adamsitnik adamsitnik modified the milestones: Future, 9.0.0 Nov 23, 2023
smx-smx added a commit to smx-smx/runtime that referenced this issue Nov 26, 2023
According to MS-FSCC, "a value of -1 indicates to the server that it MUST NOT change this attribute for all subsequent operations"
This behavior is incorrect in the scope of .NET, since a call to File.SetLastAccessTime on a open file handle
will cause all future writes to not update the last write tim.

It also triggers STATUS_INVALID_PARAMETER on NFS, since the Windows driver doesn't seem to implement the -1 value (tracking of subsequent write operations)

Fixes dotnet#95096
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants