From 164dbfd3f3a484eb32eabb60d893ae19714043ce Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 7 Apr 2021 23:59:28 +0100 Subject: [PATCH 01/23] bpo-43757: make pathlib use os.path.realpath() to resolve all symlinks in a path Removes a pathlib-specific implementation of `realpath()` that's mostly copied from `ntpath` and `posixpath`. --- Lib/pathlib.py | 95 ++++++-------------------------------------------- 1 file changed, 10 insertions(+), 85 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 37934c6038e1d1..d2a15c64602d3c 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -186,30 +186,6 @@ def casefold_parts(self, parts): def compile_pattern(self, pattern): return re.compile(fnmatch.translate(pattern), re.IGNORECASE).fullmatch - def resolve(self, path, strict=False): - s = str(path) - if not s: - return path._accessor.getcwd() - previous_s = None - if _getfinalpathname is not None: - if strict: - return self._ext_to_normal(_getfinalpathname(s)) - else: - tail_parts = [] # End of the path after the first one not found - while True: - try: - s = self._ext_to_normal(_getfinalpathname(s)) - except FileNotFoundError: - previous_s = s - s, tail = os.path.split(s) - tail_parts.append(tail) - if previous_s == s: - return path - else: - return os.path.join(s, *reversed(tail_parts)) - # Means fallback on absolute - return None - def _split_extended_path(self, s, ext_prefix=ext_namespace_prefix): prefix = '' if s.startswith(ext_prefix): @@ -220,10 +196,6 @@ def _split_extended_path(self, s, ext_prefix=ext_namespace_prefix): s = '\\' + s[3:] return prefix, s - def _ext_to_normal(self, s): - # Turn back an extended path into a normal DOS-like path - return self._split_extended_path(s)[1] - def is_reserved(self, parts): # NOTE: the rules for reserved names seem somewhat complicated # (e.g. r"..\NUL" is reserved but not r"foo\NUL"). @@ -281,54 +253,6 @@ def casefold_parts(self, parts): def compile_pattern(self, pattern): return re.compile(fnmatch.translate(pattern)).fullmatch - def resolve(self, path, strict=False): - sep = self.sep - accessor = path._accessor - seen = {} - def _resolve(path, rest): - if rest.startswith(sep): - path = '' - - for name in rest.split(sep): - if not name or name == '.': - # current dir - continue - if name == '..': - # parent dir - path, _, _ = path.rpartition(sep) - continue - if path.endswith(sep): - newpath = path + name - else: - newpath = path + sep + name - 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 RuntimeError("Symlink loop from %r" % newpath) - # Resolve the symbolic link - try: - target = accessor.readlink(newpath) - except OSError as e: - if e.errno != EINVAL and strict: - raise - # Not a symlink, or non-strict mode. We just leave the path - # untouched. - path = newpath - else: - seen[newpath] = None # not resolved symlink - path = _resolve(path, target) - seen[newpath] = path # resolved symlink - - return path - # NOTE: according to POSIX, getcwd() cannot contain path components - # which are symlinks. - base = '' if path.is_absolute() else accessor.getcwd() - return _resolve(base, str(path)) or sep - def is_reserved(self, parts): return False @@ -424,6 +348,8 @@ def group(self, path): expanduser = staticmethod(os.path.expanduser) + realpath = staticmethod(os.path.realpath) + _normal_accessor = _NormalAccessor() @@ -1132,15 +1058,14 @@ def resolve(self, strict=False): normalizing it (for example turning slashes into backslashes under Windows). """ - s = self._flavour.resolve(self, strict=strict) - if s is None: - # No symlink resolution => for consistency, raise an error if - # the path doesn't exist or is forbidden - self.stat() - s = str(self.absolute()) - # Now we have no symlinks in the path, it's safe to normalize it. - normed = self._flavour.pathmod.normpath(s) - return self._from_parts((normed,)) + p = self._from_parts((self._accessor.realpath(self),)) + try: + if S_ISLNK(self.lstat().st_mode): + raise RuntimeError("Symlink loop from %r" % str(p)) + except OSError: + if strict: + raise + return p def stat(self, *, follow_symlinks=True): """ From 2894e0a21a84e00b2959ca42f0e109a61fdfad2d Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 02:24:55 +0100 Subject: [PATCH 02/23] Adjust `os.path.realpath` to match `pathlib.Path.resolve()` behaviour when a new `strict=True` keyword-only argument is passed. --- Doc/library/os.path.rst | 15 +++++++++++---- Lib/ntpath.py | 4 +++- Lib/pathlib.py | 13 ++++++------- Lib/posixpath.py | 31 ++++++++++++++++++------------- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/Doc/library/os.path.rst b/Doc/library/os.path.rst index 4cab3113c9e4e5..941af5ca648ae0 100644 --- a/Doc/library/os.path.rst +++ b/Doc/library/os.path.rst @@ -344,15 +344,15 @@ the :mod:`glob` module.) Accepts a :term:`path-like object`. -.. function:: realpath(path) +.. function:: realpath(path, *, strict=False) Return the canonical path of the specified filename, eliminating any symbolic links encountered in the path (if they are supported by the operating system). - .. note:: - When symbolic link cycles occur, the returned path will be one member of - the cycle, but no guarantee is made about which member that will be. + In non-strict mode (the default), missing or inaccessible ancestors are + permitted; when encountered, the remainder of the path joined on and + returned. In strict mode an :exc:`OSError` is raised in this scenario. .. versionchanged:: 3.6 Accepts a :term:`path-like object`. @@ -360,6 +360,13 @@ the :mod:`glob` module.) .. versionchanged:: 3.8 Symbolic links and junctions are now resolved on Windows. + .. versionchanged:: 3.10 + The *strict* parameter was added. + + .. versionchanged:: 3.10 + Raises :exc:`OSError` with :const:`~errno.ELOOP` when a symbolic link + cycle occurs. Previously returned one member of the cycle. + .. function:: relpath(path, start=os.curdir) diff --git a/Lib/ntpath.py b/Lib/ntpath.py index 5ae8079074cd91..527c7ae1938fbb 100644 --- a/Lib/ntpath.py +++ b/Lib/ntpath.py @@ -635,7 +635,7 @@ def _getfinalpathname_nonstrict(path): tail = join(name, tail) if tail else name return tail - def realpath(path): + def realpath(path, *, strict=False): path = normpath(path) if isinstance(path, bytes): prefix = b'\\\\?\\' @@ -660,6 +660,8 @@ def realpath(path): path = _getfinalpathname(path) initial_winerror = 0 except OSError as ex: + if strict: + raise initial_winerror = ex.winerror path = _getfinalpathname_nonstrict(path) # The path returned by _getfinalpathname will always start with \\?\ - diff --git a/Lib/pathlib.py b/Lib/pathlib.py index d2a15c64602d3c..6cfde1f2b394b1 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -1058,14 +1058,13 @@ def resolve(self, strict=False): normalizing it (for example turning slashes into backslashes under Windows). """ - p = self._from_parts((self._accessor.realpath(self),)) try: - if S_ISLNK(self.lstat().st_mode): - raise RuntimeError("Symlink loop from %r" % str(p)) - except OSError: - if strict: - raise - return p + p = self._accessor.realpath(self, strict=strict) + except OSError as ex: + if ex.errno == ELOOP: + raise RuntimeError("Symlink loop from %r", ex.filename) + raise + return self._from_parts((p,)) def stat(self, *, follow_symlinks=True): """ diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 62afbd0ccf0f0f..6b8dd4ee134ebe 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 @@ -387,16 +388,16 @@ def abspath(path): # Return a canonical path (i.e. the absolute location of a file on the # filesystem). -def realpath(filename): +def realpath(filename, *, strict=False): """Return the canonical path of the specified filename, eliminating any symbolic links encountered in the path.""" filename = os.fspath(filename) - path, ok = _joinrealpath(filename[:0], filename, {}) + path = _joinrealpath(filename[:0], filename, strict, {}) return abspath(path) # Join two paths, normalizing and eliminating any symbolic links # encountered in the second path. -def _joinrealpath(path, rest, seen): +def _joinrealpath(path, rest, strict, seen): if isinstance(path, bytes): sep = b'/' curdir = b'.' @@ -425,9 +426,17 @@ def _joinrealpath(path, rest, seen): path = pardir continue newpath = join(path, name) - if not islink(newpath): - path = newpath - continue + try: + st = os.lstat(newpath) + except OSError: + if strict: + raise + is_link = False + else: + is_link = stat.S_ISLNK(st.st_mode) + if not is_link: + path = newpath + continue # Resolve the symbolic link if newpath in seen: # Already seen this path @@ -436,15 +445,11 @@ def _joinrealpath(path, rest, seen): # use cached value continue # The symlink is not resolved, so we must have a symlink loop. - # Return already resolved part + rest of the path unchanged. - return join(newpath, rest), False - seen[newpath] = None # not resolved symlink - path, ok = _joinrealpath(path, os.readlink(newpath), seen) - if not ok: - return join(path, rest), False + raise OSError(errno.ELOOP, os.strerror(errno.ELOOP), newpath) + path = _joinrealpath(path, os.readlink(newpath), strict, seen) seen[newpath] = path # resolved symlink - return path, True + return path supports_unicode_filenames = (sys.platform == 'darwin') From 495bd8b483a91d6f2a0969de179b7d5ec6770ac3 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 02:46:10 +0100 Subject: [PATCH 03/23] Call `os.stat()` to raise `OSError(errno.ELOOP, ...)`. --- Lib/posixpath.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 6b8dd4ee134ebe..9f2a09233bb461 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -445,7 +445,8 @@ def _joinrealpath(path, rest, strict, seen): # use cached value continue # The symlink is not resolved, so we must have a symlink loop. - raise OSError(errno.ELOOP, os.strerror(errno.ELOOP), newpath) + # Raise OSError(errno.ELOOP) + os.stat(newpath) path = _joinrealpath(path, os.readlink(newpath), strict, seen) seen[newpath] = path # resolved symlink From 492c66ba004be9814e61fc3f180e5bb6d8f0f018 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 02:46:36 +0100 Subject: [PATCH 04/23] Remove unused import --- Lib/posixpath.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 9f2a09233bb461..82a89fd33de7ef 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 c8bac15402acdc9fbd6034b37d11642c86840bdf Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 03:54:01 +0100 Subject: [PATCH 05/23] Stop ignoring ERROR_CANT_RESOLVE_FILENAME error in _getfinalpathname_nonstrict() --- Lib/ntpath.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/ntpath.py b/Lib/ntpath.py index 527c7ae1938fbb..2498c3116c7130 100644 --- a/Lib/ntpath.py +++ b/Lib/ntpath.py @@ -603,8 +603,7 @@ def _getfinalpathname_nonstrict(path): # 87: ERROR_INVALID_PARAMETER # 123: ERROR_INVALID_NAME # 1920: ERROR_CANT_ACCESS_FILE - # 1921: ERROR_CANT_RESOLVE_FILENAME (implies unfollowable symlink) - allowed_winerror = 1, 2, 3, 5, 21, 32, 50, 67, 87, 123, 1920, 1921 + allowed_winerror = 1, 2, 3, 5, 21, 32, 50, 67, 87, 123, 1920 # Non-strict algorithm is to find as much of the target directory # as we can and join the rest. From fe69688bceb22644195a69394306dc93ff39576a Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 04:31:15 +0100 Subject: [PATCH 06/23] Fix symlink detection and `posixpath` tests. --- Lib/posixpath.py | 1 + Lib/test/test_posixpath.py | 27 ++++++++++++--------------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 82a89fd33de7ef..46b0060177fb57 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -446,6 +446,7 @@ def _joinrealpath(path, rest, strict, seen): # The symlink is not resolved, so we must have a symlink loop. # Raise OSError(errno.ELOOP) os.stat(newpath) + seen[newpath] = None # not resolved symlink path = _joinrealpath(path, os.readlink(newpath), strict, seen) seen[newpath] = path # resolved symlink diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index e18d01f4635a3a..8ab27f93b33471 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -369,36 +369,33 @@ def test_realpath_relative(self): "Missing symlink implementation") @skip_if_ABSTFN_contains_backslash def test_realpath_symlink_loops(self): - # Bug #930024, return the path unchanged if we get into an infinite - # symlink loop. + # Bug #43757, raise OSError if we get into an infinite symlink loop. try: os.symlink(ABSTFN, ABSTFN) - self.assertEqual(realpath(ABSTFN), ABSTFN) + self.assertRaises(OSError, realpath, ABSTFN) os.symlink(ABSTFN+"1", ABSTFN+"2") os.symlink(ABSTFN+"2", ABSTFN+"1") - self.assertEqual(realpath(ABSTFN+"1"), ABSTFN+"1") - self.assertEqual(realpath(ABSTFN+"2"), ABSTFN+"2") + self.assertRaises(OSError, realpath, ABSTFN+"1") + self.assertRaises(OSError, realpath, ABSTFN+"2") - self.assertEqual(realpath(ABSTFN+"1/x"), ABSTFN+"1/x") - self.assertEqual(realpath(ABSTFN+"1/.."), dirname(ABSTFN)) - self.assertEqual(realpath(ABSTFN+"1/../x"), dirname(ABSTFN) + "/x") + self.assertRaises(OSError, realpath, ABSTFN+"1/x") + self.assertRaises(OSError, realpath, ABSTFN+"1/..") + self.assertRaises(OSError, realpath, ABSTFN+"1/../x") os.symlink(ABSTFN+"x", ABSTFN+"y") - self.assertEqual(realpath(ABSTFN+"1/../" + basename(ABSTFN) + "y"), - ABSTFN + "y") - self.assertEqual(realpath(ABSTFN+"1/../" + basename(ABSTFN) + "1"), - ABSTFN + "1") + self.assertRaises(OSError, realpath, ABSTFN+"1/../" + basename(ABSTFN) + "y") + self.assertRaises(OSError, realpath, ABSTFN+"1/../" + basename(ABSTFN) + "1") os.symlink(basename(ABSTFN) + "a/b", ABSTFN+"a") - self.assertEqual(realpath(ABSTFN+"a"), ABSTFN+"a/b") + self.assertRaises(OSError, realpath, ABSTFN+"a") os.symlink("../" + basename(dirname(ABSTFN)) + "/" + basename(ABSTFN) + "c", ABSTFN+"c") - self.assertEqual(realpath(ABSTFN+"c"), ABSTFN+"c") + self.assertRaises(OSError, realpath, ABSTFN+"c") # Test using relative path as well. with os_helper.change_cwd(dirname(ABSTFN)): - self.assertEqual(realpath(basename(ABSTFN)), ABSTFN) + self.assertRaises(OSError, realpath, basename(ABSTFN)) finally: os_helper.unlink(ABSTFN) os_helper.unlink(ABSTFN+"1") From 4114be91f6f0c5db26749807d223bdbb7a40536b Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 04:38:48 +0100 Subject: [PATCH 07/23] Raise RuntimeError in Path.resolve() if realpath() raises ERROR_CANT_RESOLVE_FILENAME --- Lib/pathlib.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 6cfde1f2b394b1..ae257a8911e98a 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -29,14 +29,17 @@ # Internals # +_WINERROR_NOT_READY = 21 # drive exists but is not accessible +_WINERROR_INVALID_NAME = 123 # fix for bpo-35306 +_WINERROR_CANT_RESOLVE_FILENAME = 1921 # broken symlink pointing to itself + # EBADF - guard against macOS `stat` throwing EBADF _IGNORED_ERROS = (ENOENT, ENOTDIR, EBADF, ELOOP) _IGNORED_WINERRORS = ( - 21, # ERROR_NOT_READY - drive exists but is not accessible - 123, # ERROR_INVALID_NAME - fix for bpo-35306 - 1921, # ERROR_CANT_RESOLVE_FILENAME - fix for broken symlink pointing to itself -) + _WINERROR_NOT_READY, + _WINERROR_INVALID_NAME, + _WINERROR_CANT_RESOLVE_FILENAME) def _ignore_error(exception): return (getattr(exception, 'errno', None) in _IGNORED_ERROS or @@ -1061,7 +1064,8 @@ def resolve(self, strict=False): try: p = self._accessor.realpath(self, strict=strict) except OSError as ex: - if ex.errno == ELOOP: + winerr = getattr(ex, 'winerror', 0) + if ex.errno == ELOOP or winerr == _WINERROR_CANT_RESOLVE_FILENAME: raise RuntimeError("Symlink loop from %r", ex.filename) raise return self._from_parts((p,)) From 2a594ced11da760695e9d8601d7f2023ef0789bd Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 04:39:07 +0100 Subject: [PATCH 08/23] Remove unused import --- Lib/pathlib.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index ae257a8911e98a..87a3b0ff849bb0 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -14,12 +14,6 @@ from urllib.parse import quote_from_bytes as urlquote_from_bytes -if os.name == 'nt': - from nt import _getfinalpathname -else: - _getfinalpathname = None - - __all__ = [ "PurePath", "PurePosixPath", "PureWindowsPath", "Path", "PosixPath", "WindowsPath", From fb36a40db972571cf37d42f392eb4df3ae328db8 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 04:41:42 +0100 Subject: [PATCH 09/23] Make documentation a little more general to accurately cover Windows. --- Doc/library/os.path.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/os.path.rst b/Doc/library/os.path.rst index 941af5ca648ae0..7f9964cd41b1b4 100644 --- a/Doc/library/os.path.rst +++ b/Doc/library/os.path.rst @@ -364,8 +364,8 @@ the :mod:`glob` module.) The *strict* parameter was added. .. versionchanged:: 3.10 - Raises :exc:`OSError` with :const:`~errno.ELOOP` when a symbolic link - cycle occurs. Previously returned one member of the cycle. + Raises :exc:`OSError` when a symbolic link cycle occurs. Previously + returned one member of the cycle. .. function:: relpath(path, start=os.curdir) From 0634486f61c022964a7a444f25eab00456041e2e Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 05:06:03 +0100 Subject: [PATCH 10/23] First pass on fixing `ntpath` tests --- Lib/test/test_ntpath.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index f97aca5f94f579..6b5ea4fe1df389 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -340,8 +340,7 @@ def test_realpath_broken_symlinks(self): @os_helper.skip_unless_symlink @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') def test_realpath_symlink_loops(self): - # Symlink loops are non-deterministic as to which path is returned, but - # it will always be the fully resolved path of one member of the cycle + # Symlink loops raise OSError ABSTFN = ntpath.abspath(os_helper.TESTFN) self.addCleanup(os_helper.unlink, ABSTFN) self.addCleanup(os_helper.unlink, ABSTFN + "1") @@ -351,16 +350,13 @@ def test_realpath_symlink_loops(self): self.addCleanup(os_helper.unlink, ABSTFN + "a") os.symlink(ABSTFN, ABSTFN) - self.assertPathEqual(ntpath.realpath(ABSTFN), ABSTFN) + self.assertRaises(OSError, ntpath.realpath, ABSTFN) os.symlink(ABSTFN + "1", ABSTFN + "2") os.symlink(ABSTFN + "2", ABSTFN + "1") - expected = (ABSTFN + "1", ABSTFN + "2") - self.assertPathIn(ntpath.realpath(ABSTFN + "1"), expected) - self.assertPathIn(ntpath.realpath(ABSTFN + "2"), expected) - - self.assertPathIn(ntpath.realpath(ABSTFN + "1\\x"), - (ntpath.join(r, "x") for r in expected)) + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "2") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\x") self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\.."), ntpath.dirname(ABSTFN)) self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\x"), @@ -369,9 +365,8 @@ def test_realpath_symlink_loops(self): self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "y"), ABSTFN + "x") - self.assertPathIn(ntpath.realpath(ABSTFN + "1\\..\\" - + ntpath.basename(ABSTFN) + "1"), - expected) + self.assertRaises(OSError, ntpath.realpath, + ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "1") os.symlink(ntpath.basename(ABSTFN) + "a\\b", ABSTFN + "a") self.assertPathEqual(ntpath.realpath(ABSTFN + "a"), ABSTFN + "a") From 600fc9e7d1b2244ddd28b0e8fa159feab87a84e5 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 05:34:58 +0100 Subject: [PATCH 11/23] Second pass on fixing `ntpath` tests --- Lib/test/test_ntpath.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index 6b5ea4fe1df389..74fd0cea2d79c3 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -369,14 +369,14 @@ def test_realpath_symlink_loops(self): ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "1") os.symlink(ntpath.basename(ABSTFN) + "a\\b", ABSTFN + "a") - self.assertPathEqual(ntpath.realpath(ABSTFN + "a"), ABSTFN + "a") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "a") os.symlink("..\\" + ntpath.basename(ntpath.dirname(ABSTFN)) + "\\" + ntpath.basename(ABSTFN) + "c", ABSTFN + "c") - self.assertPathEqual(ntpath.realpath(ABSTFN + "c"), ABSTFN + "c") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "c") # Test using relative path as well. - self.assertPathEqual(ntpath.realpath(ntpath.basename(ABSTFN)), ABSTFN) + self.assertRaises(OSError, ntpath.realpath, ntpath.basename(ABSTFN)) @os_helper.skip_unless_symlink @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') From 047471c1b445f68efb02fc58b15e42a31bf3ffbb Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 21:24:04 +0100 Subject: [PATCH 12/23] Fix indentation --- Lib/posixpath.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 46b0060177fb57..210114bafb3397 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -434,8 +434,8 @@ def _joinrealpath(path, rest, strict, seen): else: is_link = stat.S_ISLNK(st.st_mode) if not is_link: - path = newpath - continue + path = newpath + continue # Resolve the symbolic link if newpath in seen: # Already seen this path From bf3a3f793cea27fdd1b549b877286df5932c4527 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 21:45:59 +0100 Subject: [PATCH 13/23] Copy description of 'strict' from pathlib. --- Doc/library/os.path.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/library/os.path.rst b/Doc/library/os.path.rst index 7f9964cd41b1b4..d5fcdc268f3bd1 100644 --- a/Doc/library/os.path.rst +++ b/Doc/library/os.path.rst @@ -350,9 +350,9 @@ the :mod:`glob` module.) links encountered in the path (if they are supported by the operating system). - In non-strict mode (the default), missing or inaccessible ancestors are - permitted; when encountered, the remainder of the path joined on and - returned. In strict mode an :exc:`OSError` is raised in this scenario. + If the path doesn't exist and *strict* is ``True``, :exc:`FileNotFoundError` + is raised. If *strict* is ``False``, the path is resolved as far as possible + and any remainder is appended without checking whether it exists. .. versionchanged:: 3.6 Accepts a :term:`path-like object`. From f839db022be47b6d252ec4ba4cf83be53f1c3122 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 22:02:02 +0100 Subject: [PATCH 14/23] Add tests --- Lib/test/test_ntpath.py | 12 ++++++++++++ Lib/test/test_posixpath.py | 13 +++++++++++++ 2 files changed, 25 insertions(+) diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index 74fd0cea2d79c3..f1c1a5867a5f2c 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -269,6 +269,18 @@ def test_realpath_basic(self): self.assertPathEqual(ntpath.realpath(os.fsencode(ABSTFN + "1")), os.fsencode(ABSTFN)) + @unittest.skipUnless(hasattr(os, "symlink"), + "Missing symlink implementation") + @skip_if_ABSTFN_contains_backslash + def test_realpath_strict(self): + # Bug #43757: raise FileNotFoundError in strict mode if we encounter + # a path that does not exist. + ABSTFN = ntpath.abspath(os_helper.TESTFN) + os.symlink(ABSTFN + "1", ABSTFN) + self.addCleanup(os_helper.unlink, ABSTFN) + self.assertRaises(FileNotFoundError, ntpath.realpath, ABSTFN, strict=True) + self.assertRaises(FileNotFoundError, ntpath.realpath, ABSTFN + "2", strict=True) + @os_helper.skip_unless_symlink @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') def test_realpath_relative(self): diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 8ab27f93b33471..3f599336915e20 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -355,6 +355,19 @@ def test_realpath_basic(self): finally: os_helper.unlink(ABSTFN) + @unittest.skipUnless(hasattr(os, "symlink"), + "Missing symlink implementation") + @skip_if_ABSTFN_contains_backslash + def test_realpath_strict(self): + # Bug #43757: raise FileNotFoundError in strict mode if we encounter + # a path that does not exist. + try: + os.symlink(ABSTFN+"1", ABSTFN) + self.assertRaises(FileNotFoundError, realpath, ABSTFN, strict=True) + self.assertRaises(FileNotFoundError, realpath, ABSTFN + "2", strict=True) + finally: + os_helper.unlink(ABSTFN) + @unittest.skipUnless(hasattr(os, "symlink"), "Missing symlink implementation") @skip_if_ABSTFN_contains_backslash From 2d30bb783c1b8478f95f68ab568d14906f97c528 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 22:13:26 +0100 Subject: [PATCH 15/23] Add NEWS --- .../next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst diff --git a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst new file mode 100644 index 00000000000000..a58452bbf74dd1 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst @@ -0,0 +1,6 @@ +:func:`os.path.realpath` now raises :exc:`OSError` when a symlink loop is +encountered. Previously it returned a path with unresolved symlinks. + +:func:`os.path.realpath` now accepts a *strict* keyword-only argument. +When set to ``True``, :exc:`FileNotFoundException` is raised if a path +doesn't exist. From 68c6533511e564e5d7d633e854b3ca4ef5035541 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 8 Apr 2021 22:15:36 +0100 Subject: [PATCH 16/23] Fix ntpath tests (pass 1) --- Lib/test/test_ntpath.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index f1c1a5867a5f2c..b10a17f6d57ce5 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -269,9 +269,8 @@ def test_realpath_basic(self): self.assertPathEqual(ntpath.realpath(os.fsencode(ABSTFN + "1")), os.fsencode(ABSTFN)) - @unittest.skipUnless(hasattr(os, "symlink"), - "Missing symlink implementation") - @skip_if_ABSTFN_contains_backslash + @os_helper.skip_unless_symlink + @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') def test_realpath_strict(self): # Bug #43757: raise FileNotFoundError in strict mode if we encounter # a path that does not exist. From 9fa60eb9ec059e164eda1e3010f87d39627a5eaa Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 10 Apr 2021 01:02:03 +0100 Subject: [PATCH 17/23] Add some notes on OS differences --- Doc/library/os.path.rst | 8 ++++++++ Lib/test/test_ntpath.py | 3 +++ 2 files changed, 11 insertions(+) diff --git a/Doc/library/os.path.rst b/Doc/library/os.path.rst index d5fcdc268f3bd1..7b46ae3cae8887 100644 --- a/Doc/library/os.path.rst +++ b/Doc/library/os.path.rst @@ -354,6 +354,14 @@ the :mod:`glob` module.) is raised. If *strict* is ``False``, the path is resolved as far as possible and any remainder is appended without checking whether it exists. + .. note:: + This function emulates the operating system's procedure for making a path + canonical, which differs slightly between Windows and UNIX with respect + to how links and subsequent path components interact. + + Operating system APIs make paths canonical as needed, so it's not + normally necessary to call this function. + .. versionchanged:: 3.6 Accepts a :term:`path-like object`. diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index b10a17f6d57ce5..fbfcc776fb4149 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -368,6 +368,9 @@ def test_realpath_symlink_loops(self): self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1") self.assertRaises(OSError, ntpath.realpath, ABSTFN + "2") self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\x") + + # Windows eliminates '..' components before resolving links, so the + # following 3 realpath() calls are not expected to raise. self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\.."), ntpath.dirname(ABSTFN)) self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\x"), From f979947a207015bdb87dfdc5f3f1fa316daca66b Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 10 Apr 2021 01:04:16 +0100 Subject: [PATCH 18/23] Split NEWS --- .../next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst | 3 +++ .../next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst diff --git a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst new file mode 100644 index 00000000000000..92f2c3df499507 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst @@ -0,0 +1,3 @@ +:func:`os.path.realpath` now accepts a *strict* keyword-only argument. +When set to ``True``, :exc:`FileNotFoundException` is raised if a path +doesn't exist. diff --git a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst index a58452bbf74dd1..f7ddfac922a82f 100644 --- a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst +++ b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst @@ -1,6 +1,2 @@ :func:`os.path.realpath` now raises :exc:`OSError` when a symlink loop is encountered. Previously it returned a path with unresolved symlinks. - -:func:`os.path.realpath` now accepts a *strict* keyword-only argument. -When set to ``True``, :exc:`FileNotFoundException` is raised if a path -doesn't exist. From a82bc185699ea1550fd9588ab668ad7674a25544 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 10 Apr 2021 23:23:14 +0100 Subject: [PATCH 19/23] Do not suppress initial ERROR_CANT_RESOLVE_FILENAME error in non-strict mode. --- Lib/ntpath.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/ntpath.py b/Lib/ntpath.py index 2498c3116c7130..9f577c61e8e060 100644 --- a/Lib/ntpath.py +++ b/Lib/ntpath.py @@ -659,7 +659,10 @@ def realpath(path, *, strict=False): path = _getfinalpathname(path) initial_winerror = 0 except OSError as ex: - if strict: + # ERROR_CANT_RESOLVE_FILENAME (1921) is from exceeding the + # max allowed number of reparse attempts (currently 63), which + # is either due to a loop or a chain of links that's too long. + if strict or ex.winerror == 1921: raise initial_winerror = ex.winerror path = _getfinalpathname_nonstrict(path) From 86c318c61c8b9eb29e6f609af46fe25dcfd3ea61 Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 13 Apr 2021 01:55:29 +0100 Subject: [PATCH 20/23] realpath(): only raise OSError for symlink loops in strict mode --- Doc/library/os.path.rst | 11 ++- Lib/ntpath.py | 8 +-- Lib/pathlib.py | 18 +++-- Lib/posixpath.py | 16 +++-- Lib/test/test_ntpath.py | 72 ++++++++++++++++--- Lib/test/test_posixpath.py | 69 ++++++++++++++---- .../2021-04-08-22-11-27.bpo-25264.b33fa0.rst | 4 +- .../2021-04-08-22-11-27.bpo-25264.hEqA5d.rst | 2 - 8 files changed, 151 insertions(+), 49 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst diff --git a/Doc/library/os.path.rst b/Doc/library/os.path.rst index 7b46ae3cae8887..d06d9ce8c9e3d7 100644 --- a/Doc/library/os.path.rst +++ b/Doc/library/os.path.rst @@ -350,9 +350,10 @@ the :mod:`glob` module.) links encountered in the path (if they are supported by the operating system). - If the path doesn't exist and *strict* is ``True``, :exc:`FileNotFoundError` - is raised. If *strict* is ``False``, the path is resolved as far as possible - and any remainder is appended without checking whether it exists. + If a path doesn't exist or a symlink loop is encountered, and *strict* is + ``True``, :exc:`OSError` is raised. If *strict* is ``False``, the path is + resolved as far as possible and any remainder is appended without checking + whether it exists. .. note:: This function emulates the operating system's procedure for making a path @@ -371,10 +372,6 @@ the :mod:`glob` module.) .. versionchanged:: 3.10 The *strict* parameter was added. - .. versionchanged:: 3.10 - Raises :exc:`OSError` when a symbolic link cycle occurs. Previously - returned one member of the cycle. - .. function:: relpath(path, start=os.curdir) diff --git a/Lib/ntpath.py b/Lib/ntpath.py index 9f577c61e8e060..527c7ae1938fbb 100644 --- a/Lib/ntpath.py +++ b/Lib/ntpath.py @@ -603,7 +603,8 @@ def _getfinalpathname_nonstrict(path): # 87: ERROR_INVALID_PARAMETER # 123: ERROR_INVALID_NAME # 1920: ERROR_CANT_ACCESS_FILE - allowed_winerror = 1, 2, 3, 5, 21, 32, 50, 67, 87, 123, 1920 + # 1921: ERROR_CANT_RESOLVE_FILENAME (implies unfollowable symlink) + allowed_winerror = 1, 2, 3, 5, 21, 32, 50, 67, 87, 123, 1920, 1921 # Non-strict algorithm is to find as much of the target directory # as we can and join the rest. @@ -659,10 +660,7 @@ def realpath(path, *, strict=False): path = _getfinalpathname(path) initial_winerror = 0 except OSError as ex: - # ERROR_CANT_RESOLVE_FILENAME (1921) is from exceeding the - # max allowed number of reparse attempts (currently 63), which - # is either due to a loop or a chain of links that's too long. - if strict or ex.winerror == 1921: + if strict: raise initial_winerror = ex.winerror path = _getfinalpathname_nonstrict(path) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 87a3b0ff849bb0..e87ea3f210825d 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -1055,14 +1055,22 @@ def resolve(self, strict=False): normalizing it (for example turning slashes into backslashes under Windows). """ + try: - p = self._accessor.realpath(self, strict=strict) + s = self._accessor.realpath(self, strict=strict) + p = self._from_parts((s,)) + + # In non-strict mode, realpath() doesn't raise on symlink loops. + # Ensure we get an exception by calling stat() + if not strict: + p.stat() except OSError as ex: - winerr = getattr(ex, 'winerror', 0) - if ex.errno == ELOOP or winerr == _WINERROR_CANT_RESOLVE_FILENAME: + winerror = getattr(ex, 'winerror', 0) + if ex.errno == ELOOP or winerror == _WINERROR_CANT_RESOLVE_FILENAME: raise RuntimeError("Symlink loop from %r", ex.filename) - raise - return self._from_parts((p,)) + if strict: + raise + return p def stat(self, *, follow_symlinks=True): """ diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 210114bafb3397..259baa64b193b8 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -391,7 +391,7 @@ def realpath(filename, *, strict=False): """Return the canonical path of the specified filename, eliminating any symbolic links encountered in the path.""" filename = os.fspath(filename) - path = _joinrealpath(filename[:0], filename, strict, {}) + path, ok = _joinrealpath(filename[:0], filename, strict, {}) return abspath(path) # Join two paths, normalizing and eliminating any symbolic links @@ -444,13 +444,19 @@ def _joinrealpath(path, rest, strict, seen): # use cached value continue # The symlink is not resolved, so we must have a symlink loop. - # Raise OSError(errno.ELOOP) - os.stat(newpath) + if strict: + # Raise OSError(errno.ELOOP) + os.stat(newpath) + else: + # Return already resolved part + rest of the path unchanged. + return join(newpath, rest), False seen[newpath] = None # not resolved symlink - path = _joinrealpath(path, os.readlink(newpath), strict, seen) + path, ok = _joinrealpath(path, os.readlink(newpath), strict, seen) + if not ok: + return join(path, rest), False seen[newpath] = path # resolved symlink - return path + return path, True supports_unicode_filenames = (sys.platform == 'darwin') diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index fbfcc776fb4149..85a3203e5c75da 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -351,7 +351,9 @@ def test_realpath_broken_symlinks(self): @os_helper.skip_unless_symlink @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') def test_realpath_symlink_loops(self): - # Symlink loops raise OSError + # Symlink loops in non-strict mode are non-deterministic as to which + # path is returned, but it will always be the fully resolved path of + # one member of the cycle ABSTFN = ntpath.abspath(os_helper.TESTFN) self.addCleanup(os_helper.unlink, ABSTFN) self.addCleanup(os_helper.unlink, ABSTFN + "1") @@ -361,16 +363,16 @@ def test_realpath_symlink_loops(self): self.addCleanup(os_helper.unlink, ABSTFN + "a") os.symlink(ABSTFN, ABSTFN) - self.assertRaises(OSError, ntpath.realpath, ABSTFN) + self.assertPathEqual(ntpath.realpath(ABSTFN), ABSTFN) os.symlink(ABSTFN + "1", ABSTFN + "2") os.symlink(ABSTFN + "2", ABSTFN + "1") - self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1") - self.assertRaises(OSError, ntpath.realpath, ABSTFN + "2") - self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\x") + expected = (ABSTFN + "1", ABSTFN + "2") + self.assertPathIn(ntpath.realpath(ABSTFN + "1"), expected) + self.assertPathIn(ntpath.realpath(ABSTFN + "2"), expected) - # Windows eliminates '..' components before resolving links, so the - # following 3 realpath() calls are not expected to raise. + self.assertPathIn(ntpath.realpath(ABSTFN + "1\\x"), + (ntpath.join(r, "x") for r in expected)) self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\.."), ntpath.dirname(ABSTFN)) self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\x"), @@ -379,18 +381,66 @@ def test_realpath_symlink_loops(self): self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "y"), ABSTFN + "x") + self.assertPathIn(ntpath.realpath(ABSTFN + "1\\..\\" + + ntpath.basename(ABSTFN) + "1"), + expected) + + os.symlink(ntpath.basename(ABSTFN) + "a\\b", ABSTFN + "a") + self.assertPathEqual(ntpath.realpath(ABSTFN + "a"), ABSTFN + "a") + + os.symlink("..\\" + ntpath.basename(ntpath.dirname(ABSTFN)) + + "\\" + ntpath.basename(ABSTFN) + "c", ABSTFN + "c") + self.assertPathEqual(ntpath.realpath(ABSTFN + "c"), ABSTFN + "c") + + # Test using relative path as well. + self.assertPathEqual(ntpath.realpath(ntpath.basename(ABSTFN)), ABSTFN) + + @os_helper.skip_unless_symlink + @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') + def test_realpath_symlink_loops_strict(self): + # Symlink loops raise OSError in strict mode + ABSTFN = ntpath.abspath(os_helper.TESTFN) + self.addCleanup(os_helper.unlink, ABSTFN) + self.addCleanup(os_helper.unlink, ABSTFN + "1") + self.addCleanup(os_helper.unlink, ABSTFN + "2") + self.addCleanup(os_helper.unlink, ABSTFN + "y") + self.addCleanup(os_helper.unlink, ABSTFN + "c") + self.addCleanup(os_helper.unlink, ABSTFN + "a") + + os.symlink(ABSTFN, ABSTFN) + self.assertRaises(OSError, ntpath.realpath, ABSTFN, strict=True) + + os.symlink(ABSTFN + "1", ABSTFN + "2") + os.symlink(ABSTFN + "2", ABSTFN + "1") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1", strict=True) + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "2", strict=True) + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\x", strict=True) + + # Windows eliminates '..' components before resolving links, so the + # following 3 realpath() calls are not expected to raise. + self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..", strict=True), + ntpath.dirname(ABSTFN)) + self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\x", strict=True), + ntpath.dirname(ABSTFN) + "\\x") + os.symlink(ABSTFN + "x", ABSTFN + "y") + self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\" + + ntpath.basename(ABSTFN) + "y", + strict=True), + ABSTFN + "x") self.assertRaises(OSError, ntpath.realpath, - ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "1") + ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "1", + strict=True) os.symlink(ntpath.basename(ABSTFN) + "a\\b", ABSTFN + "a") - self.assertRaises(OSError, ntpath.realpath, ABSTFN + "a") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "a", strict=True) os.symlink("..\\" + ntpath.basename(ntpath.dirname(ABSTFN)) + "\\" + ntpath.basename(ABSTFN) + "c", ABSTFN + "c") - self.assertRaises(OSError, ntpath.realpath, ABSTFN + "c") + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "c", strict=True) # Test using relative path as well. - self.assertRaises(OSError, ntpath.realpath, ntpath.basename(ABSTFN)) + self.assertRaises(OSError, ntpath.realpath, ntpath.basename(ABSTFN), + strict=True) @os_helper.skip_unless_symlink @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 3f599336915e20..8d398ec0103544 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -382,33 +382,78 @@ def test_realpath_relative(self): "Missing symlink implementation") @skip_if_ABSTFN_contains_backslash def test_realpath_symlink_loops(self): - # Bug #43757, raise OSError if we get into an infinite symlink loop. + # Bug #930024, return the path unchanged if we get into an infinite + # symlink loop in non-strict mode (default). try: os.symlink(ABSTFN, ABSTFN) - self.assertRaises(OSError, realpath, ABSTFN) + self.assertEqual(realpath(ABSTFN), ABSTFN) os.symlink(ABSTFN+"1", ABSTFN+"2") os.symlink(ABSTFN+"2", ABSTFN+"1") - self.assertRaises(OSError, realpath, ABSTFN+"1") - self.assertRaises(OSError, realpath, ABSTFN+"2") + self.assertEqual(realpath(ABSTFN+"1"), ABSTFN+"1") + self.assertEqual(realpath(ABSTFN+"2"), ABSTFN+"2") - self.assertRaises(OSError, realpath, ABSTFN+"1/x") - self.assertRaises(OSError, realpath, ABSTFN+"1/..") - self.assertRaises(OSError, realpath, ABSTFN+"1/../x") + self.assertEqual(realpath(ABSTFN+"1/x"), ABSTFN+"1/x") + self.assertEqual(realpath(ABSTFN+"1/.."), dirname(ABSTFN)) + self.assertEqual(realpath(ABSTFN+"1/../x"), dirname(ABSTFN) + "/x") os.symlink(ABSTFN+"x", ABSTFN+"y") - self.assertRaises(OSError, realpath, ABSTFN+"1/../" + basename(ABSTFN) + "y") - self.assertRaises(OSError, realpath, ABSTFN+"1/../" + basename(ABSTFN) + "1") + self.assertEqual(realpath(ABSTFN+"1/../" + basename(ABSTFN) + "y"), + ABSTFN + "y") + self.assertEqual(realpath(ABSTFN+"1/../" + basename(ABSTFN) + "1"), + ABSTFN + "1") os.symlink(basename(ABSTFN) + "a/b", ABSTFN+"a") - self.assertRaises(OSError, realpath, ABSTFN+"a") + self.assertEqual(realpath(ABSTFN+"a"), ABSTFN+"a/b") os.symlink("../" + basename(dirname(ABSTFN)) + "/" + basename(ABSTFN) + "c", ABSTFN+"c") - self.assertRaises(OSError, realpath, ABSTFN+"c") + self.assertEqual(realpath(ABSTFN+"c"), ABSTFN+"c") # Test using relative path as well. with os_helper.change_cwd(dirname(ABSTFN)): - self.assertRaises(OSError, realpath, basename(ABSTFN)) + self.assertEqual(realpath(basename(ABSTFN)), ABSTFN) + finally: + os_helper.unlink(ABSTFN) + os_helper.unlink(ABSTFN+"1") + os_helper.unlink(ABSTFN+"2") + os_helper.unlink(ABSTFN+"y") + os_helper.unlink(ABSTFN+"c") + os_helper.unlink(ABSTFN+"a") + + @unittest.skipUnless(hasattr(os, "symlink"), + "Missing symlink implementation") + @skip_if_ABSTFN_contains_backslash + def test_realpath_symlink_loops_strict(self): + # Bug #43757, raise OSError if we get into an infinite symlink loop in + # strict mode. + try: + os.symlink(ABSTFN, ABSTFN) + self.assertRaises(OSError, realpath, ABSTFN, strict=True) + + os.symlink(ABSTFN+"1", ABSTFN+"2") + os.symlink(ABSTFN+"2", ABSTFN+"1") + self.assertRaises(OSError, realpath, ABSTFN+"1", strict=True) + self.assertRaises(OSError, realpath, ABSTFN+"2", strict=True) + + self.assertRaises(OSError, realpath, ABSTFN+"1/x", strict=True) + self.assertRaises(OSError, realpath, ABSTFN+"1/..", strict=True) + self.assertRaises(OSError, realpath, ABSTFN+"1/../x", strict=True) + os.symlink(ABSTFN+"x", ABSTFN+"y") + self.assertRaises(OSError, realpath, + ABSTFN+"1/../" + basename(ABSTFN) + "y", strict=True) + self.assertRaises(OSError, realpath, + ABSTFN+"1/../" + basename(ABSTFN) + "1", strict=True) + + os.symlink(basename(ABSTFN) + "a/b", ABSTFN+"a") + self.assertRaises(OSError, realpath, ABSTFN+"a", strict=True) + + os.symlink("../" + basename(dirname(ABSTFN)) + "/" + + basename(ABSTFN) + "c", ABSTFN+"c") + self.assertRaises(OSError, realpath, ABSTFN+"c", strict=True) + + # Test using relative path as well. + with os_helper.change_cwd(dirname(ABSTFN)): + self.assertRaises(OSError, realpath, basename(ABSTFN), strict=True) finally: os_helper.unlink(ABSTFN) os_helper.unlink(ABSTFN+"1") diff --git a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst index 92f2c3df499507..593846ec15c5b9 100644 --- a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst +++ b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.b33fa0.rst @@ -1,3 +1,3 @@ :func:`os.path.realpath` now accepts a *strict* keyword-only argument. -When set to ``True``, :exc:`FileNotFoundException` is raised if a path -doesn't exist. +When set to ``True``, :exc:`OSError` is raised if a path doesn't exist +or a symlink loop is encountered. diff --git a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst b/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst deleted file mode 100644 index f7ddfac922a82f..00000000000000 --- a/Misc/NEWS.d/next/Library/2021-04-08-22-11-27.bpo-25264.hEqA5d.rst +++ /dev/null @@ -1,2 +0,0 @@ -:func:`os.path.realpath` now raises :exc:`OSError` when a symlink loop is -encountered. Previously it returned a path with unresolved symlinks. From 7362f7e8fa68924321d76456ec4210083acab0a1 Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 13 Apr 2021 02:18:03 +0100 Subject: [PATCH 21/23] Fix test_ntpath (pass 1) --- Lib/test/test_ntpath.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index 85a3203e5c75da..259ffef61b3b4f 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -415,13 +415,11 @@ def test_realpath_symlink_loops_strict(self): self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1", strict=True) self.assertRaises(OSError, ntpath.realpath, ABSTFN + "2", strict=True) self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\x", strict=True) - + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\..\\x", strict=True) # Windows eliminates '..' components before resolving links, so the - # following 3 realpath() calls are not expected to raise. + # following 2 realpath() calls are not expected to raise. self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..", strict=True), ntpath.dirname(ABSTFN)) - self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\x", strict=True), - ntpath.dirname(ABSTFN) + "\\x") os.symlink(ABSTFN + "x", ABSTFN + "y") self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "y", From 72147c5d1955e21c8dba99c22ab8470b993917d7 Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 13 Apr 2021 02:34:46 +0100 Subject: [PATCH 22/23] Fix test_ntpath (pass 2) --- Lib/test/test_ntpath.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index 259ffef61b3b4f..661c59d6171635 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -415,16 +415,15 @@ def test_realpath_symlink_loops_strict(self): self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1", strict=True) self.assertRaises(OSError, ntpath.realpath, ABSTFN + "2", strict=True) self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\x", strict=True) - self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\..\\x", strict=True) # Windows eliminates '..' components before resolving links, so the - # following 2 realpath() calls are not expected to raise. + # following call is not expected to raise. self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..", strict=True), ntpath.dirname(ABSTFN)) + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\..\\x", strict=True) os.symlink(ABSTFN + "x", ABSTFN + "y") - self.assertPathEqual(ntpath.realpath(ABSTFN + "1\\..\\" + self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "y", - strict=True), - ABSTFN + "x") + strict=True) self.assertRaises(OSError, ntpath.realpath, ABSTFN + "1\\..\\" + ntpath.basename(ABSTFN) + "1", strict=True) From df04357fafe104d9114a245c24dbea92b2b5e60b Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 13 Apr 2021 03:44:52 +0100 Subject: [PATCH 23/23] Split up exception handling in resolve() for greater clarity. --- Lib/pathlib.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index e87ea3f210825d..073fce82ad5705 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -1056,20 +1056,25 @@ def resolve(self, strict=False): Windows). """ + def check_eloop(e): + winerror = getattr(e, 'winerror', 0) + if e.errno == ELOOP or winerror == _WINERROR_CANT_RESOLVE_FILENAME: + raise RuntimeError("Symlink loop from %r" % e.filename) + try: s = self._accessor.realpath(self, strict=strict) - p = self._from_parts((s,)) + except OSError as e: + check_eloop(e) + raise + p = self._from_parts((s,)) - # In non-strict mode, realpath() doesn't raise on symlink loops. - # Ensure we get an exception by calling stat() - if not strict: + # In non-strict mode, realpath() doesn't raise on symlink loops. + # Ensure we get an exception by calling stat() + if not strict: + try: p.stat() - except OSError as ex: - winerror = getattr(ex, 'winerror', 0) - if ex.errno == ELOOP or winerror == _WINERROR_CANT_RESOLVE_FILENAME: - raise RuntimeError("Symlink loop from %r", ex.filename) - if strict: - raise + except OSError as e: + check_eloop(e) return p def stat(self, *, follow_symlinks=True):