Skip to content

gh-91205: fix bug in shutil.copytree with relative links and ignore_dangling_symlinks=True #132984

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

gh-91205: fix bug in shutil.copytree with relative links and ignore_dangling_symlinks=True #132984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71de858
gh-91205: fix bug in shutil.copytree with relative links and ignore_d…
duaneg Apr 26, 2025
c0686d6
Address reviewer feedback and in particular add a couple of tests for…
duaneg Apr 26, 2025
3de355d
Add an extra test for circular and multiple ultimately dangling links
duaneg May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Lib/shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,10 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function,
os.symlink(linkto, dstname)
copystat(srcobj, dstname, follow_symlinks=not symlinks)
else:
# if the link is not to an absolute path it is relative to
# the source (see gh-91205)
if not os.path.isabs(linkto):
linkto = os.path.join(os.path.dirname(srcname), linkto)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original patch used os.path.normpath. Do we need it? Also, shouldn't we retest whether linkto is a symlink or not? if not, please also add a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use normpath: it does a string replacement which can change the meaning if symbolic links are involved, which I think would be a bug if it can happen. If it doesn't change the meaning then it shouldn't matter, since we just use the result in one place immediately after, to check whether it exists or not.

We don't need to check whether the result is a link: it may or may not be, but is correctly handled either way by the following code. os.path.exists will return false if it is a dangling link (and we will either skip it or carry on and raise an error depending on ignore_dangling_symlinks). Note that absolute symbolic links are handled the same way, so if this was necessary they would already be broken.

Having said that, good point about testing: AFAICT this case is not currently under test. I'll add a few levels of valid links-to-links to the new test case and expand test_copytree_dangling_symlinks to test dangling links-to-links.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expand test_copytree_dangling_symlinks to test dangling links-to-links.

Let's do it in a separate test function. It's easier to debug. Namely, one test for flat links and one test for multiple links. What about circular links? (are they allowed actually? namely l1 -> l2 -> l1?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it in a separate test function. It's easier to debug. Namely, one test for flat links and one test for multiple links.

Sure thing, will do.

What about circular links? (are they allowed actually? namely l1 -> l2 -> l1?)

Good question. As it happens, they are treated exactly like dangling links for the purposes of this method, since os.path.exists will return False for them. I.e. if symlinks=True they will be recreated as symbolic links (and it doesn't matter that they are circular), otherwise they will be skipped or cause an error depending if the ignore_dangling_symlinks flag is true or false, respectively.

Naturally this means they are also affected by this bug. E.g. if the symlink target is a relative path that happens to exist relative to the working directory the code will think they are valid and then fail when it tries to copy their content.

# ignore dangling symlink if the flag is on
if not os.path.exists(linkto) and ignore_dangling_symlinks:
continue
Expand Down
75 changes: 75 additions & 0 deletions Lib/test/test_shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,57 @@ def test_copytree_dangling_symlinks(self):
shutil.copytree(src_dir, dst_dir, symlinks=True)
self.assertIn('test.txt', os.listdir(dst_dir))

@os_helper.skip_unless_symlink
def test_copytree_dangling_links_to_links(self):
src_dir = self.mkdtemp()
create_file(os.path.join(src_dir, 'c'), 'abc')
os.symlink('IDONTEXIST', os.path.join(src_dir, 'foo'))
os.symlink(os.path.join(src_dir, 'foo'), os.path.join(src_dir, 'broken'))

# A dangling symlink should raise an error.
dst_dir = os.path.join(self.mkdtemp(), 'destination')
self.assertRaises(Error, shutil.copytree, src_dir, dst_dir)
self.assertEqual(['c'], os.listdir(dst_dir))

# Dangling symlinks should be ignored with the proper flag.
dst_dir = os.path.join(self.mkdtemp(), 'destination2')
shutil.copytree(src_dir, dst_dir, ignore_dangling_symlinks=True)
self.assertEqual(['c'], os.listdir(dst_dir))

# a dangling symlink is copied if symlinks=True
dst_dir = os.path.join(self.mkdtemp(), 'destination3')
shutil.copytree(src_dir, dst_dir, symlinks=True)
self.assertEqual({'broken', 'c', 'foo'}, set(os.listdir(dst_dir)))

@os_helper.skip_unless_symlink
def test_copytree_circular_symlinks(self):
src_dir = self.mkdtemp()
os.symlink('a', os.path.join(src_dir, 'b'))
os.symlink('b', os.path.join(src_dir, 'a'))
create_file(os.path.join(src_dir, 'c'), 'abc')

# A circular symlink should raise an error if symlinks=False and
# ignore_dangling_symlinks=False
dst_dir = os.path.join(self.mkdtemp(), 'destination')
with self.assertRaises(Error):
shutil.copytree(src_dir, dst_dir, symlinks=False,
ignore_dangling_symlinks=False)
self.assertEqual(['c'], os.listdir(dst_dir))

# ...however it should work if ignore_dangling_symlinks=True...
dst_dir = os.path.join(self.mkdtemp(), 'destination')
shutil.copytree(src_dir, dst_dir, symlinks=False,
ignore_dangling_symlinks=True)
self.assertEqual(['c'], os.listdir(dst_dir))

# ...and of course if symlinks=True
for ignore in (True, False):
dst_dir = os.path.join(self.mkdtemp(), 'destination')
with self.subTest(ignore_dangling_symlinks=ignore):
shutil.copytree(src_dir, dst_dir, symlinks=True,
ignore_dangling_symlinks=ignore)
self.assertEqual({'a', 'b', 'c'}, set(os.listdir(dst_dir)))

@os_helper.skip_unless_symlink
def test_copytree_symlink_dir(self):
src_dir = self.mkdtemp()
Expand All @@ -1077,6 +1128,30 @@ def test_copytree_symlink_dir(self):
self.assertTrue(os.path.islink(os.path.join(dst_dir, 'link_to_dir')))
self.assertIn('test.txt', os.listdir(os.path.join(dst_dir, 'link_to_dir')))

@os_helper.skip_unless_symlink
def test_copytree_relative_symlink(self):
# gh-91205: Ensure valid relative symlinks are copied regardless of the
# value of the ``ignore_dangling_symlinks`` flag.
src_dir = self.mkdtemp()
dir_a = os.path.join(src_dir, 'a')
dir_a_dir_b = os.path.join(dir_a, 'b')
os.mkdir(dir_a)
os.mkdir(dir_a_dir_b)
create_file(os.path.join(dir_a, 'a.txt'))
# create a symlink from src/a/b/a.txt to ../a.txt
os.symlink(os.path.join(os.pardir, 'a.txt'),
os.path.join(dir_a_dir_b, 'a.txt'))

for ignore_dangling_symlinks in (True, False):
with self.subTest(ignore_dangling_symlinks=ignore_dangling_symlinks):
dst_dir = os.path.join(self.mkdtemp(), 'x')
shutil.copytree(
dir_a_dir_b, dst_dir, symlinks=False,
ignore_dangling_symlinks=ignore_dangling_symlinks)
self.assertIn('a.txt', os.listdir(dst_dir))
self.assertFalse(
os.path.islink(os.path.join(dst_dir, 'a.txt')))

def test_copytree_return_value(self):
# copytree returns its destination path.
src_dir = self.mkdtemp()
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2025-04-26-13-44-46.gh-issue-91205.kYPo51.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Avoid skipping symbolic links to relative paths in :func:`shutil.copytree` when
``ignore_dangling_symlinks=True`` and ``symlinks=False``.
Loading