From 66c0f01f98fd6db0a4b1e789b9db9c7247a24ba9 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 15 Nov 2019 15:25:03 -0800 Subject: [PATCH] bpo-38453: Ensure ntpath.realpath correctly resolves relative paths (GH-16967) Ensure isabs() is always True for \\?\ prefixed paths Avoid unnecessary usage of readlink() to avoid resolving broken links incorrectly Ensure shutil tests run in test directory --- Lib/ntpath.py | 49 ++++++++++++++--- Lib/test/test_ntpath.py | 52 ++++++++++++++----- Lib/test/test_shutil.py | 12 +++-- .../2019-10-28-10-32-43.bpo-38453.NwwatW.rst | 1 + 4 files changed, 91 insertions(+), 23 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2019-10-28-10-32-43.bpo-38453.NwwatW.rst diff --git a/Lib/ntpath.py b/Lib/ntpath.py index d4ecff97c95..6f771773a7d 100644 --- a/Lib/ntpath.py +++ b/Lib/ntpath.py @@ -61,6 +61,14 @@ def normcase(s): def isabs(s): """Test whether a path is absolute""" s = os.fspath(s) + # Paths beginning with \\?\ are always absolute, but do not + # necessarily contain a drive. + if isinstance(s, bytes): + if s.replace(b'/', b'\\').startswith(b'\\\\?\\'): + return True + else: + if s.replace('/', '\\').startswith('\\\\?\\'): + return True s = splitdrive(s)[1] return len(s) > 0 and s[0] in _get_bothseps(s) @@ -526,10 +534,7 @@ except ImportError: # realpath is a no-op on systems without _getfinalpathname support. realpath = abspath else: - def _readlink_deep(path, seen=None): - if seen is None: - seen = set() - + def _readlink_deep(path): # These error codes indicate that we should stop reading links and # return the path we currently have. # 1: ERROR_INVALID_FUNCTION @@ -546,10 +551,22 @@ else: # 4393: ERROR_REPARSE_TAG_INVALID allowed_winerror = 1, 2, 3, 5, 21, 32, 50, 67, 87, 4390, 4392, 4393 + seen = set() while normcase(path) not in seen: seen.add(normcase(path)) try: + old_path = path path = _nt_readlink(path) + # Links may be relative, so resolve them against their + # own location + if not isabs(path): + # If it's something other than a symlink, we don't know + # what it's actually going to be resolved against, so + # just return the old path. + if not islink(old_path): + path = old_path + break + path = normpath(join(dirname(old_path), path)) except OSError as ex: if ex.winerror in allowed_winerror: break @@ -579,23 +596,31 @@ else: # Non-strict algorithm is to find as much of the target directory # as we can and join the rest. tail = '' - seen = set() while path: try: - path = _readlink_deep(path, seen) path = _getfinalpathname(path) return join(path, tail) if tail else path except OSError as ex: if ex.winerror not in allowed_winerror: raise + try: + # The OS could not resolve this path fully, so we attempt + # to follow the link ourselves. If we succeed, join the tail + # and return. + new_path = _readlink_deep(path) + if new_path != path: + return join(new_path, tail) if tail else new_path + except OSError: + # If we fail to readlink(), let's keep traversing + pass path, name = split(path) # TODO (bpo-38186): Request the real file name from the directory # entry using FindFirstFileW. For now, we will return the path # as best we have it if path and not name: - return abspath(path + tail) + return path + tail tail = join(name, tail) if tail else name - return abspath(tail) + return tail def realpath(path): path = normpath(path) @@ -604,12 +629,20 @@ else: unc_prefix = b'\\\\?\\UNC\\' new_unc_prefix = b'\\\\' cwd = os.getcwdb() + # bpo-38081: Special case for realpath(b'nul') + if normcase(path) == normcase(os.fsencode(devnull)): + return b'\\\\.\\NUL' else: prefix = '\\\\?\\' unc_prefix = '\\\\?\\UNC\\' new_unc_prefix = '\\\\' cwd = os.getcwd() + # bpo-38081: Special case for realpath('nul') + if normcase(path) == normcase(devnull): + return '\\\\.\\NUL' had_prefix = path.startswith(prefix) + if not had_prefix and not isabs(path): + path = join(cwd, path) try: path = _getfinalpathname(path) initial_winerror = 0 diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py index e0ec4419852..a84b94c9bad 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -286,14 +286,16 @@ class TestNtpath(NtpathTestCase): ABSTFN + r"\missing") self.assertPathEqual(ntpath.realpath(r"broken\foo"), ABSTFN + r"\missing\foo") + # bpo-38453: We no longer recursively resolve segments of relative + # symlinks that the OS cannot resolve. self.assertPathEqual(ntpath.realpath(r"broken1"), - ABSTFN + r"\missing\bar") + ABSTFN + r"\broken\bar") self.assertPathEqual(ntpath.realpath(r"broken1\baz"), - ABSTFN + r"\missing\bar\baz") + ABSTFN + r"\broken\bar\baz") self.assertPathEqual(ntpath.realpath("broken2"), - ABSTFN + r"\missing") + ABSTFN + r"\self\self\missing") self.assertPathEqual(ntpath.realpath("broken3"), - ABSTFN + r"\missing") + ABSTFN + r"\subdir\parent\subdir\parent\missing") self.assertPathEqual(ntpath.realpath("broken4"), ABSTFN + r"\missing") self.assertPathEqual(ntpath.realpath("broken5"), @@ -304,13 +306,13 @@ class TestNtpath(NtpathTestCase): self.assertPathEqual(ntpath.realpath(rb"broken\foo"), os.fsencode(ABSTFN + r"\missing\foo")) self.assertPathEqual(ntpath.realpath(rb"broken1"), - os.fsencode(ABSTFN + r"\missing\bar")) + os.fsencode(ABSTFN + r"\broken\bar")) self.assertPathEqual(ntpath.realpath(rb"broken1\baz"), - os.fsencode(ABSTFN + r"\missing\bar\baz")) + os.fsencode(ABSTFN + r"\broken\bar\baz")) self.assertPathEqual(ntpath.realpath(b"broken2"), - os.fsencode(ABSTFN + r"\missing")) + os.fsencode(ABSTFN + r"\self\self\missing")) self.assertPathEqual(ntpath.realpath(rb"broken3"), - os.fsencode(ABSTFN + r"\missing")) + os.fsencode(ABSTFN + r"\subdir\parent\subdir\parent\missing")) self.assertPathEqual(ntpath.realpath(b"broken4"), os.fsencode(ABSTFN + r"\missing")) self.assertPathEqual(ntpath.realpath(b"broken5"), @@ -319,8 +321,8 @@ class TestNtpath(NtpathTestCase): @support.skip_unless_symlink @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') def test_realpath_symlink_loops(self): - # Bug #930024, return the path unchanged if we get into an infinite - # symlink loop. + # 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 ABSTFN = ntpath.abspath(support.TESTFN) self.addCleanup(support.unlink, ABSTFN) self.addCleanup(support.unlink, ABSTFN + "1") @@ -332,8 +334,6 @@ class TestNtpath(NtpathTestCase): os.symlink(ABSTFN, ABSTFN) self.assertPathEqual(ntpath.realpath(ABSTFN), ABSTFN) - # cycles are non-deterministic as to which path is returned, but - # it will always be the fully resolved path of one member of the cycle os.symlink(ABSTFN + "1", ABSTFN + "2") os.symlink(ABSTFN + "2", ABSTFN + "1") expected = (ABSTFN + "1", ABSTFN + "2") @@ -402,6 +402,34 @@ class TestNtpath(NtpathTestCase): def test_realpath_nul(self): tester("ntpath.realpath('NUL')", r'\\.\NUL') + @unittest.skipUnless(HAVE_GETFINALPATHNAME, 'need _getfinalpathname') + def test_realpath_cwd(self): + ABSTFN = ntpath.abspath(support.TESTFN) + + support.unlink(ABSTFN) + support.rmtree(ABSTFN) + os.mkdir(ABSTFN) + self.addCleanup(support.rmtree, ABSTFN) + + test_dir_long = ntpath.join(ABSTFN, "MyVeryLongDirectoryName") + test_dir_short = ntpath.join(ABSTFN, "MYVERY~1") + test_file_long = ntpath.join(test_dir_long, "file.txt") + test_file_short = ntpath.join(test_dir_short, "file.txt") + + os.mkdir(test_dir_long) + + with open(test_file_long, "wb") as f: + f.write(b"content") + + self.assertPathEqual(test_file_long, ntpath.realpath(test_file_short)) + + with support.change_cwd(test_dir_long): + self.assertPathEqual(test_file_long, ntpath.realpath("file.txt")) + with support.change_cwd(test_dir_long.lower()): + self.assertPathEqual(test_file_long, ntpath.realpath("file.txt")) + with support.change_cwd(test_dir_short): + self.assertPathEqual(test_file_long, ntpath.realpath("file.txt")) + def test_expandvars(self): with support.EnvironmentVarGuard() as env: env.clear() diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 636e3bd9795..b98e7dc798a 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -177,7 +177,10 @@ class TestShutil(unittest.TestCase): Returns the path of the directory. """ - d = tempfile.mkdtemp() + basedir = None + if sys.platform == "win32": + basedir = os.path.realpath(os.getcwd()) + d = tempfile.mkdtemp(dir=basedir) self.tempdirs.append(d) return d @@ -1788,8 +1791,11 @@ class TestMove(unittest.TestCase): def setUp(self): filename = "foo" - self.src_dir = tempfile.mkdtemp() - self.dst_dir = tempfile.mkdtemp() + basedir = None + if sys.platform == "win32": + basedir = os.path.realpath(os.getcwd()) + self.src_dir = tempfile.mkdtemp(dir=basedir) + self.dst_dir = tempfile.mkdtemp(dir=basedir) self.src_file = os.path.join(self.src_dir, filename) self.dst_file = os.path.join(self.dst_dir, filename) with open(self.src_file, "wb") as f: diff --git a/Misc/NEWS.d/next/Windows/2019-10-28-10-32-43.bpo-38453.NwwatW.rst b/Misc/NEWS.d/next/Windows/2019-10-28-10-32-43.bpo-38453.NwwatW.rst new file mode 100644 index 00000000000..deacb03c6f0 --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2019-10-28-10-32-43.bpo-38453.NwwatW.rst @@ -0,0 +1 @@ +Ensure ntpath.realpath() correctly resolves relative paths.