-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-142155: Fix infinite recursion in shutil.copytree on Windows junctions #142156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a23ffa3
0013914
cdebad2
debc710
cee3c73
19f55fb
d71b49b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -543,12 +543,22 @@ def _ignore_patterns(path, names): | |
| return _ignore_patterns | ||
|
|
||
| def _copytree(entries, src, dst, symlinks, ignore, copy_function, | ||
| ignore_dangling_symlinks, dirs_exist_ok=False): | ||
| ignore_dangling_symlinks, dirs_exist_ok=False, _seen=None): | ||
| if ignore is not None: | ||
| ignored_names = ignore(os.fspath(src), [x.name for x in entries]) | ||
| else: | ||
| ignored_names = () | ||
|
|
||
| # Track visited directories to detect cycles (e.g., Windows junctions) | ||
| if _seen is None: | ||
| _seen = frozenset() | ||
| src_st = os.stat(src) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call can fail and (can be) expensive. But I assume that we need it right? |
||
| src_id = (src_st.st_dev, src_st.st_ino) | ||
| if src_id in _seen: | ||
| raise Error([(src, dst, "Infinite recursion detected")]) | ||
|
|
||
| _seen = _seen | {src_id} | ||
|
|
||
| os.makedirs(dst, exist_ok=dirs_exist_ok) | ||
| errors = [] | ||
| use_srcentry = copy_function is copy2 or copy_function is copy | ||
|
|
@@ -583,12 +593,12 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function, | |
| if srcentry.is_dir(): | ||
| copytree(srcobj, dstname, symlinks, ignore, | ||
| copy_function, ignore_dangling_symlinks, | ||
| dirs_exist_ok) | ||
| dirs_exist_ok, _seen=_seen) | ||
| else: | ||
| copy_function(srcobj, dstname) | ||
| elif srcentry.is_dir(): | ||
| copytree(srcobj, dstname, symlinks, ignore, copy_function, | ||
| ignore_dangling_symlinks, dirs_exist_ok) | ||
| ignore_dangling_symlinks, dirs_exist_ok, _seen=_seen) | ||
| else: | ||
| # Will raise a SpecialFileError for unsupported file types | ||
| copy_function(srcobj, dstname) | ||
|
|
@@ -609,7 +619,7 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function, | |
| return dst | ||
|
|
||
| def copytree(src, dst, symlinks=False, ignore=None, copy_function=copy2, | ||
| ignore_dangling_symlinks=False, dirs_exist_ok=False): | ||
| ignore_dangling_symlinks=False, dirs_exist_ok=False, _seen=None): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, but I think adding a private parameter to a public function is a little odd. I see we added the I wonder if we could just change If we should trigger the audit recursively, maybe we could create another private function for the recursive logic and let
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. It is better to have a private helper for that. |
||
| """Recursively copy a directory tree and return the destination directory. | ||
|
|
||
| If exception(s) occur, an Error is raised with a list of reasons. | ||
|
|
@@ -654,7 +664,7 @@ def copytree(src, dst, symlinks=False, ignore=None, copy_function=copy2, | |
| return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks, | ||
| ignore=ignore, copy_function=copy_function, | ||
| ignore_dangling_symlinks=ignore_dangling_symlinks, | ||
| dirs_exist_ok=dirs_exist_ok) | ||
| dirs_exist_ok=dirs_exist_ok, _seen=_seen) | ||
|
|
||
| if hasattr(os.stat_result, 'st_file_attributes'): | ||
| def _rmtree_islink(st): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1099,6 +1099,42 @@ def test_copytree_subdirectory(self): | |||||
| rv = shutil.copytree(src_dir, dst_dir) | ||||||
| self.assertEqual(['pol'], os.listdir(rv)) | ||||||
|
|
||||||
| @unittest.skipUnless(sys.platform == "win32", "Windows-specific test") | ||||||
| def test_copytree_recursive_junction(self): | ||||||
| # Test that copytree raises Error for recursive junctions (Windows) | ||||||
| base_dir = self.mkdtemp() | ||||||
| self.addCleanup(shutil.rmtree, base_dir, ignore_errors=True) | ||||||
|
|
||||||
| # Create source directory structure | ||||||
| src_dir = os.path.join(base_dir, "source") | ||||||
| junction_dir = os.path.join(src_dir, "junction") | ||||||
| os.makedirs(junction_dir) | ||||||
|
|
||||||
| # Create a junction pointing to its parent, creating a cycle | ||||||
| junction_target = os.path.dirname(junction_dir) # Points to Source | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| try: | ||||||
| result = subprocess.run( | ||||||
| ["mklink", "/J", junction_dir, junction_target], | ||||||
| shell=True, check=False, capture_output=True, text=True | ||||||
| ) | ||||||
| if result.returncode != 0: | ||||||
| # Skip if we don't have permission to create junctions | ||||||
| self.skipTest(f"Failed to create junction: {result.stderr.strip()}") | ||||||
| except Exception as e: | ||||||
| # Skip if mklink is not available or fails for any reason | ||||||
| self.skipTest(f"Failed to create junction: {e}") | ||||||
|
Comment on lines
+1123
to
+1125
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's a good idea. Can't we create a junction from Python directly? (cc @barneygale).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still not addressed. |
||||||
|
|
||||||
| # Create destination directory | ||||||
| dst_dir = os.path.join(base_dir, "Dest") | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dest -> dest |
||||||
|
|
||||||
| # Test that copytree raises Error with infinite recursion message | ||||||
| with self.assertRaises(shutil.Error) as cm: | ||||||
| shutil.copytree(src_dir, dst_dir) | ||||||
|
|
||||||
| # Verify the error message contains "Infinite recursion detected" | ||||||
| self.assertIn("Infinite recursion detected", str(cm.exception)) | ||||||
|
|
||||||
ChuheLin marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| class TestCopy(BaseTest, unittest.TestCase): | ||||||
|
|
||||||
| ### shutil.copymode | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix infinite recursion crash in :func:`shutil.copytree` when a cycle involving a directory junction is present on Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why we do this. Why not just using a default argument? (it is no more mutable so it is fine)