From 34a336570e37110c10d33a554718d1518191fc08 Mon Sep 17 00:00:00 2001 From: barneygale Date: Fri, 14 Jun 2024 20:32:56 +0100 Subject: [PATCH 1/4] GH-73991: Add *follow_symlinks* argument to `pathlib.Path.copy()` Add support for not following symlinks in `pathlib.Path.copy()`. On Windows we add the `COPY_FILE_COPY_SYMLINK` flag is following symlinks is disabled. If the source is symlink to a directory, this call will fail with `ERROR_ACCESS_DENIED`. In this case we add `COPY_FILE_DIRECTORY` to the flags and retry. --- Doc/library/pathlib.rst | 11 +++++++++- Lib/pathlib/_abc.py | 9 ++++++-- Lib/pathlib/_local.py | 10 +++++---- Lib/pathlib/_os.py | 25 +++++++++++++++++++++-- Lib/test/test_pathlib/test_pathlib_abc.py | 22 +++++++++++++++++++- Modules/_winapi.c | 5 +++++ 6 files changed, 72 insertions(+), 10 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index c8a3272d7bab4c..5bfcad0dadff6a 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1432,17 +1432,26 @@ Creating files and directories Copying, renaming and deleting ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -.. method:: Path.copy(target) +.. method:: Path.copy(target, *, follow_symlinks=True) Copy the contents of this file to the *target* file. If *target* specifies a file that already exists, it will be replaced. + If *follow_symlinks* is false, and this file is a symbolic link, *target* + will be created as a symbolic link. If *follow_symlinks* is true and this + file is a symbolic link, *target* will be a copy of the symlink target. + .. note:: This method uses operating system functionality to copy file content efficiently. The OS might also copy some metadata, such as file permissions. After the copy is complete, users may wish to call :meth:`Path.chmod` to set the permissions of the target file. + .. warning:: + On old builds of Windows (before Windows 10 build 19041), this method + raises :exc:`OSError` when a symlink to a directory is encountered and + *follow_symlinks* is false. + .. versionadded:: 3.14 diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 586145ead384ea..f1f350a196091a 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -790,14 +790,19 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): """ raise UnsupportedOperation(self._unsupported_msg('mkdir()')) - def copy(self, target): + def copy(self, target, follow_symlinks=True): """ - Copy the contents of this file to the given target. + Copy the contents of this file to the given target. If this file is a + symlink and follow_symlinks is false, a symlink will be created at the + target. """ if not isinstance(target, PathBase): target = self.with_segments(target) if self._samefile_safe(target): raise OSError(f"{self!r} and {target!r} are the same file") + if not follow_symlinks and self.is_symlink(): + target.symlink_to(self.readlink()) + return with self.open('rb') as source_f: try: with target.open('wb') as target_f: diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index cffed10dbd1207..0105ea3042422e 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -782,9 +782,11 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): raise if copyfile: - def copy(self, target): + def copy(self, target, follow_symlinks=True): """ - Copy the contents of this file to the given target. + Copy the contents of this file to the given target. If this file is a + symlink and follow_symlinks is false, a symlink will be created at the + target. """ try: target = os.fspath(target) @@ -792,9 +794,9 @@ def copy(self, target): if isinstance(target, PathBase): # Target is an instance of PathBase but not os.PathLike. # Use generic implementation from PathBase. - return PathBase.copy(self, target) + return PathBase.copy(self, target, follow_symlinks=follow_symlinks) raise - copyfile(os.fspath(self), target) + copyfile(os.fspath(self), target, follow_symlinks) def chmod(self, mode, *, follow_symlinks=True): """ diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index 1771d54e4167c1..ca4af8df15e06d 100644 --- a/Lib/pathlib/_os.py +++ b/Lib/pathlib/_os.py @@ -4,6 +4,7 @@ from errno import EBADF, EOPNOTSUPP, ETXTBSY, EXDEV import os +import stat import sys try: import fcntl @@ -92,11 +93,31 @@ def copyfd(source_fd, target_fd): if _winapi and hasattr(_winapi, 'CopyFile2'): - def copyfile(source, target): + def is_dirlink(path): + try: + st = os.lstat(path) + except (OSError, ValueError): + return False + return (st.st_file_attributes & stat.FILE_ATTRIBUTE_DIRECTORY and + st.st_reparse_tag == stat.IO_REPARSE_TAG_SYMLINK) + + def copyfile(source, target, follow_symlinks): """ Copy from one file to another using CopyFile2 (Windows only). """ - _winapi.CopyFile2(source, target, 0) + if follow_symlinks: + flags = 0 + else: + flags = _winapi.COPY_FILE_COPY_SYMLINK + try: + _winapi.CopyFile2(source, target, flags) + return + except OSError as err: + # Check for ERROR_ACCESS_DENIED + if err.winerror != 5 or not is_dirlink(source): + raise + flags |= _winapi.COPY_FILE_DIRECTORY + _winapi.CopyFile2(source, target, flags) else: copyfile = None diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index fd71284159d5c0..fee2b083712bfb 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -1712,7 +1712,7 @@ def test_copy_directory(self): source.copy(target) @needs_symlinks - def test_copy_symlink(self): + def test_copy_symlink_follow_symlinks_true(self): base = self.cls(self.base) source = base / 'linkA' target = base / 'copyA' @@ -1721,6 +1721,26 @@ def test_copy_symlink(self): self.assertFalse(target.is_symlink()) self.assertEqual(source.read_text(), target.read_text()) + @needs_symlinks + def test_copy_symlink_follow_symlinks_false(self): + base = self.cls(self.base) + source = base / 'linkA' + target = base / 'copyA' + source.copy(target, follow_symlinks=False) + self.assertTrue(target.exists()) + self.assertTrue(target.is_symlink()) + self.assertEqual(source.readlink(), target.readlink()) + + @needs_symlinks + def test_copy_directory_symlink_follow_symlinks_false(self): + base = self.cls(self.base) + source = base / 'linkB' + target = base / 'copyA' + source.copy(target, follow_symlinks=False) + self.assertTrue(target.exists()) + self.assertTrue(target.is_symlink()) + self.assertEqual(source.readlink(), target.readlink()) + def test_copy_to_existing_file(self): base = self.cls(self.base) source = base / 'fileA' diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 8794d568e92a36..c90d6c5a9ef3ef 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -3166,6 +3166,11 @@ static int winapi_exec(PyObject *m) #define COPY_FILE_REQUEST_COMPRESSED_TRAFFIC 0x10000000 #endif WINAPI_CONSTANT(F_DWORD, COPY_FILE_REQUEST_COMPRESSED_TRAFFIC); +#ifndef COPY_FILE_DIRECTORY + // Only defined in newer WinSDKs + #define COPY_FILE_DIRECTORY 0x00000080 +#endif + WINAPI_CONSTANT(F_DWORD, COPY_FILE_DIRECTORY); WINAPI_CONSTANT(F_DWORD, COPYFILE2_CALLBACK_CHUNK_STARTED); WINAPI_CONSTANT(F_DWORD, COPYFILE2_CALLBACK_CHUNK_FINISHED); From 38bf791c05ffefa5fee663ec4e910b3bc0592f16 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 19 Jun 2024 01:22:23 +0100 Subject: [PATCH 2/4] Test copying to existing symlink with follow_symlinks=False --- Lib/test/test_pathlib/test_pathlib_abc.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index fee2b083712bfb..da67883dc2e91b 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -1769,6 +1769,19 @@ def test_copy_to_existing_symlink(self): self.assertFalse(real_target.is_symlink()) self.assertEqual(source.read_text(), real_target.read_text()) + @needs_symlinks + def test_copy_to_existing_symlink_follow_symlinks_false(self): + base = self.cls(self.base) + source = base / 'dirB' / 'fileB' + target = base / 'linkA' + real_target = base / 'fileA' + source.copy(target, follow_symlinks=False) + self.assertTrue(target.exists()) + self.assertTrue(target.is_symlink()) + self.assertTrue(real_target.exists()) + self.assertFalse(real_target.is_symlink()) + self.assertEqual(source.read_text(), real_target.read_text()) + def test_copy_empty(self): base = self.cls(self.base) source = base / 'empty' From 07177929c1e19be71aeadd528f7e67be1c7569fc Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 19 Jun 2024 01:33:12 +0100 Subject: [PATCH 3/4] Check for os.stat_result.st_file_attributes --- Lib/pathlib/_os.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index ca4af8df15e06d..81d4df8b4cadd9 100644 --- a/Lib/pathlib/_os.py +++ b/Lib/pathlib/_os.py @@ -92,7 +92,7 @@ def copyfd(source_fd, target_fd): copyfd = None -if _winapi and hasattr(_winapi, 'CopyFile2'): +if _winapi and hasattr(_winapi, 'CopyFile2') and hasattr(os.stat_result, 'st_file_attributes'): def is_dirlink(path): try: st = os.lstat(path) From de5a23033531d06b5bc2dfaef658b02ed6a38ff6 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 19 Jun 2024 01:35:56 +0100 Subject: [PATCH 4/4] Add underscore prefix --- Lib/pathlib/_os.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index 81d4df8b4cadd9..bbb019b6534503 100644 --- a/Lib/pathlib/_os.py +++ b/Lib/pathlib/_os.py @@ -93,7 +93,7 @@ def copyfd(source_fd, target_fd): if _winapi and hasattr(_winapi, 'CopyFile2') and hasattr(os.stat_result, 'st_file_attributes'): - def is_dirlink(path): + def _is_dirlink(path): try: st = os.lstat(path) except (OSError, ValueError): @@ -114,7 +114,7 @@ def copyfile(source, target, follow_symlinks): return except OSError as err: # Check for ERROR_ACCESS_DENIED - if err.winerror != 5 or not is_dirlink(source): + if err.winerror != 5 or not _is_dirlink(source): raise flags |= _winapi.COPY_FILE_DIRECTORY _winapi.CopyFile2(source, target, flags)