From 3a22d932de5ecbc35a2f751e502deb1a169d4079 Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Sun, 19 May 2024 11:38:52 +0200 Subject: [PATCH 01/12] Limit `posixpath.realpath(..., strict=True)` symlinks --- Lib/posixpath.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index c04c628de55ee2..8518337636b00f 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -449,6 +449,9 @@ def realpath(filename, *, strict=False): # the same links. seen = {} + # How many symlinks can still be read (excluding caching) + remaining_symlinks = 40 # TODO: use limit set by OS + while rest: name = rest.pop() if name is None: @@ -483,7 +486,10 @@ def realpath(filename, *, strict=False): os.stat(newpath) path = newpath continue + if remaining_symlinks <= 0 and strict: + raise OSError(errno.ELOOP, "Too many symbolic links", newpath) target = os.readlink(newpath) + remaining_symlinks -= 1 except OSError: if strict: raise From dc6f7d3aaec00722015af29eeb5e84e283d0d980 Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Sun, 19 May 2024 12:08:39 +0200 Subject: [PATCH 02/12] Add test --- Lib/test/test_posixpath.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 5c27b7bee8f60e..d87954e158bfab 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -677,6 +677,29 @@ def test_realpath_unreadable_symlink(self): os.chmod(ABSTFN, 0o755, follow_symlinks=False) os.unlink(ABSTFN) + @os_helper.skip_unless_symlink + @skip_if_ABSTFN_contains_backslash + def test_realpath_too_many_symlinks(self): + depth = 40 + 1 # TODO: use limit set by OS + 1 + try: + os.mkdir(ABSTFN) + os.symlink('.', f'{ABSTFN}/1') + for i in range(1, depth): + os.symlink(f'{i}'), f'{ABSTFN}/{i + 1}') + self.assertEqual(realpath(f'{ABSTFN}/{depth}'), ABSTFN) + with self.assertRaises(OSError): + realpath(f'{ABSTFN}/{depth}', strict=True) + + # Test using relative path as well. + with os_helper.change_cwd(ABSTFN): + self.assertEqual(realpath(f'{depth}'), ABSTFN) + with self.assertRaises(OSError): + realpath(f'{depth}', strict=True) + finally: + for i in range(1, depth + 1): + os_helper.unlink(f'{ABSTFN}/{i}') + safe_rmdir(ABSTFN) + def test_relpath(self): (real_getcwd, os.getcwd) = (os.getcwd, lambda: r"/home/user/bar") try: From 2605bfcf08e370ccd872a62a1a6f89db270c4391 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sun, 19 May 2024 10:12:59 +0000 Subject: [PATCH 03/12] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst new file mode 100644 index 00000000000000..690ddd3d54aca5 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst @@ -0,0 +1 @@ +:func:`os.path.realpath` now raises :exc:`OSError` when *strict* mode is enabled and a path with too many symlinks is supplied. From d9475bfdb5444225b6cd42f579e94d8c691be23d Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Sun, 19 May 2024 12:22:30 +0200 Subject: [PATCH 04/12] Fix syntax error --- Lib/test/test_posixpath.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index d87954e158bfab..5fe0eb777e371a 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -685,7 +685,7 @@ def test_realpath_too_many_symlinks(self): os.mkdir(ABSTFN) os.symlink('.', f'{ABSTFN}/1') for i in range(1, depth): - os.symlink(f'{i}'), f'{ABSTFN}/{i + 1}') + os.symlink(f'{i}', f'{ABSTFN}/{i + 1}') self.assertEqual(realpath(f'{ABSTFN}/{depth}'), ABSTFN) with self.assertRaises(OSError): realpath(f'{ABSTFN}/{depth}', strict=True) From b18e5d46d85225533e4e5dc1388de539c521f105 Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Sun, 19 May 2024 12:32:35 +0200 Subject: [PATCH 05/12] import `errno` --- Lib/posixpath.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 8518337636b00f..b02c19e8059dce 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 @@ -482,8 +483,7 @@ def realpath(filename, *, strict=False): continue # The symlink is not resolved, so we must have a symlink loop. if strict: - # Raise OSError(errno.ELOOP) - os.stat(newpath) + raise OSError(errno.ELOOP, "Symlink loop", newpath) path = newpath continue if remaining_symlinks <= 0 and strict: From 628acd3116bed7e1cd90691f84628ec74f35934e Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Sun, 19 May 2024 18:45:18 +0200 Subject: [PATCH 06/12] Use `len(seen)` --- Lib/posixpath.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index b02c19e8059dce..3bfea753517fef 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -450,8 +450,8 @@ def realpath(filename, *, strict=False): # the same links. seen = {} - # How many symlinks can still be read (excluding caching) - remaining_symlinks = 40 # TODO: use limit set by OS + # How many unique symlinks can be read + maxlinks = 40 # TODO: use limit set by OS while rest: name = rest.pop() @@ -486,10 +486,9 @@ def realpath(filename, *, strict=False): raise OSError(errno.ELOOP, "Symlink loop", newpath) path = newpath continue - if remaining_symlinks <= 0 and strict: + if strict and maxlinks != -1 and len(seen) >= maxlinks: raise OSError(errno.ELOOP, "Too many symbolic links", newpath) target = os.readlink(newpath) - remaining_symlinks -= 1 except OSError: if strict: raise From 3ada54690c0ff4b9073b8757fe4e9c3ad1bb2e01 Mon Sep 17 00:00:00 2001 From: Nineteendo Date: Wed, 5 Jun 2024 20:17:52 +0200 Subject: [PATCH 07/12] Restrict `maxlinks` when strict is set --- Lib/posixpath.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index fccca4e066b76f..728391e1bf9f80 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -402,7 +402,9 @@ def realpath(filename, *, strict=False): curdir = '.' pardir = '..' getcwd = os.getcwd - return _realpath(filename, strict, sep, curdir, pardir, getcwd) + maxlinks = 40 if strict else None + return _realpath(filename, strict, sep, curdir, pardir, getcwd, + maxlinks=maxlinks) def _realpath(filename, strict=False, sep=sep, curdir=curdir, pardir=pardir, getcwd=os.getcwd, lstat=os.lstat, readlink=os.readlink, maxlinks=None): From e502666ec22959dff6b5b15b95ba219935ebf831 Mon Sep 17 00:00:00 2001 From: Nineteendo Date: Wed, 5 Jun 2024 20:57:13 +0200 Subject: [PATCH 08/12] Simplify test --- Lib/posixpath.py | 2 +- Lib/test/test_posixpath.py | 23 +++++++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 728391e1bf9f80..02cf440432309d 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -402,7 +402,7 @@ def realpath(filename, *, strict=False): curdir = '.' pardir = '..' getcwd = os.getcwd - maxlinks = 40 if strict else None + maxlinks = 40 if strict else None # TODO: use limit set by OS return _realpath(filename, strict, sep, curdir, pardir, getcwd, maxlinks=maxlinks) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index b4249339f436ac..6989351bb64059 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -692,24 +692,27 @@ def test_realpath_unreadable_symlink(self): @os_helper.skip_unless_symlink @skip_if_ABSTFN_contains_backslash def test_realpath_too_many_symlinks(self): - depth = 40 + 1 # TODO: use limit set by OS + 1 + maxlinks = 40 # TODO: use limit set by OS try: os.mkdir(ABSTFN) - os.symlink('.', f'{ABSTFN}/1') - for i in range(1, depth): - os.symlink(f'{i}', f'{ABSTFN}/{i + 1}') - self.assertEqual(realpath(f'{ABSTFN}/{depth}'), ABSTFN) + os.symlink('.', f'{ABSTFN}/link') + self.assertEqual(realpath(ABSTFN + '/link' * maxlinks), ABSTFN) + self.assertEqual(realpath(ABSTFN + '/link' * maxlinks, + strict=True), ABSTFN) + self.assertEqual(realpath(ABSTFN + '/link' * (maxlinks+1)), ABSTFN) with self.assertRaises(OSError): - realpath(f'{ABSTFN}/{depth}', strict=True) + realpath(ABSTFN + '/link' * (maxlinks+1), strict=True) # Test using relative path as well. with os_helper.change_cwd(ABSTFN): - self.assertEqual(realpath(f'{depth}'), ABSTFN) + self.assertEqual(realpath('link/' * maxlinks), ABSTFN) + self.assertEqual(realpath('link/' * maxlinks, strict=True), + ABSTFN) + self.assertEqual(realpath('link/' * (maxlinks+1)), ABSTFN) with self.assertRaises(OSError): - realpath(f'{depth}', strict=True) + realpath('link/' * (maxlinks+1), strict=True) finally: - for i in range(1, depth + 1): - os_helper.unlink(f'{ABSTFN}/{i}') + os_helper.unlink(f'{ABSTFN}/link') safe_rmdir(ABSTFN) def test_relpath(self): From 06c3853ec2285ecd5712c15a1cca170088ecad11 Mon Sep 17 00:00:00 2001 From: Nineteendo Date: Wed, 5 Jun 2024 22:07:24 +0200 Subject: [PATCH 09/12] Fix error message --- Lib/posixpath.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 02cf440432309d..58a3d7ee29decb 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -455,7 +455,7 @@ def _realpath(filename, strict=False, sep=sep, curdir=curdir, pardir=pardir, if link_count > maxlinks: if strict: raise OSError(errno.ELOOP, os.strerror(errno.ELOOP), - newpath) + filename) path = newpath continue elif newpath in seen: @@ -467,7 +467,7 @@ def _realpath(filename, strict=False, sep=sep, curdir=curdir, pardir=pardir, # The symlink is not resolved, so we must have a symlink loop. if strict: raise OSError(errno.ELOOP, os.strerror(errno.ELOOP), - newpath) + filename) path = newpath continue target = readlink(newpath) From c3c0979fcb10d991df379d16a2f1af613079a5e6 Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Wed, 19 Jun 2024 13:17:00 +0200 Subject: [PATCH 10/12] Rename 2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst to 2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst --- .../2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Misc/NEWS.d/next/{Core and Builtins => Library}/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst (100%) diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst b/Misc/NEWS.d/next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst similarity index 100% rename from Misc/NEWS.d/next/Core and Builtins/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst rename to Misc/NEWS.d/next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst From f34484e21d0b39f4b2cb250e6b1de70c6dc4f7f4 Mon Sep 17 00:00:00 2001 From: Nice Zombies Date: Fri, 21 Jun 2024 07:35:03 +0200 Subject: [PATCH 11/12] Update 2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst --- .../next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/Misc/NEWS.d/next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst b/Misc/NEWS.d/next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst index 690ddd3d54aca5..00025722d8c03d 100644 --- a/Misc/NEWS.d/next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst +++ b/Misc/NEWS.d/next/Library/2024-05-19-10-12-58.gh-issue-118441.QpzMKV.rst @@ -1 +1,2 @@ +Fix error message for :func:`os.path.realpath` on Unix. :func:`os.path.realpath` now raises :exc:`OSError` when *strict* mode is enabled and a path with too many symlinks is supplied. From dff6700a39925ab06413200b4b0729fd8897fe70 Mon Sep 17 00:00:00 2001 From: Nineteendo Date: Fri, 6 Sep 2024 13:39:46 +0200 Subject: [PATCH 12/12] Add private constant --- Lib/posixpath.py | 5 +++-- Lib/test/test_posixpath.py | 19 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 58a3d7ee29decb..6175ed81948abc 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -388,6 +388,8 @@ def abspath(path): # Return a canonical path (i.e. the absolute location of a file on the # filesystem). +_MAXLINKS = 40 # TODO: use limit set by OS + def realpath(filename, *, strict=False): """Return the canonical path of the specified filename, eliminating any symbolic links encountered in the path.""" @@ -402,9 +404,8 @@ def realpath(filename, *, strict=False): curdir = '.' pardir = '..' getcwd = os.getcwd - maxlinks = 40 if strict else None # TODO: use limit set by OS return _realpath(filename, strict, sep, curdir, pardir, getcwd, - maxlinks=maxlinks) + maxlinks=_MAXLINKS if strict else None) def _realpath(filename, strict=False, sep=sep, curdir=curdir, pardir=pardir, getcwd=os.getcwd, lstat=os.lstat, readlink=os.readlink, maxlinks=None): diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 6989351bb64059..919f158d7e6465 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -3,7 +3,7 @@ import posixpath import sys import unittest -from posixpath import realpath, abspath, dirname, basename +from posixpath import _MAXLINKS, realpath, abspath, dirname, basename from test import test_genericpath from test.support import import_helper from test.support import cpython_only, os_helper @@ -692,25 +692,24 @@ def test_realpath_unreadable_symlink(self): @os_helper.skip_unless_symlink @skip_if_ABSTFN_contains_backslash def test_realpath_too_many_symlinks(self): - maxlinks = 40 # TODO: use limit set by OS try: os.mkdir(ABSTFN) os.symlink('.', f'{ABSTFN}/link') - self.assertEqual(realpath(ABSTFN + '/link' * maxlinks), ABSTFN) - self.assertEqual(realpath(ABSTFN + '/link' * maxlinks, + self.assertEqual(realpath(ABSTFN + '/link' * _MAXLINKS), ABSTFN) + self.assertEqual(realpath(ABSTFN + '/link' * _MAXLINKS, strict=True), ABSTFN) - self.assertEqual(realpath(ABSTFN + '/link' * (maxlinks+1)), ABSTFN) + self.assertEqual(realpath(ABSTFN + '/link' * (_MAXLINKS+1)), ABSTFN) with self.assertRaises(OSError): - realpath(ABSTFN + '/link' * (maxlinks+1), strict=True) + realpath(ABSTFN + '/link' * (_MAXLINKS+1), strict=True) # Test using relative path as well. with os_helper.change_cwd(ABSTFN): - self.assertEqual(realpath('link/' * maxlinks), ABSTFN) - self.assertEqual(realpath('link/' * maxlinks, strict=True), + self.assertEqual(realpath('link/' * _MAXLINKS), ABSTFN) + self.assertEqual(realpath('link/' * _MAXLINKS, strict=True), ABSTFN) - self.assertEqual(realpath('link/' * (maxlinks+1)), ABSTFN) + self.assertEqual(realpath('link/' * (_MAXLINKS+1)), ABSTFN) with self.assertRaises(OSError): - realpath('link/' * (maxlinks+1), strict=True) + realpath('link/' * (_MAXLINKS+1), strict=True) finally: os_helper.unlink(f'{ABSTFN}/link') safe_rmdir(ABSTFN)