[Bug 45935] New: Race condition in implementation of MoveFile, MoveFileEx, MoveFileWithProgress
https://bugs.winehq.org/show_bug.cgi?id=45935
Bug ID: 45935 Summary: Race condition in implementation of MoveFile, MoveFileEx, MoveFileWithProgress Product: Wine Version: 3.0.3 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: kernel32 Assignee: wine-bugs@winehq.org Reporter: joshudson@gmail.com Distribution: ---
When using MoveFile (or any of its variants) to make unique file names, there is a race condition when two threads do so in the same directory.
We have commercial software that tries to do this.
path.c, line 1373 reads
if (rename( source_unix.Buffer, dest_unix.Buffer ) == -1)
Correct code:
if (renameat2( AT_FDCWD, source_unix.Buffer, AT_FDCWD, dest_unix.Buffer, (flag & MOVEFILE_REPLACE_EXISTING) ? 0 : RENAME_NOREPLACE ) == -1)
but you'll have to do a feature test for renameat2.
https://bugs.winehq.org/show_bug.cgi?id=45935
Fabian Maurer dark.shadow4@web.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dark.shadow4@web.de
--- Comment #1 from Fabian Maurer dark.shadow4@web.de --- What exactly is the race condition here? What exactly does the program do?
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #2 from Joshua joshudson@gmail.com --- The program is generating an ordered list of unique files from multiple processes by MoveFile with "1", "2", "3", etc*. until the MoveFile goes through. The race condition exists because the implementation of MoveFile is checking if the target exists, and calling rename() if it doesn't. If the other process just did a rename to the same file in between the two system calls, one of the files in the sequence will be lost. rename() clobbers the target name if it exists.
*In fact it is timestamp_sequencenumber but the burst rate is several hundred per second.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #3 from Fabian Maurer dark.shadow4@web.de --- Makes sense. But then we have to have a fallback, which will have he exact same race condition...
On windows this works reliably?
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #4 from Joshua joshudson@gmail.com --- Yes, this works reliably on Windows. The Windows version of the resulting system call is guaranteed to not replace in kernel and there's an opflag between MOVEFILE_REPLACE_EXISTING or not in the CIFS protocol.
Yes, you'd have to have a fallback, but it'll almost never get invoked. Modern Linux, BSD, etc. have had rename2 for 10 years. I don't have a Mac to check if the fallback would be invoked there but at this point it's look pretty silly if it's still not.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #5 from Fabian Maurer dark.shadow4@web.de --- According to https://www.systutorials.com/docs/linux/man/2-renameat2/ renameat2 is Linux-specific.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #6 from Joshua joshudson@gmail.com --- Oops. My failure of reading comprehension.
My current fallback of CreateHardLink()/DeleteFile() fails spectacularly on FAT32 filesystems though. link() returns -EPERM on FAT. -EMLINK would have been nice...
Nobody's silly enough to run server-class software on a Mac; BSD on the other hand ...
https://bugs.winehq.org/show_bug.cgi?id=45935
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12@gmail.com
--- Comment #7 from Zebediah Figura z.figura12@gmail.com --- I suspect we don't want a fallback to have this kind of race condition on any operating system if we can avoid it.
Could we hold an open handle to the destination, with exclusive sharing, while performing the rename()? That would prevent any win32 APIs from writing to the file before we do, but I'm not sure if there's any reason it wouldn't work.
If that doesn't work I guess we should fall back to copy+delete.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #8 from Joshua joshudson@gmail.com --- I don't know enough about the internals of Wine to know if you could take an exclusive lock on a file name that corresponds to a non-extant file. I do know that there's no way that Copy+Delete will ever work. The race condition on that one is stupid huge, as the reader could pick up a partial or empty file. The original author didn't do the rename() thing but just kept on opening files until he got a name that didn't exist. We found that one years ago because it was really broken.
The obvious ways of taking a name lock don't work because this MoveFile call is cross-process.
This appears to be a solution:
if (0 >= (h = open(newfile, O_EXCL|O_CREAT, 0)) { close(h); ret = rename(oldfile, newfile); }
In fact it is not. The race is very much present in the reader process opening the junk file. In theory if you could take a mandatory lock on open() this would work:
if (0 >= (h = open(newfile, O_EXCL|O_CREAT|O_MANDLOCK, 04000)) { retn = rename(oldfile, newfile); close(h); }
But you can't. The following works but is a crawling horror:
if (!(mkdir(newfile)) { ret = rename(oldfile, newfile); }
Basically I shudder at this one because while it *works*, the quantum directory just seems like it's asking for something else to fall down on it.
You could try the quantum symlink to nowhere trick
if (!symlink("\001\001\001\001", newfile)) { ret = rename(oldfile, newfile); }
but that won't work on FAT, which is how I got here in the first place.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #9 from Zebediah Figura z.figura12@gmail.com --- (In reply to Joshua from comment #8)
I don't know enough about the internals of Wine to know if you could take an exclusive lock on a file name that corresponds to a non-extant file. I do know that there's no way that Copy+Delete will ever work. The race condition on that one is stupid huge, as the reader could pick up a partial or empty file. The original author didn't do the rename() thing but just kept on opening files until he got a name that didn't exist. We found that one years ago because it was really broken.
I'm not sure where you're getting that; what race condition are you expecting in the case of copy+delete? It's already the fallback for moving files across filesystems (i.e. rename() returns EXDEV).
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #10 from Joshua joshudson@gmail.com ---
It's already the fallback for moving files across filesystems (i.e. rename() returns EXDEV).
The particular caller I'm using is guaranteed to never try to span filesystems due to source and target being in the same directory. I suppose I could have said MoveFileEx(sourcefile, taragetfle, 0); which must fail on EXDEV.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #11 from Zebediah Figura z.figura12@gmail.com --- The point remains; why would copy+delete be racy?
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #12 from Joshua joshudson@gmail.com --- Because copy+delete runs the significant risk of picking up half a file. We've seen this fail already when somebody insisted on not doing the rename dance. Locking doesn't help when one of the risks is loss of power.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #13 from Zebediah Figura z.figura12@gmail.com --- (In reply to Joshua from comment #12)
Because copy+delete runs the significant risk of picking up half a file. We've seen this fail already when somebody insisted on not doing the rename dance. Locking doesn't help when one of the risks is loss of power.
I'm failing to understand what you mean by "picking up half a file". If someone's writing to the file that's being copied to while it's being copied, well, I guess it's their own fault for doing so, but I don't know how that would differ in the case of an atomic rename.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #14 from Joshua joshudson@gmail.com --- Ah. It's not writing to a half-written file. It's reading from a half-written file. The rename dance is so that the reader only ever sees whole files.
https://bugs.winehq.org/show_bug.cgi?id=45935
--- Comment #15 from Joshua joshudson@gmail.com --- I should clarify. The file format does not permit the file to know its length and I cannot change it so that it does. While they aren't text files, text files also have this property.
-
wine-bugs＠winehq.org