From c4ee4e756a311f03633723445299bde90eb7b79c Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Sat, 24 Aug 2024 15:11:39 +0100 Subject: GH-122890: Fix low-level error handling in `pathlib.Path.copy()` (#122897) Give unique names to our low-level FD copying functions, and try each one in turn. Handle errors appropriately for each implementation: - `fcntl.FICLONE`: suppress `EBADF`, `EOPNOTSUPP`, `ETXTBSY`, `EXDEV` - `posix._fcopyfile`: suppress `EBADF`, `ENOTSUP` - `os.copy_file_range`: suppress `ETXTBSY`, `EXDEV` - `os.sendfile`: suppress `ENOTSOCK` --- Lib/pathlib/_os.py | 58 +++++++++++++++++++++++++---------- Lib/test/test_pathlib/test_pathlib.py | 48 +++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 16 deletions(-) diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index 63dbe13..642b3a5 100644 --- a/Lib/pathlib/_os.py +++ b/Lib/pathlib/_os.py @@ -20,7 +20,7 @@ except ImportError: _winapi = None -def get_copy_blocksize(infd): +def _get_copy_blocksize(infd): """Determine blocksize for fastcopying on Linux. Hopefully the whole file will be copied in a single call. The copying itself should be performed in a loop 'till EOF is @@ -40,7 +40,7 @@ def get_copy_blocksize(infd): if fcntl and hasattr(fcntl, 'FICLONE'): - def clonefd(source_fd, target_fd): + def _ficlone(source_fd, target_fd): """ Perform a lightweight copy of two files, where the data blocks are copied only when modified. This is known as Copy on Write (CoW), @@ -48,18 +48,22 @@ if fcntl and hasattr(fcntl, 'FICLONE'): """ fcntl.ioctl(target_fd, fcntl.FICLONE, source_fd) else: - clonefd = None + _ficlone = None if posix and hasattr(posix, '_fcopyfile'): - def copyfd(source_fd, target_fd): + def _fcopyfile(source_fd, target_fd): """ Copy a regular file content using high-performance fcopyfile(3) syscall (macOS). """ posix._fcopyfile(source_fd, target_fd, posix._COPYFILE_DATA) -elif hasattr(os, 'copy_file_range'): - def copyfd(source_fd, target_fd): +else: + _fcopyfile = None + + +if hasattr(os, 'copy_file_range'): + def _copy_file_range(source_fd, target_fd): """ Copy data from one regular mmap-like fd to another by using a high-performance copy_file_range(2) syscall that gives filesystems @@ -67,7 +71,7 @@ elif hasattr(os, 'copy_file_range'): copy. This should work on Linux >= 4.5 only. """ - blocksize = get_copy_blocksize(source_fd) + blocksize = _get_copy_blocksize(source_fd) offset = 0 while True: sent = os.copy_file_range(source_fd, target_fd, blocksize, @@ -75,13 +79,17 @@ elif hasattr(os, 'copy_file_range'): if sent == 0: break # EOF offset += sent -elif hasattr(os, 'sendfile'): - def copyfd(source_fd, target_fd): +else: + _copy_file_range = None + + +if hasattr(os, 'sendfile'): + def _sendfile(source_fd, target_fd): """Copy data from one regular mmap-like fd to another by using high-performance sendfile(2) syscall. This should work on Linux >= 2.6.33 only. """ - blocksize = get_copy_blocksize(source_fd) + blocksize = _get_copy_blocksize(source_fd) offset = 0 while True: sent = os.sendfile(target_fd, source_fd, offset, blocksize) @@ -89,7 +97,7 @@ elif hasattr(os, 'sendfile'): break # EOF offset += sent else: - copyfd = None + _sendfile = None if _winapi and hasattr(_winapi, 'CopyFile2'): @@ -114,18 +122,36 @@ def copyfileobj(source_f, target_f): else: try: # Use OS copy-on-write where available. - if clonefd: + if _ficlone: try: - clonefd(source_fd, target_fd) + _ficlone(source_fd, target_fd) return except OSError as err: if err.errno not in (EBADF, EOPNOTSUPP, ETXTBSY, EXDEV): raise err # Use OS copy where available. - if copyfd: - copyfd(source_fd, target_fd) - return + if _fcopyfile: + try: + _fcopyfile(source_fd, target_fd) + return + except OSError as err: + if err.errno not in (EINVAL, ENOTSUP): + raise err + if _copy_file_range: + try: + _copy_file_range(source_fd, target_fd) + return + except OSError as err: + if err.errno not in (ETXTBSY, EXDEV): + raise err + if _sendfile: + try: + _sendfile(source_fd, target_fd) + return + except OSError as err: + if err.errno != ENOTSOCK: + raise err except OSError as err: # Produce more useful error messages. err.filename = source_f.name diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index 4c60f27..ad1720c 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -1,3 +1,4 @@ +import contextlib import io import os import sys @@ -23,9 +24,17 @@ from test.test_pathlib import test_pathlib_abc from test.test_pathlib.test_pathlib_abc import needs_posix, needs_windows, needs_symlinks try: + import fcntl +except ImportError: + fcntl = None +try: import grp, pwd except ImportError: grp = pwd = None +try: + import posix +except ImportError: + posix = None root_in_posix = False @@ -707,6 +716,45 @@ class PathTest(test_pathlib_abc.DummyPathTest, PurePathTest): if hasattr(source_st, 'st_flags'): self.assertEqual(source_st.st_flags, target_st.st_flags) + def test_copy_error_handling(self): + def make_raiser(err): + def raiser(*args, **kwargs): + raise OSError(err, os.strerror(err)) + return raiser + + base = self.cls(self.base) + source = base / 'fileA' + target = base / 'copyA' + + # Raise non-fatal OSError from all available fast copy functions. + with contextlib.ExitStack() as ctx: + if fcntl and hasattr(fcntl, 'FICLONE'): + ctx.enter_context(mock.patch('fcntl.ioctl', make_raiser(errno.EXDEV))) + if posix and hasattr(posix, '_fcopyfile'): + ctx.enter_context(mock.patch('posix._fcopyfile', make_raiser(errno.ENOTSUP))) + if hasattr(os, 'copy_file_range'): + ctx.enter_context(mock.patch('os.copy_file_range', make_raiser(errno.EXDEV))) + if hasattr(os, 'sendfile'): + ctx.enter_context(mock.patch('os.sendfile', make_raiser(errno.ENOTSOCK))) + + source.copy(target) + self.assertTrue(target.exists()) + self.assertEqual(source.read_text(), target.read_text()) + + # Raise fatal OSError from first available fast copy function. + if fcntl and hasattr(fcntl, 'FICLONE'): + patchpoint = 'fcntl.ioctl' + elif posix and hasattr(posix, '_fcopyfile'): + patchpoint = 'posix._fcopyfile' + elif hasattr(os, 'copy_file_range'): + patchpoint = 'os.copy_file_range' + elif hasattr(os, 'sendfile'): + patchpoint = 'os.sendfile' + else: + return + with mock.patch(patchpoint, make_raiser(errno.ENOENT)): + self.assertRaises(FileNotFoundError, source.copy, target) + @unittest.skipIf(sys.platform == "win32" or sys.platform == "wasi", "directories are always readable on Windows and WASI") @unittest.skipIf(root_in_posix, "test fails with root privilege") def test_copy_dir_no_read_permission(self): -- cgit v0.12