bpo-30400: Fix race condtion in shutil.copyfile

Author: Preston Moore

Lib/shutil.py

@@ -5,6 +5,7 @@
 """
 
 import os
+import fcntl
 import sys
 import stat
 import fnmatch
@@ -280,6 +281,11 @@ def _stat(fn):
 def _islink(fn):
     return fn.is_symlink() if isinstance(fn, os.DirEntry) else os.path.islink(fn)
 
+def _samefilef(fsrc, fdst):
+    if os.path.abspath(fsrc.name) == os.path.abspath(fdst.name):
+        return True
+    return False
+
 def copyfile(src, dst, *, follow_symlinks=True):
     """Copy data from src to dst in the most efficient way possible.
 
@@ -307,48 +313,121 @@ def copyfile(src, dst, *, follow_symlinks=True):
             if _WINDOWS and i == 0:
                 file_size = st.st_size
 
+    # XXX Is there a race issue here with os.path.islink() operating on a
+    # path rather than a file descriptor
     if not follow_symlinks and _islink(src):
         os.symlink(os.readlink(src), dst)
+        return dst
+
+    # In Unix, we must open the files non-blocking initially as opening a
+    # FIFO without data present in it will block.
+    if os.name == 'posix':
+        src_flags = os.O_RDONLY | os.O_NONBLOCK
+        ### Do we need os.O_EXCL?
+        dst_flags = os.O_CREAT | os.O_WRONLY | os.O_NONBLOCK
     else:
-        with open(src, 'rb') as fsrc:
+        src_flags = os.O_RDONLY
+        dst_flags = os.O_WRONLY
+
+    def _src_opener(path, flags):
+        return os.open(path, flags)
+
+    # dst's opener is more complex.  We need to atomically check and open the
+    # destination if it doesn't exist.  If it does exist, we need to detect so
+    # we don't destroy it later should we have to fail out for some reason.
+    dst_was_created = False
+    def _dst_opener(path, flags):
+        nonlocal dst_was_created
+        try:
+            dst_was_created = True
+            return os.open(path, flags | dst_flags | os.O_EXCL)
+        except FileExistsError:
+            dst_was_created = False
             try:
-                with open(dst, 'wb') as fdst:
-                    # macOS
-                    if _HAS_FCOPYFILE:
+                # Open the already existing file in as non-blocking
+                ### We have O_CREATE and O_NONBLOCK in dst_flags.  Is that intended?
+                fd = os.open(path, flags | dst_flags)
+            # If the destination is already a FIFO for some reason, we'll get an
+            # OSError here as we've opened a FIFO for a non-blocking write with
+            # no reader on the other end.
+            except OSError as e:
+                # Use errno to decide whether the OSError we caught is due to
+                # the above reason or some other reason.  For the above reason,
+                # raise the appropriate special file error.  Otherwise, we need
+                # to re-raise the OSError
+                if e.errno == errno.ENXIO:
+                    raise SpecialFileError("`%s` is a named pipe" % path)
+                else:
+                    raise e
+            # This check is required to catch the case where the destination
+            # file is a FIFO that's we successfully opened non-blocking because
+            # for some reason there was a reader listening preventing an ENXIO
+            # situation.
+            st = os.fstat(fd)
+            if stat.S_ISFIFO(st.st_mode):
+                raise SpecialFileError("`%s` is a named pipe" % path)
+            return fd
+
+    with open(src, 'rb', opener=_src_opener) as fsrc:
+        try:
+            with open(dst, 'wb', opener=_dst_opener) as fdst:
+                file_size = 0
+                # file.name is not populated when using os.fdopen()
+                st = os.fstat(fsrc.fileno())
+                if stat.S_ISFIFO(st.st_mode):
+                    # Don't unlink dst unless we created it above
+                    if dst_was_created:
+                        os.unlink(dst)
+                    raise SpecialFileError("`%s` is a named pipe" % src)
+                if _WINDOWS and i == 0:
+                    file_size = st.st_size
+
+                # In Unix, we must set the file descriptors back to blocking as that's
+                # what the rest of this function expects.
+                # Additonally, fsrc, and fdst must be turned into file objects
+                if os.name == 'posix':
+                    srcfl = fcntl.fcntl(fsrc.fileno(), fcntl.F_GETFL)
+                    dstfl = fcntl.fcntl(fdst.fileno(), fcntl.F_GETFL)
+
+                    # Unset the non-blocking flag
+                    fcntl.fcntl(fsrc.fileno(), fcntl.F_SETFL, srcfl & ~os.O_NONBLOCK)
+                    fcntl.fcntl(fdst.fileno(), fcntl.F_SETFL, dstfl & ~os.O_NONBLOCK)
+
+                # macOS
+                if _HAS_FCOPYFILE:
+                    try:
+                        _fastcopy_fcopyfile(fsrc, fdst, posix._COPYFILE_DATA)
+                        return dst
+                    except _GiveupOnFastCopy:
+                        pass
+                # Linux / Android / Solaris
+                elif _USE_CP_SENDFILE or _USE_CP_COPY_FILE_RANGE:
+                    # reflink may be implicit in copy_file_range.
+                    if _USE_CP_COPY_FILE_RANGE:
                         try:
-                            _fastcopy_fcopyfile(fsrc, fdst, posix._COPYFILE_DATA)
+                            _fastcopy_copy_file_range(fsrc, fdst)
+                            return dst
+                        except _GiveupOnFastCopy:
+                            pass
+                    if _USE_CP_SENDFILE:
+                        try:
+                            _fastcopy_sendfile(fsrc, fdst)
                             return dst
                         except _GiveupOnFastCopy:
                             pass
-                    # Linux / Android / Solaris
-                    elif _USE_CP_SENDFILE or _USE_CP_COPY_FILE_RANGE:
-                        # reflink may be implicit in copy_file_range.
-                        if _USE_CP_COPY_FILE_RANGE:
-                            try:
-                                _fastcopy_copy_file_range(fsrc, fdst)
-                                return dst
-                            except _GiveupOnFastCopy:
-                                pass
-                        if _USE_CP_SENDFILE:
-                            try:
-                                _fastcopy_sendfile(fsrc, fdst)
-                                return dst
-                            except _GiveupOnFastCopy:
-                                pass
-                    # Windows, see:
-                    # https://github.com/python/cpython/pull/7160#discussion_r195405230
-                    elif _WINDOWS and file_size > 0:
-                        _copyfileobj_readinto(fsrc, fdst, min(file_size, COPY_BUFSIZE))
-                        return dst
-
-                    copyfileobj(fsrc, fdst)
 
-            # Issue 43219, raise a less confusing exception
-            except IsADirectoryError as e:
-                if not os.path.exists(dst):
-                    raise FileNotFoundError(f'Directory does not exist: {dst}') from e
-                else:
-                    raise
+                # Windows, see:
+                # https://github.com/python/cpython/pull/7160#discussion_r195405230
+                elif _WINDOWS and file_size > 0:
+                    _copyfileobj_readinto(fsrc, fdst, min(file_size, COPY_BUFSIZE))
+                    return dst
+                copyfileobj(fsrc, fdst)
+
+        # Issue 43219, raise a less confusing exception
+        except IsADirectoryError as e:
+            if not os.path.exists(dst):
+                raise FileNotFoundError(f'Directory does not exist: {dst}') from e
+            raise
 
     return dst
 

Lib/test/test_shutil.py

@@ -2306,6 +2306,18 @@ def check_chown(path, uid=None, gid=None):
         with self.assertRaises(ValueError):
             shutil.chown(filename, dir_fd=dirfd)
 
+    def test_copyfile_same_file(self):
+        # copyfile() should raise SameFileError if the source and destination
+        # are the same.
+        src_dir = self.mkdtemp()
+        src_file = os.path.join(src_dir, 'foo')
+        create_file(src_file, b'foo')
+        self.assertRaises(SameFileError, shutil.copyfile, src_file, src_file)
+        # But Error should work too, to stay backward compatible.
+        self.assertRaises(Error, shutil.copyfile, src_file, src_file)
+        # Make sure file is not corrupted.
+        self.assertEqual(read_file(src_file), 'foo')
+
 
 @support.requires_subprocess()
 class TestWhich(BaseTest, unittest.TestCase):
@@ -2997,7 +3009,7 @@ def __exit__(self, exc_type, exc_val, exc_tb):
             return self._suppress_at_exit
 
     def test_w_source_open_fails(self):
-        def _open(filename, mode='r'):
+        def _open(filename, mode='r', opener=None):
             if filename == 'srcfile':
                 raise OSError('Cannot open "srcfile"')
             assert 0  # shouldn't reach here.
@@ -3010,7 +3022,7 @@ def _open(filename, mode='r'):
     def test_w_dest_open_fails(self):
         srcfile = self.Faux()
 
-        def _open(filename, mode='r'):
+        def _open(filename, mode='r', opener=None):
             if filename == 'srcfile':
                 return srcfile
             if filename == 'destfile':
@@ -3029,7 +3041,7 @@ def test_w_dest_close_fails(self):
         srcfile = self.Faux()
         destfile = self.Faux(True)
 
-        def _open(filename, mode='r'):
+        def _open(filename, mode='r', opener=None):
             if filename == 'srcfile':
                 return srcfile
             if filename == 'destfile':
@@ -3051,7 +3063,7 @@ def test_w_source_close_fails(self):
         srcfile = self.Faux(True)
         destfile = self.Faux()
 
-        def _open(filename, mode='r'):
+        def _open(filename, mode='r', opener=None):
             if filename == 'srcfile':
                 return srcfile
             if filename == 'destfile':

Misc/NEWS.d/next/Library/2018-07-07.bpo-30400.Pjhd8.rst

@@ -0,0 +1,2 @@
+Fix race condition in shutil.copyfile() by ensuring the inode of the source
+file does not change while it is being copied.  Patch by Preston Moore.