gh-74585: Fix race condition in shutil.copyfile

[abadger]

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

  • gh-74585: Fix race condition in shutil.copyfile #136836 (comment)
• 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

• Issue: Race condition in shutil.copyfile(): source file replaced file during copy #74585

[abadger]

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]

@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.

[github-actions]

This PR is stale because it has been open for 30 days with no activity.