From 0049b2255a4781f21123804d56aaf2e62739c476 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 15 Nov 2019 09:49:21 -0800 Subject: [PATCH 1/4] 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 | 22 ++++---- .../2019-10-28-10-32-43.bpo-38453.NwwatW.rst | 1 + 4 files changed, 93 insertions(+), 31 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 d4ecff97c95286a..6f771773a7d1b5d 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 @@ def abspath(path): # 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 @@ def _readlink_deep(path, seen=None): # 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 @@ def _getfinalpathname_nonstrict(path): # 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 @@ def realpath(path): 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 e0ec441985230ad..a84b94c9badbeb4 100644 --- a/Lib/test/test_ntpath.py +++ b/Lib/test/test_ntpath.py @@ -286,14 +286,16 @@ def test_realpath_broken_symlinks(self): 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 @@ def test_realpath_broken_symlinks(self): 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 @@ def test_realpath_broken_symlinks(self): @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 @@ def test_realpath_symlink_loops(self): 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 @@ def test_realpath_symlink_prefix(self): 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 636e3bd9795593f..39573c8e7928a16 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -123,12 +123,12 @@ def supports_file2file_sendfile(): srcname = None dstname = None try: - with tempfile.NamedTemporaryFile("wb", delete=False) as f: + with tempfile.NamedTemporaryFile("wb", dir=os.getcwd(), delete=False) as f: srcname = f.name f.write(b"0123456789") with open(srcname, "rb") as src: - with tempfile.NamedTemporaryFile("wb", delete=False) as dst: + with tempfile.NamedTemporaryFile("wb", dir=os.getcwd(), delete=False) as dst: dstname = dst.name infd = src.fileno() outfd = dst.fileno() @@ -172,12 +172,12 @@ def tearDown(self): shutil.rmtree(d, os.name in ('nt', 'cygwin')) - def mkdtemp(self): + def mkdtemp(self, prefix=None): """Create a temporary directory that will be cleaned up. Returns the path of the directory. """ - d = tempfile.mkdtemp() + d = tempfile.mkdtemp(prefix=prefix, dir=os.getcwd()) self.tempdirs.append(d) return d @@ -238,6 +238,7 @@ def test_rmtree_fails_on_junctions(self): os.mkdir(dir_) link = os.path.join(tmp, 'link') _winapi.CreateJunction(dir_, link) + self.addCleanup(support.unlink, link) self.assertRaises(OSError, shutil.rmtree, link) self.assertTrue(os.path.exists(dir_)) self.assertTrue(os.path.lexists(link)) @@ -274,7 +275,7 @@ def test_rmtree_works_on_junctions(self): def test_rmtree_errors(self): # filename is guaranteed not to exist - filename = tempfile.mktemp() + filename = tempfile.mktemp(dir=self.mkdtemp()) self.assertRaises(FileNotFoundError, shutil.rmtree, filename) # test that ignore_errors option is honored shutil.rmtree(filename, ignore_errors=True) @@ -1606,8 +1607,7 @@ def test_copytree_return_value(self): class TestWhich(unittest.TestCase): def setUp(self): - self.temp_dir = tempfile.mkdtemp(prefix="Tmp") - self.addCleanup(shutil.rmtree, self.temp_dir, True) + self.temp_dir = self.mkdtemp(prefix="Tmp") # Give the temp_file an ".exe" suffix for all. # It's needed on Windows and not harmful on other platforms. self.temp_file = tempfile.NamedTemporaryFile(dir=self.temp_dir, @@ -1837,7 +1837,7 @@ def test_move_file_to_dir_other_fs(self): def test_move_dir(self): # Move a dir to another location on the same filesystem. - dst_dir = tempfile.mktemp() + dst_dir = tempfile.mktemp(dir=self.mkdtemp()) try: self._check_move_dir(self.src_dir, dst_dir, dst_dir) finally: @@ -2155,7 +2155,7 @@ def test_win_impl(self): # If file size < 1 MiB memoryview() length must be equal to # the actual file size. - with tempfile.NamedTemporaryFile(delete=False) as f: + with tempfile.NamedTemporaryFile(dir=os.getcwd(), delete=False) as f: f.write(b'foo') fname = f.name self.addCleanup(support.unlink, fname) @@ -2164,7 +2164,7 @@ def test_win_impl(self): self.assertEqual(m.call_args[0][2], 3) # Empty files should not rely on readinto() variant. - with tempfile.NamedTemporaryFile(delete=False) as f: + with tempfile.NamedTemporaryFile(dir=os.getcwd(), delete=False) as f: pass fname = f.name self.addCleanup(support.unlink, fname) @@ -2230,7 +2230,7 @@ def test_same_file(self): self.assertEqual(read_file(TESTFN, binary=True), self.FILEDATA) def test_non_existent_src(self): - name = tempfile.mktemp() + name = tempfile.mktemp(dir=os.getcwd()) with self.assertRaises(FileNotFoundError) as cm: shutil.copyfile(name, "new") self.assertEqual(cm.exception.filename, name) 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 000000000000000..deacb03c6f01dbc --- /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. From df052b18c15bb11b664058b26e5bab29b0bfcb01 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 15 Nov 2019 11:36:20 -0800 Subject: [PATCH 2/4] Revert shutil test changes for backport --- Lib/test/test_shutil.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 39573c8e7928a16..636e3bd9795593f 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -123,12 +123,12 @@ def supports_file2file_sendfile(): srcname = None dstname = None try: - with tempfile.NamedTemporaryFile("wb", dir=os.getcwd(), delete=False) as f: + with tempfile.NamedTemporaryFile("wb", delete=False) as f: srcname = f.name f.write(b"0123456789") with open(srcname, "rb") as src: - with tempfile.NamedTemporaryFile("wb", dir=os.getcwd(), delete=False) as dst: + with tempfile.NamedTemporaryFile("wb", delete=False) as dst: dstname = dst.name infd = src.fileno() outfd = dst.fileno() @@ -172,12 +172,12 @@ def tearDown(self): shutil.rmtree(d, os.name in ('nt', 'cygwin')) - def mkdtemp(self, prefix=None): + def mkdtemp(self): """Create a temporary directory that will be cleaned up. Returns the path of the directory. """ - d = tempfile.mkdtemp(prefix=prefix, dir=os.getcwd()) + d = tempfile.mkdtemp() self.tempdirs.append(d) return d @@ -238,7 +238,6 @@ def test_rmtree_fails_on_junctions(self): os.mkdir(dir_) link = os.path.join(tmp, 'link') _winapi.CreateJunction(dir_, link) - self.addCleanup(support.unlink, link) self.assertRaises(OSError, shutil.rmtree, link) self.assertTrue(os.path.exists(dir_)) self.assertTrue(os.path.lexists(link)) @@ -275,7 +274,7 @@ def test_rmtree_works_on_junctions(self): def test_rmtree_errors(self): # filename is guaranteed not to exist - filename = tempfile.mktemp(dir=self.mkdtemp()) + filename = tempfile.mktemp() self.assertRaises(FileNotFoundError, shutil.rmtree, filename) # test that ignore_errors option is honored shutil.rmtree(filename, ignore_errors=True) @@ -1607,7 +1606,8 @@ def test_copytree_return_value(self): class TestWhich(unittest.TestCase): def setUp(self): - self.temp_dir = self.mkdtemp(prefix="Tmp") + self.temp_dir = tempfile.mkdtemp(prefix="Tmp") + self.addCleanup(shutil.rmtree, self.temp_dir, True) # Give the temp_file an ".exe" suffix for all. # It's needed on Windows and not harmful on other platforms. self.temp_file = tempfile.NamedTemporaryFile(dir=self.temp_dir, @@ -1837,7 +1837,7 @@ def test_move_file_to_dir_other_fs(self): def test_move_dir(self): # Move a dir to another location on the same filesystem. - dst_dir = tempfile.mktemp(dir=self.mkdtemp()) + dst_dir = tempfile.mktemp() try: self._check_move_dir(self.src_dir, dst_dir, dst_dir) finally: @@ -2155,7 +2155,7 @@ def test_win_impl(self): # If file size < 1 MiB memoryview() length must be equal to # the actual file size. - with tempfile.NamedTemporaryFile(dir=os.getcwd(), delete=False) as f: + with tempfile.NamedTemporaryFile(delete=False) as f: f.write(b'foo') fname = f.name self.addCleanup(support.unlink, fname) @@ -2164,7 +2164,7 @@ def test_win_impl(self): self.assertEqual(m.call_args[0][2], 3) # Empty files should not rely on readinto() variant. - with tempfile.NamedTemporaryFile(dir=os.getcwd(), delete=False) as f: + with tempfile.NamedTemporaryFile(delete=False) as f: pass fname = f.name self.addCleanup(support.unlink, fname) @@ -2230,7 +2230,7 @@ def test_same_file(self): self.assertEqual(read_file(TESTFN, binary=True), self.FILEDATA) def test_non_existent_src(self): - name = tempfile.mktemp(dir=os.getcwd()) + name = tempfile.mktemp() with self.assertRaises(FileNotFoundError) as cm: shutil.copyfile(name, "new") self.assertEqual(cm.exception.filename, name) From 2a44825aab6d001f5ae48db3c69cd90e42f4b861 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 15 Nov 2019 14:14:02 -0800 Subject: [PATCH 3/4] Move temp directories under working directory --- Lib/test/test_shutil.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 636e3bd9795593f..3ce66c1db5f576c 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -1788,8 +1788,8 @@ class TestMove(unittest.TestCase): def setUp(self): filename = "foo" - self.src_dir = tempfile.mkdtemp() - self.dst_dir = tempfile.mkdtemp() + self.src_dir = tempfile.mkdtemp(dir=".") + self.dst_dir = tempfile.mkdtemp(dir=".") 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: From 589288f3d664d881ae11ac7cc12306aff70bfd6f Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 15 Nov 2019 15:06:59 -0800 Subject: [PATCH 4/4] Try alternate fix for shutil test --- Lib/test/test_shutil.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 3ce66c1db5f576c..b98e7dc798abe79 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -177,7 +177,10 @@ def mkdtemp(self): 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(dir=".") - self.dst_dir = tempfile.mkdtemp(dir=".") + 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: