-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Directory.Move behaves differently on Unix vs. Windows #27908
Comments
@karelz @danmosemsft what is your take on this issue? |
cc @JeremyKuhne |
Seems like a bug/test hole to me. It should fail like Windows does if the destination exists when moving a file via |
Yeah, it's a bug that should be fixed. |
@marek-safar is it blocking you? Or is 3.0 fix sufficient? |
Very similar to this we already fixed |
@karelz yes, it's blocking us as it's a regression to .NET/Mono framework functionality |
@karelz Mono are absorbing more of corefx in 3.0 time frame |
Includes the following from System.IO: * FileSystemInfo * FileInfo * DirectoryInfo * Directory * FileSystem * Path `Join()` Span API and all of System.IO.Enumeration namespace. Adds about 1600 System.IO tests Behavior changes: * `File.Replace` now replaces files even if they are read-only (using the `rename` syscall on Unix) * Directory enumeration is no longer stable in the way that many Mono tests expect * Some changes in when/whether some representations of directories contain a trailing separator character * Changes in which exceptions are thrown (sometimes these are platform-dependent on CoreFX) Issues: * https://github.com/dotnet/corefx/issues/33486 * https://github.com/dotnet/corefx/issues/33490 Part of #7246
I have a fix for this and will submit a PR soon. Mentioning the area owner @JeremyKuhne. Explanation: The behavior differs between Windows and Unix because the function that does the actual move (and unexpected file overwrite in Unix) is the C++ rename function , which has the following remark message in the description:
In Windows, when we attempt to move a file and the destination exists using an Interop function, Windows throws error /// File: corefx\src\System.IO.FileSystem\src\System\IO\FileSystem.Windows.cs, line 342:
public static void MoveDirectory(string sourceFullPath, string destFullPath)
{
if (!Interop.Kernel32.MoveFile(sourceFullPath, destFullPath, overwrite: false))
{
int errorCode = Marshal.GetLastWin32Error();
...
...
throw Win32Marshal.GetExceptionForWin32Error(errorCode);
}
} But in Unix, the Interop function does not fail when the destination file already exists, and we don't do any preemptive checks for the existence of the destFullPath: /// File: corefx\src\System.IO.FileSystem\src\System\IO\FileSystem.Unix.cs
public static void MoveDirectory(string sourceFullPath, string destFullPath)
{
if (FileExists(sourceFullPath))
{
...
}
if (Interop.Sys.Rename(sourceFullPath, destFullPath) < 0)
{
...
}
} Finally, the Interop.Sys.Rename function calls SystemNative_Rename: // corefx\src\Common\src\Interop\Unix\System.Native\Interop.Rename.cs, line 20:
[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_Rename", SetLastError = true)]
internal static extern int Rename(string oldPath, string newPath); And SystemNative_Rename in turn calls the C++ rename function: // corefx\src\Native\Unix\System.Native\pal_io.c, line 1094:
int32_t SystemNative_Rename(const char* oldPath, const char* newPath)
{
int32_t result;
while ((result = rename(oldPath, newPath)) < 0 && errno == EINTR);
return result;
} |
Linux man page https://linux.die.net/man/2/rename may be more helpful than the generic C++ std library page. Comes to the same thing, though. |
When you fix this, we should add it to the list of "potentially breaking changes" in case folks run into it. |
BTW the "list" is in the platform-compat analyzer Unfortunately it has not had much attention for a while, and I am not sure how heavily used it is. If it is broadly useful, we should giev it more attention. |
Thank you @danmosemsft. There are 3 files in the location you shared. I have some questions:
This is the change I would make: exceptions.csv
As a FYI, the other two files have this structure but I suspect we don't care about them for this change: deprecated.csv
net461.csv
|
@carlossanlop the owner left. I think @joperezr may have worked on it (?) Otherwise do what you think is best - or open an issue there to do it later. |
@terrajobst Immo, it seems you've modified the platform-compat files in the past. Could you please confirm if what I stated above is correct? |
The following program throws
System.IO.IOException: Cannot create a file when that file already exists
on Windows, but succeeds (and silently replaces the destination file) on i.e. macOS:For context, expecting this to fail is part of Mono's tests.
The text was updated successfully, but these errors were encountered: