Remove race conditions from File.Move and Directory.Move by joshudson · Pull Request #33370 · dotnet/runtime

joshudson · 2020-03-09T13:25:11Z

(Directory.Move only applies on Linux)

joshudson · 2020-03-09T13:27:10Z

comment about performance of trying link() on FAT is clearly not correct because the syscall fast-fails so if you have to perform any checks before deciding to rename() it's slower.

… (Directory.Move only applies on Linux)

jhudsoncedaron · 2020-03-11T16:15:29Z

Azure builds die with

🍺 /usr/local/Cellar/pkg-config/0.29.2: 11 files, 626.9KB
Error: python 3.7.6_1 is already installed
To upgrade to 3.7.7, run brew upgrade python.

##[error]Bash exited with code '1'.
Finishing: Install native dependencies

Not mine!

joshudson · 2020-04-07T03:47:30Z

(NETCORE_ENGINEERING_TELEMETRY=InitializeToolset) Failed to install dotnet SDK from public location (exit code '1').

Not mine again!

joshudson · 2020-04-11T16:04:14Z

Ok somebody else needs to look at this. Remaining failed test cases:

System.IO.Tests.DirectoryInfo_MoveTo.MoveDirectory_NoOpWhenMovingDirectoryWithUpperCaseToOtherDirectoryWithLowerCase
System.IO.IOException : Cannot create '/var/folders/j1/76_p663903g5g2rg7ckqbv440000gy/T/DirectoryInfo_MoveTo_xz4to5q4.r0u/bar/foo' because a file or directory with the same name already exists.
at System.IO.FileSystem.MoveDirectory(String sourceFullPath, String destFullPath) in //src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs:line 317
at System.IO.Directory.Move(String sourceDirName, String destDirName) in //src/libraries/System.IO.FileSystem/src/System/IO/Directory.cs:line 300
at System.IO.Tests.Directory_Move.MoveDirectory_NoOpWhenMovingDirectoryWithUpperCaseToOtherDirectoryWithLowerCase() in /_/src/libraries/System.IO.FileSystem/tests/Directory/Move.cs:line 341

This one is testing for
Directory.Create("foo");
Directory.Create("bar/FOO");
Directory.Move("foo", "bar/foo");

actually succeeding and clobbering bar/FOO. I'm convinced my patch behaves correctly and the test is just wrong, because the documentation says it won't clobber the directory.

The remaining two failing tests are:

System.IO.Tests.File_Move_Tests.Unix_File_Move_To_Same_Director
Renamed event did not occur as expected\nExpected: True\nActual: False
at System.IO.Tests.FileSystemWatcherTest.ExecuteAndVerifyEvents(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Boolean assertExpected, String[] expectedPaths, Int32 timeout) in //src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs:line 339
at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String[] expectedPaths, Int32 attempts, Int32 timeout) in //src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs:line 227
at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String expectedPath, Int32 attempts, Int32 timeout) in //src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs:line 197
at System.IO.Tests.File_Move_Tests.FileMove_SameDirectory(WatcherChangeTypes eventType) in //src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs:line 142
at System.IO.Tests.File_Move_Tests.Unix_File_Move_To_Same_Directory() in /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs:line 24

System.IO.Tests.File_Move_Tests.Unix_File_Move_With_Set_NotifyFilte
Renamed event did not occur as expected\nExpected: True\nActual: False
at System.IO.Tests.FileSystemWatcherTest.ExecuteAndVerifyEvents(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Boolean assertExpected, String[] expectedPaths, Int32 timeout) in //src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs:line 339
at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String[] expectedPaths, Int32 attempts, Int32 timeout) in //src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs:line 227
at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String expectedPath, Int32 attempts, Int32 timeout) in //src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs:line 197
at System.IO.Tests.File_Move_Tests.FileMove_WithNotifyFilter(WatcherChangeTypes eventType) in //src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs:line 311
at System.IO.Tests.File_Move_Tests.Unix_File_Move_With_Set_NotifyFilter() in /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs:line 120

which check if File.Move() actually does a rename operation. But you can't do rename() and have race-free clobber avoidance here. The OS for which this is failing is Mac OSX. The only race-free solution involves link() and unlink().

adamsitnik

Hi @joshudson

First of all, I want to thank you for your work and apologize for the delay in the reviewing process.

Could you please respond to my comments and also solve the conflicts?

Thanks,
Adam

adamsitnik · 2020-12-07T10:44:49Z

+    }
+    else
+    {
+#if HAVE_RENAMEAT2 && defined(RENAME_NOREPLACE)


just for my education: is it possible to have HAVE_RENAMEAT2 but no RENAME_NOREPLACE defined?

It's feature probing. Clearly both must exist or it won't work. Officially, we don't need to know. I think it's possible, but I'm not certain.

adamsitnik · 2020-12-07T11:11:44Z

+            else if (errno == EEXIST)
+            {
+                // But renameat2 returns EEXIST on renaming a file onto itself. Check that.
+                result = RenameOnEexist(result, NULL, oldPath, newPath);


on renaming a file onto itself

Is this a breaking change? I would expect that previously we would have thrown an exception in the managed layer because FileExists would return true?

Do we have a unit test that covers this path?

No unit test is possible because we can't demand root to run unit tests. We can't get here if the name components are equal.

adamsitnik · 2020-12-07T11:17:10Z

+        // Linux renameat2() provides the functionality we want inline.
+        // This is in a feature check block as this API may well be ported to other OSes later.
+        do {
+            result = renameat2(AT_FDCWD, oldPath, AT_FDCWD, newPath, RENAME_NOREPLACE);


Thank you for using AT_FDCWD here and for taking care of the edge case! 👍

Could you please add a comment about why it's important to use it here? (I had to check the docs to find out)

Is there any chance that you could add a unit test that would guard us from removing this flag in the future?

I'm calling renameat2() for RENAME_NOREPLACE. AT_FDCWD is what you pass if you don't have anything else to pass in that slot.

On third attempt, pronoun [it] is ambiguous. Do you mean AT_FDCWD or RENAME_NOREPLACE? A unit test for missing RENAME_NOREPLACE would just check that MoveFile(,,false) doesn't ovewrite its target and I think that already exists. If AT_FDCWD isn't passed, it will fail pretty hard as handle 0 won't be a directory handle.

adamsitnik · 2020-12-07T11:38:14Z

+            result = -1; // Keeps compiler happy.
+            if (!(flags & 2))
+            {
+                result = link(oldPath, newPath);


Suggested change

result = link(oldPath, newPath);

result = link(oldPath, newPath); // link does not overwrite newPath if it exists

adamsitnik · 2020-12-07T11:43:59Z

+                result = link(oldPath, newPath);
+                if (result >= 0)
+                {
+                    while ((result = unlink(oldPath)) < 0 && errno == EINTR);


what are the side effects of using this approach (link and unlink) compared to what we had before (check + rename with TOCTOU as a side effect)? can it lead to any new problems? (please keep in mind that I am not a Unix expert)

Side effect: File system watcher looking for rename() won't ever see it.

Incidentally I had to go through our own codebase ten years ago and systematically swap File.Move() for P/Invoke of CreateHardLink because somebody deployed our application on Windows pointing at a NAS box and hit the race condition where Windows resolving a UNC to a NAS box will inherently have this race. There is one other side effect, that is if the application crashes, you can see both names at once. But it's not really a side effect on Unix, because that can happen anyway on power loss.

joshudson · 2020-12-12T16:03:27Z

Hey just to let you know I'm not ignoring this. I've got some time off upcoming and I intend to look at this during that time.

jozkee · 2021-01-04T20:24:41Z

-                // file, if it exists.  If deletion fails for a reason other than the file not existing, fail.
-                if (Interop.Sys.Unlink(destBackupFullPath) != 0)
+                // We're backing up the destination file to the backup file. If it exists, it will be removed if possible.
+                if (Interop.Sys.Rename(destFullPath, destBackupFullPath, 1) < 0)


Is the call Interop.Sys.Rename(destFullPath, destBackupFullPath, 1) identical to calling the method Interop.Sys.Rename(string oldPath, string newPath) that existed prior to this PR?

It's supposed to be.

jozkee · 2021-01-04T20:25:44Z

-                // file, if it exists.  If deletion fails for a reason other than the file not existing, fail.
-                if (Interop.Sys.Unlink(destBackupFullPath) != 0)
+                // We're backing up the destination file to the backup file. If it exists, it will be removed if possible.
+                if (Interop.Sys.Rename(destFullPath, destBackupFullPath, 1) < 0)


Is < 0 correct or should it be != 0 instead?

That's a style debate, not a functional debate. I kept the original return values of 0 for success and -1 for failure.

jozkee · 2021-01-04T20:27:46Z

+                        errno.Error == Interop.Error.EMLINK)       // too many hard links to the source file
+                   {
+                        // Destination file could not be renamed. Copy it.
+                        CopyFile(destFullPath, destBackupFullPath, true);


Why are we no longer attempting to create a link before a copy for the destBackupFullPath?

Because if you hit any of those errors, link() won't work either.

joshudson · 2021-01-05T02:43:06Z

I'm blocked on #46301 ; the tree doesn't build referencing SOS_readme.md

joshudson · 2021-01-09T19:47:00Z

I fixed minor problems from review; however I ran into problems dealing with File.Replace. Filed doc issue dotnet/docs#22288 ; the code might also be wrong in master.

danmoseley · 2021-02-01T17:48:38Z

Some test failures:

    System.IO.Tests.File_Move_Tests.Unix_File_Move_With_Set_NotifyFilter [FAIL]
      Renamed event did not occur as expected
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(337,0): at System.IO.Tests.FileSystemWatcherTest.ExecuteAndVerifyEvents(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Boolean assertExpected, String[] expectedPaths, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(225,0): at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String[] expectedPaths, Int32 attempts, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(195,0): at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String expectedPath, Int32 attempts, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs(311,0): at System.IO.Tests.File_Move_Tests.FileMove_WithNotifyFilter(WatcherChangeTypes eventType)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs(120,0): at System.IO.Tests.File_Move_Tests.Unix_File_Move_With_Set_NotifyFilter()
      Renamed event did not occur as expected
      Expected: True
      Actual:   False
    System.IO.Tests.File_Move_Tests.Unix_File_Move_In_Nested_Directory(includeSubdirectories: True) [FAIL]
      Stack Trace:
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(337,0): at System.IO.Tests.FileSystemWatcherTest.ExecuteAndVerifyEvents(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Boolean assertExpected, String[] expectedPaths, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(225,0): at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String[] expectedPaths, Int32 attempts, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(195,0): at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String expectedPath, Int32 attempts, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs(290,0): at System.IO.Tests.File_Move_Tests.FileMove_NestedDirectory(WatcherChangeTypes eventType, Boolean includeSubdirectories)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs(106,0): at System.IO.Tests.File_Move_Tests.Unix_File_Move_In_Nested_Directory(Boolean includeSubdirectories)
    System.IO.Tests.File_Move_Tests.Unix_File_Move_To_Same_Directory [FAIL]
      Renamed event did not occur as expected
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(337,0): at System.IO.Tests.FileSystemWatcherTest.ExecuteAndVerifyEvents(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Boolean assertExpected, String[] expectedPaths, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(225,0): at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String[] expectedPaths, Int32 attempts, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherTest.cs(195,0): at System.IO.Tests.FileSystemWatcherTest.ExpectEvent(FileSystemWatcher watcher, WatcherChangeTypes expectedEvents, Action action, Action cleanup, String expectedPath, Int32 attempts, Int32 timeout)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs(142,0): at System.IO.Tests.File_Move_Tests.FileMove_SameDirectory(WatcherChangeTypes eventType)
        /_/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Move.cs(24,0): at System.IO.Tests.File_Move_Tests.Unix_File_Move_To_Same_Directory()

joshudson · 2021-02-01T17:54:14Z

@danmosemsft : Yes, that's a decision that has to be made. I left these failing until we decide we're good with the filesystem watcher not seeing any rename events on File.Move(..., ..., false); until the build target has a high enough libc, or carry around the syscall definition of renameat2() so we can call it w/o libc.

danmoseley · 2021-02-08T17:30:04Z

@adamsitnik thoughts about next steps for this PR?

danmoseley · 2021-03-19T00:58:35Z

We don't seem to be making progress on this PR. Regretfully, I'm going to close it. It can always be reopened .

Dotnet-GitSync-Bot added the area-System.IO label Mar 9, 2020

joshudson commented Mar 9, 2020

View reviewed changes

jkotas changed the title ~~#31228 remove race conditions from File.Move and Directory.Move~~ Remove race conditions from File.Move and Directory.Move Mar 9, 2020

dotnet#31228 remove race conditions from File.Move and Directory.Move…

8347e48

… (Directory.Move only applies on Linux)

joshudson force-pushed the master branch from 533f898 to 8347e48 Compare March 11, 2020 03:25

joshudson added 2 commits April 6, 2020 18:31

Merge https://github.com/dotnet/runtime

f16ef0c

Use check_symbol_exists as well because android's stdio.h is broken

4441e04

joshudson force-pushed the master branch from 94ece54 to 4441e04 Compare April 7, 2020 03:12

jkotas requested review from carlossanlop and jozkee April 11, 2020 16:40

jaredpar mentioned this pull request Apr 23, 2020

Errors installing the SDK during builds #34015

Closed

carlossanlop reviewed Apr 23, 2020

View reviewed changes

Comment thread src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs Outdated

Improve doc comment for Interop.Sys.Rename

0005693

jeffhandley requested a review from adamsitnik December 7, 2020 06:58

adamsitnik reviewed Dec 7, 2020

View reviewed changes

jeffhandley assigned adamsitnik Dec 7, 2020

Merge remote-tracking branch 'upstream/master'

6441f22

jozkee reviewed Jan 4, 2021

View reviewed changes

Comment thread src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs

Fix minor issues from review

c973af8

joshudson force-pushed the master branch from 81b3ad3 to c973af8 Compare January 9, 2021 19:42

runfoapp Bot mentioned this pull request Jan 11, 2021

Testing out the new system jaredpar/runfo#71

Open

joshudson added 2 commits January 30, 2021 15:04

Revert changes to ReplaceFile() as the documentation is not reliable

335e503

Merge https://github.com/dotnet/runtime

efc301d

Base automatically changed from master to main March 1, 2021 09:06

danmoseley closed this Mar 19, 2021

ghost locked as resolved and limited conversation to collaborators Apr 18, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

	result = link(oldPath, newPath);
	result = link(oldPath, newPath); // link does not overwrite newPath if it exists

Conversation

joshudson commented Mar 9, 2020 • edited by jkotas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhudsoncedaron commented Mar 11, 2020

Uh oh!

joshudson commented Apr 7, 2020

Uh oh!

joshudson commented Apr 11, 2020

Uh oh!

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshudson Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshudson commented Dec 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshudson Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshudson commented Jan 5, 2021

Uh oh!

joshudson commented Jan 9, 2021

Uh oh!

danmoseley commented Feb 1, 2021

Uh oh!

joshudson commented Feb 1, 2021

Uh oh!

danmoseley commented Feb 8, 2021

Uh oh!

danmoseley commented Mar 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

joshudson commented Mar 9, 2020 •

edited by jkotas

Loading

joshudson Jan 9, 2021 •

edited

Loading

joshudson Jan 5, 2021 •

edited

Loading