Skip to content

Conversation

abadger
Copy link
Contributor

@abadger abadger commented Jul 19, 2025

This PR fixes a race condition in shutil.copyfile (there are several other shutil functions that should also be fixed. The current state is that only copyfile is fixed.) Unittests are passing but there are a few things that I'd like to look into:

Reading through the issue, the following questions were asked and should be looked into:

  • giampaolo: I wonder whether this can introduce any change in semantic though:
    >>> import os
    >>> os.stat('foobar')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    FileNotFoundError: [Errno 2] No such file or directory: 'foobar'
    >>> os.stat(333)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [Errno 9] Bad file descriptor: 333
  • vstinner: Python doesn't use FD of directories by default because it introduces issues of FD limit with deep directory tree, issue of managing FD lifetime, etc. That's why some API have one flavor for path and another for FD.
  • Look through the copyfile changes for operations that are duplicated/should be done inside of the openers. For instance, this PR presently stats a file twice. I think that's unnecessary.
  • [_] Look into whether there's other ways to pass the information about whether the file was created or not than via a closured variable.
    • We could subclass int and then return an instance of the subclass from dst_opener with a dst_was_created attribute set. I don't think this makes the code any cleaner than it is now. If there's some other reason a nonlocal variable won't work in later revisions of the code, this is a strategy that can be explored.
    • Looking into fixing the WASI error, it seems like replacing open() is needed instead of simply implementing an opener for src and dst. With that being the case, we can probably move dst_was_created into there.
  • fix other shutil functions: giampaulo: All copy* functions and move() are subjects to race conditions (the only exception is rmtree()).
  • Vstinner proposed that fixing copyfile is good in and of itself, regardless of whether we fix the rest of the shutil functions. I'll take a look at how similar they are before commenting on this.

This is a forward-port of #1659 from @pkmoore

@abadger
Copy link
Contributor Author

abadger commented Jul 20, 2025

For the WASI CI failures, it seems that WASI is setting os.name == "posix" but doesn't have fcntl. We could check whether the file is a socket using a stat call on the filename and only if it is not a socket, proceeding to the code which opened the file, which could be opened in regular blocking mode since we don't have to worry about reading a socket anymore (unless the system experiences a race in that specific scenario. I believe it would lead to the copyfile code hanging until data is sent on the socket.) We would need to stat for a second time to be sure the stat and operations for the rest of the operations were the same. I believe this could result in WASI having a potential DOS that the other platforms do not have but it would not be as large a hole as we currently have (where the race could cause files to be overwritten or private data to be read.)

@abadger abadger force-pushed the fix-race-in-shutil-74585 branch 2 times, most recently from d361a00 to 970fce1 Compare July 20, 2025 15:39
@abadger abadger force-pushed the fix-race-in-shutil-74585 branch from 970fce1 to 8ef96f8 Compare September 22, 2025 09:43
@abadger abadger requested a review from giampaolo as a code owner September 22, 2025 09:43
@abadger
Copy link
Contributor Author

abadger commented Sep 22, 2025

@giampaolo I believe your question about os.stat('nonexistent') raising a different exception than os.stat(33333) (bad file descriptor) won't cause problems. The API for copyfile takes a filename. In the new code, we need to open the file to get the filedescriptor before we can use stat with it. The open should raise a FileNotFoundError. Let me know if this doesn't addres the problem you are thinking of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant