From 1619428840a85ef91a356e9705b091403aa549a8 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 1 May 2024 21:41:17 +0100 Subject: [PATCH 1/7] GH-118447: Handle unreadable symlinks in `os.path.realpath()` --- Lib/test/test_posixpath.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 32a20efbb64e1d..acd3d4889f8676 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -660,6 +660,31 @@ def test_realpath_resolve_first(self): safe_rmdir(ABSTFN + "/k") safe_rmdir(ABSTFN) + @os_helper.skip_unless_symlink + @skip_if_ABSTFN_contains_backslash + @unittest.skipIf(os.chmod not in os.supports_follow_symlinks, "Can't set symlink permissions") + def test_realpath_unreadable_symlink(self): + try: + os.symlink(ABSTFN+"1", ABSTFN) + os.chmod(ABSTFN, 0o000, follow_symlinks=False) + self.assertEqual(realpath(ABSTFN), ABSTFN) + finally: + os.chmod(ABSTFN, 0o755, follow_symlinks=False) + os.unlink(ABSTFN) + + @os_helper.skip_unless_symlink + @skip_if_ABSTFN_contains_backslash + @unittest.skipIf(os.chmod not in os.supports_follow_symlinks, "Can't set symlink permissions") + def test_realpath_unreadable_symlink_strict(self): + try: + os.symlink(ABSTFN+"1", ABSTFN) + os.chmod(ABSTFN, 0o000, follow_symlinks=False) + with self.assertRaises(PermissionError): + realpath(ABSTFN) + finally: + os.chmod(ABSTFN, 0o755, follow_symlinks=False) + os.unlink(ABSTFN) + def test_relpath(self): (real_getcwd, os.getcwd) = (os.getcwd, lambda: r"/home/user/bar") try: From fc5e670bbf55546e9b262d50d2f65d213762a799 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 1 May 2024 21:45:13 +0100 Subject: [PATCH 2/7] DRY --- Lib/test/test_posixpath.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index acd3d4889f8676..3ff6560bc96e57 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -668,19 +668,8 @@ def test_realpath_unreadable_symlink(self): os.symlink(ABSTFN+"1", ABSTFN) os.chmod(ABSTFN, 0o000, follow_symlinks=False) self.assertEqual(realpath(ABSTFN), ABSTFN) - finally: - os.chmod(ABSTFN, 0o755, follow_symlinks=False) - os.unlink(ABSTFN) - - @os_helper.skip_unless_symlink - @skip_if_ABSTFN_contains_backslash - @unittest.skipIf(os.chmod not in os.supports_follow_symlinks, "Can't set symlink permissions") - def test_realpath_unreadable_symlink_strict(self): - try: - os.symlink(ABSTFN+"1", ABSTFN) - os.chmod(ABSTFN, 0o000, follow_symlinks=False) with self.assertRaises(PermissionError): - realpath(ABSTFN) + realpath(ABSTFN, strict=True) finally: os.chmod(ABSTFN, 0o755, follow_symlinks=False) os.unlink(ABSTFN) From 8d3fbffffc41fd4ee10329fccde8c4325475f638 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 1 May 2024 22:24:37 +0100 Subject: [PATCH 3/7] Fix + NEWS --- Lib/posixpath.py | 23 ++++++++----------- ...-05-01-22-24-05.gh-issue-110863.GjYBbq.rst | 2 ++ 2 files changed, 12 insertions(+), 13 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-05-01-22-24-05.gh-issue-110863.GjYBbq.rst diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 56b7915826daf4..6ecb43920dcc65 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -22,6 +22,7 @@ altsep = None devnull = '/dev/null' +import errno import os import sys import stat @@ -474,26 +475,22 @@ def realpath(filename, *, strict=False): if not stat.S_ISLNK(st.st_mode): path = newpath continue + if newpath in seen: + # Already seen this path + path = seen[newpath] + if path is not None: + # use cached value + continue + # The symlink is not resolved, so we must have a symlink loop. + raise OSError(errno.ELOOP, "Symlink loop", newpath) + target = os.readlink(newpath) except OSError: if strict: raise path = newpath continue # Resolve the symbolic link - if newpath in seen: - # Already seen this path - path = seen[newpath] - if path is not None: - # use cached value - continue - # The symlink is not resolved, so we must have a symlink loop. - if strict: - # Raise OSError(errno.ELOOP) - os.stat(newpath) - path = newpath - continue seen[newpath] = None # not resolved symlink - target = os.readlink(newpath) if target.startswith(sep): # Symlink target is absolute; reset resolved path. path = sep diff --git a/Misc/NEWS.d/next/Library/2024-05-01-22-24-05.gh-issue-110863.GjYBbq.rst b/Misc/NEWS.d/next/Library/2024-05-01-22-24-05.gh-issue-110863.GjYBbq.rst new file mode 100644 index 00000000000000..37e27a6e37c7d0 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-05-01-22-24-05.gh-issue-110863.GjYBbq.rst @@ -0,0 +1,2 @@ +:func:`os.path.realpath` now suppresses any :exc:`OSError` from +:func:`os.readlink` when *strict* mode is disabled (the default). From 90a4fe35582b406dc64b20615b1eb777370e137b Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Sat, 18 May 2024 23:07:59 +0100 Subject: [PATCH 4/7] Update Lib/test/test_posixpath.py Co-authored-by: Nice Zombies --- Lib/test/test_posixpath.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 3ff6560bc96e57..5c27b7bee8f60e 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -668,6 +668,9 @@ def test_realpath_unreadable_symlink(self): os.symlink(ABSTFN+"1", ABSTFN) os.chmod(ABSTFN, 0o000, follow_symlinks=False) self.assertEqual(realpath(ABSTFN), ABSTFN) + self.assertEqual(realpath(ABSTFN + '/foo'), ABSTFN + '/foo') + self.assertEqual(realpath(ABSTFN + '/../foo'), dirname(ABSTFN) + '/foo') + self.assertEqual(realpath(ABSTFN + '/foo/..'), ABSTFN) with self.assertRaises(PermissionError): realpath(ABSTFN, strict=True) finally: From f13e4947ffcf28209554b35a60297ff6e716074d Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 18 May 2024 23:14:01 +0100 Subject: [PATCH 5/7] Switch back to using `os.stat()` to raise ELOOP. --- Lib/posixpath.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 9945800e04957f..775728ac94fa47 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -479,7 +479,7 @@ def realpath(filename, *, strict=False): # use cached value continue # The symlink is not resolved, so we must have a symlink loop. - raise OSError(errno.ELOOP, "Symlink loop", newpath) + os.stat(newpath) # raise OSError(errno.ELOOP) target = os.readlink(newpath) except OSError: if strict: From a29e836f0e885a981e89debf5beb6f6becb7c5e2 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 18 May 2024 23:14:47 +0100 Subject: [PATCH 6/7] Unused import --- Lib/posixpath.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 775728ac94fa47..2aeb9981ce373f 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -22,7 +22,6 @@ altsep = None devnull = '/dev/null' -import errno import os import sys import stat From 0af033d5976c2ade58597d4ba392f16c17814991 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 18 May 2024 23:16:30 +0100 Subject: [PATCH 7/7] Undo unnecessary changes --- Lib/posixpath.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 2aeb9981ce373f..c04c628de55ee2 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -478,7 +478,11 @@ def realpath(filename, *, strict=False): # use cached value continue # The symlink is not resolved, so we must have a symlink loop. - os.stat(newpath) # raise OSError(errno.ELOOP) + if strict: + # Raise OSError(errno.ELOOP) + os.stat(newpath) + path = newpath + continue target = os.readlink(newpath) except OSError: if strict: