From 3d64ecc9399c248bc161e62c27df51ddcae012e8 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Thu, 24 Apr 2025 23:56:04 +1200 Subject: [PATCH 1/4] gh-89708: use fds when possible in contextlib.chdir This fixes two failure modes: original directories that are longer than PATH_MAX or that were deleted. Use this safer mode when possible, falling back to the existing mode if fds cannot be used. --- Lib/contextlib.py | 34 ++++++++++++++++++++-- Lib/test/test_contextlib.py | 57 +++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/Lib/contextlib.py b/Lib/contextlib.py index 5b646fabca0225..2ac1a9808e37cf 100644 --- a/Lib/contextlib.py +++ b/Lib/contextlib.py @@ -805,10 +805,38 @@ class chdir(AbstractContextManager): def __init__(self, path): self.path = path self._old_cwd = [] + if self._supports_fd(): + self._getcwd = self._getcwd_fd + self._close = os.close + + @staticmethod + def _supports_fd(): + return os.chdir in os.supports_fd + + @staticmethod + def _getcwd(): + return os.getcwd() + + @staticmethod + def _close(dir): + pass + + @staticmethod + def _getcwd_fd(): + return os.open('.', os.O_RDONLY) def __enter__(self): - self._old_cwd.append(os.getcwd()) - os.chdir(self.path) + dir = self._getcwd() + try: + os.chdir(self.path) + except: + self._close(dir) + raise + self._old_cwd.append(dir) def __exit__(self, *excinfo): - os.chdir(self._old_cwd.pop()) + dir = self._old_cwd.pop() + try: + os.chdir(dir) + finally: + self._close(dir) diff --git a/Lib/test/test_contextlib.py b/Lib/test/test_contextlib.py index cf6519598037e9..2d50d45fd55443 100644 --- a/Lib/test/test_contextlib.py +++ b/Lib/test/test_contextlib.py @@ -1361,6 +1361,63 @@ def test_exception(self): self.assertEqual(str(re), "boom") self.assertEqual(os.getcwd(), old_cwd) + def test_failed_chdir(self): + old_cwd = os.getcwd() + target = self.make_relative_path('non_existent_directory') + self.assertNotEqual(old_cwd, target) + ctx = chdir(target) + with self.assertRaises(OSError): + with ctx: + self.fail("chdir should have raised an exception") + self.assertFalse(ctx._old_cwd) + self.assertEqual(os.getcwd(), old_cwd) + + @unittest.skipUnless(chdir._supports_fd(), "chdir requires fd support") + def test_chdir_to_fd(self): + old_cwd = os.getcwd() + target = self.make_relative_path('data') + fd = os.open(target, os.O_RDONLY) + try: + with chdir(fd): + self.assertEqual(os.getcwd(), target) + finally: + os.close(fd) + self.assertEqual(os.getcwd(), old_cwd) + + @unittest.skipUnless(chdir._supports_fd(), + "chdir requires fd support for long path") + @unittest.skipUnless(os_helper.can_symlink(), "need symlink support") + def test_original_path_too_long(self): + with tempfile.TemporaryDirectory() as dir: + with chdir(dir): + count = 0 + big_name = 'a' * os.pathconf('.', 'PC_NAME_MAX') + path_max = os.pathconf('.', 'PC_PATH_MAX') + while len(dir) < path_max + 1: + dir = os.path.join(dir, big_name) + os.mkdir(big_name) + os.symlink(big_name, 'a', target_is_directory=True) + os.chdir('a') + count += 1 + + self.assertTrue(count > 0) + os.mkdir('a') + with chdir('a'): + self.assertTrue(len(os.getcwd()) > path_max) + + @unittest.skipUnless(chdir._supports_fd(), + "chdir requires fd support for deleted dir") + def test_original_path_deleted(self): + original = os.getcwd() + dir = tempfile.TemporaryDirectory() + try: + os.chdir(dir.name) + with chdir(original): + dir.cleanup() + finally: + os.chdir(original) + dir.cleanup() + if __name__ == "__main__": unittest.main() From c46d777e222c0417685adc42c4d2ab07d96025d8 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sat, 26 Apr 2025 12:10:36 +1200 Subject: [PATCH 2/4] Add blurb --- .../next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst diff --git a/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst b/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst new file mode 100644 index 00000000000000..69e7b5d092b617 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst @@ -0,0 +1,3 @@ +Use fds in contextlib.chdir where supported. This fixes bugs if the original +working directory is greater than PATH_MAX and/or is deleted before the +context manager exits. From 5879e4417d782838566dd523dbdccc0cc6d9fef9 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 27 Apr 2025 11:17:03 +1200 Subject: [PATCH 3/4] Address reviewer feedback --- Lib/test/test_contextlib.py | 6 +++--- .../Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_contextlib.py b/Lib/test/test_contextlib.py index 2d50d45fd55443..aa4b4fc32d21e7 100644 --- a/Lib/test/test_contextlib.py +++ b/Lib/test/test_contextlib.py @@ -1400,23 +1400,23 @@ def test_original_path_too_long(self): os.chdir('a') count += 1 - self.assertTrue(count > 0) + self.assertGreater(count, 0) os.mkdir('a') with chdir('a'): - self.assertTrue(len(os.getcwd()) > path_max) + self.assertGreater(len(os.getcwd()), path_max) @unittest.skipUnless(chdir._supports_fd(), "chdir requires fd support for deleted dir") def test_original_path_deleted(self): original = os.getcwd() dir = tempfile.TemporaryDirectory() + self.addCleanup(dir.cleanup) try: os.chdir(dir.name) with chdir(original): dir.cleanup() finally: os.chdir(original) - dir.cleanup() if __name__ == "__main__": diff --git a/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst b/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst index 69e7b5d092b617..2189490d694852 100644 --- a/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst +++ b/Misc/NEWS.d/next/Library/2025-04-26-12-10-32.gh-issue-89708.haEUh3.rst @@ -1,3 +1,3 @@ -Use fds in contextlib.chdir where supported. This fixes bugs if the original -working directory is greater than PATH_MAX and/or is deleted before the -context manager exits. +Use file descriptors in :func:`contextlib.chdir` when possible to prevent +issues when the original working directory is concurrently deleted or when its +path length is longer than the maximum allowed by the host platform. From 046b2d97ebd6d3315a87b1fcc04a24fc07a17f2d Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 27 Apr 2025 11:20:06 +1200 Subject: [PATCH 4/4] More review feedback, thanks! --- Lib/test/test_contextlib.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Lib/test/test_contextlib.py b/Lib/test/test_contextlib.py index aa4b4fc32d21e7..34e009dbcccb56 100644 --- a/Lib/test/test_contextlib.py +++ b/Lib/test/test_contextlib.py @@ -1366,9 +1366,7 @@ def test_failed_chdir(self): target = self.make_relative_path('non_existent_directory') self.assertNotEqual(old_cwd, target) ctx = chdir(target) - with self.assertRaises(OSError): - with ctx: - self.fail("chdir should have raised an exception") + self.assertRaises(OSError, ctx.__enter__) self.assertFalse(ctx._old_cwd) self.assertEqual(os.getcwd(), old_cwd)