Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Check advisory locks in CopyFile #42458

Closed
wants to merge 7 commits into from
Closed

Check advisory locks in CopyFile #42458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
987d822
Check flock on CopyFile
filipnavara Nov 7, 2019
c6fd217
Add test case
filipnavara Nov 7, 2019
16e6eff
Fix lock checks, truncate after checking the lock, retry closes after…
filipnavara Nov 8, 2019
b40dd9a
One more EINTR fix
filipnavara Nov 8, 2019
73cb482
Bail out early if `open` fails
filipnavara Nov 8, 2019
42849d1
Add explicit unlocks
filipnavara Nov 8, 2019
ba0d450
Add one more explicit unlock
filipnavara Nov 8, 2019
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
62 changes: 51 additions & 11 deletions src/Native/Unix/System.Native/pal_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,16 +1223,36 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char

#if HAVE_CLONEFILE
// For clonefile we need to unlink the destination file first but we need to
// check permission first to ensure we don't try to unlink read-only file.
if (access(destPath, W_OK) != 0)
// check permissions first to ensure we don't try to unlink read-only file. Also,
// we need to check the advisory locks to align with the behavior of regular
// code path.
openFlags = O_WRONLY;
#if HAVE_O_CLOEXEC
openFlags |= O_CLOEXEC;
#endif
while ((outFd = open(destPath, openFlags, sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR);
if (outFd >= 0)
{
return -1;
while ((ret = flock(outFd, LOCK_EX | LOCK_NB)) < 0 && errno == EINTR);
if (ret < 0 && errno == EWOULDBLOCK)
{
while ((ret = close(outFd)) < 0 && errno == EINTR);
errno = EWOULDBLOCK;
return -1;
}

while ((ret = flock(outFd, LOCK_UN)) < 0 && errno == EINTR);
while ((ret = close(outFd)) < 0 && errno == EINTR);

while ((ret = unlink(destPath)) < 0 && errno == EINTR);
if (ret < 0)
{
return -1;
}
}

ret = unlink(destPath);
if (ret != 0)
else if (errno != ENOENT)
{
return ret;
return -1;
}
#endif
}
Expand All @@ -1252,7 +1272,7 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
(void)srcPath;
#endif

openFlags = O_WRONLY | O_TRUNC | O_CREAT | (overwrite ? 0 : O_EXCL);
openFlags = O_WRONLY | O_CREAT | (overwrite ? 0 : O_EXCL);
#if HAVE_O_CLOEXEC
openFlags |= O_CLOEXEC;
#endif
Expand All @@ -1265,6 +1285,23 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
fcntl(outFd, F_SETFD, FD_CLOEXEC);
#endif

while ((ret = flock(outFd, LOCK_EX | LOCK_NB)) < 0 && errno == EINTR);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Linux, it might be a better idea to use O_TMPFILE to create a temporary file while the copy is being performed, and when it's done, hardlink it with the new name. Then there's no need to use advisory file locking, which isn't really that useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advisory locking is used by the managed FileStream so whatever is used it has to be done on both sides and match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still like us to try hard to find ways around duplicating all this logic.

if (ret < 0 && errno == EWOULDBLOCK)
{
close(outFd);
errno = EWOULDBLOCK;
return -1;
}

while ((ret = ftruncate(outFd, 0)) < 0 && errno == EINTR);
if (ret < 0)
{
tmpErrno = errno;
while ((ret = close(outFd)) < 0 && errno == EINTR);
errno = tmpErrno;
return -1;
}

// Get the stats on the source file.
bool copied = false;

Expand All @@ -1287,7 +1324,8 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
if (errno != EINVAL && errno != ENOSYS)
{
tmpErrno = errno;
close(outFd);
while ((ret = flock(outFd, LOCK_UN)) < 0 && errno == EINTR);
while ((ret = close(outFd)) < 0 && errno == EINTR);
errno = tmpErrno;
return -1;
}
Expand Down Expand Up @@ -1315,7 +1353,8 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
if (!copied && CopyFile_ReadWrite(inFd, outFd) != 0)
{
tmpErrno = errno;
close(outFd);
while ((ret = flock(outFd, LOCK_UN)) < 0 && errno == EINTR);
while ((ret = close(outFd)) < 0 && errno == EINTR);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retrying close(2) in Linux is a race condition (please see the notes section in the man page for details).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. In the last iteration it may not be necessary.

errno = tmpErrno;
return -1;
}
Expand All @@ -1342,7 +1381,8 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
#endif

tmpErrno = errno;
close(outFd);
while ((ret = flock(outFd, LOCK_UN)) < 0 && errno == EINTR);
while ((ret = close(outFd)) < 0 && errno == EINTR);
errno = tmpErrno;
return 0;
}
Expand Down
13 changes: 13 additions & 0 deletions src/System.IO.FileSystem/tests/File/Copy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,5 +331,18 @@ public void WindowsAlternateDataStreamOverwrite(string defaultStream, string alt
Assert.Throws<IOException>(() => Copy(testFileAlternateStream, testFile2, overwrite: true));
Assert.Throws<IOException>(() => Copy(testFileAlternateStream, testFile2 + alternateStream, overwrite: true));
}

[Fact]
public void CopyOntoLockedFile()
{
string testFileSource = GetTestFilePath();
string testFileDest = GetTestFilePath();
File.Create(testFileSource).Dispose();
File.Create(testFileDest).Dispose();
using (var stream = new FileStream(testFileDest, FileMode.Open, FileAccess.Read, FileShare.None))
{
Assert.Throws<IOException>(() => Copy(testFileSource, testFileDest, overwrite: true));
}
}
}
}