From 6a4be8f43a02608acb4d52ccc4883ad14ee67e2a Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Tue, 5 Sep 2023 16:46:56 +0000 Subject: [PATCH 01/21] tarfile: fix sticky bit handling in FreeBSD --- Lib/tarfile.py | 8 ++++++++ Lib/test/test_tarfile.py | 11 ++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 726f9f50ba2e72..40885336806b31 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -37,6 +37,7 @@ # Imports #--------- from builtins import open as bltn_open +import errno import sys import os import io @@ -2569,6 +2570,13 @@ def chmod(self, tarinfo, targetpath): try: os.chmod(targetpath, tarinfo.mode) except OSError as e: + if hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE: + # On FreeBSD, chmod fails when trying to set the sticky bit on + # a file as a normal user. It's a noop in most other platforms. + try: + os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) + except OSError as e: + raise ExtractError("could not change mode") from e raise ExtractError("could not change mode") from e def utime(self, tarinfo, targetpath): diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 67009a3d2e9c24..e2534b94701359 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -9,6 +9,7 @@ import re import warnings import stat +import errno import unittest import unittest.mock @@ -3818,7 +3819,15 @@ def test_modes(self): pass new_mode = (os.stat(tmp_filename).st_mode | stat.S_ISVTX | stat.S_ISGID | stat.S_ISUID) - os.chmod(tmp_filename, new_mode) + try: + os.chmod(tmp_filename, new_mode) + except OSError as err: + # FreeBSD fails with EFTYPE if sticky bit cannot be set, instead + # of ignoring it. + if hasattr(errno, "EFTYPE") and err.errno == errno.EFTYPE: + os.chmod(tmp_filename, new_mode & ~stat.S_ISVTX) + else: + raise got_mode = os.stat(tmp_filename).st_mode _t_file = 't' if (got_mode & stat.S_ISVTX) else 'x' _suid_file = 's' if (got_mode & stat.S_ISUID) else 'x' From 50729f7088872a536ac6571bdedfad0b5425b322 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Tue, 5 Sep 2023 17:11:21 +0000 Subject: [PATCH 02/21] =?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 --- .../next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst diff --git a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst new file mode 100644 index 00000000000000..3421586afb621d --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst @@ -0,0 +1 @@ +mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. From 3587710b5b3676f65d2b00872270c4e6d8fe1476 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Tue, 5 Sep 2023 19:13:30 +0200 Subject: [PATCH 03/21] fix blurb --- .../next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst index 3421586afb621d..610892f20a984b 100644 --- a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst +++ b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst @@ -1 +1 @@ -mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. +On FreeBSD, mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. From 41491bbdb54f41f379cdc3cf0467341428dad100 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Tue, 5 Sep 2023 19:15:56 +0200 Subject: [PATCH 04/21] fix blurb --- .../next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst index 610892f20a984b..36bfcf6c41e6f9 100644 --- a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst +++ b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst @@ -1 +1 @@ -On FreeBSD, mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. +On FreeBSD, :mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. From 4f6536d4fd29a4e0c51eba0d91a5b31036c302ff Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Tue, 5 Sep 2023 20:47:50 +0000 Subject: [PATCH 05/21] fix chmod exception handler in tarfile --- Lib/tarfile.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 40885336806b31..736982d9528643 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -2577,7 +2577,8 @@ def chmod(self, tarinfo, targetpath): os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) except OSError as e: raise ExtractError("could not change mode") from e - raise ExtractError("could not change mode") from e + else: + raise ExtractError("could not change mode") from e def utime(self, tarinfo, targetpath): """Set modification time of targetpath according to tarinfo. From fe7dfedae65e4dfa968e260f1478ca0d52917179 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Tue, 5 Sep 2023 23:54:19 +0000 Subject: [PATCH 06/21] chain exceptions --- Lib/tarfile.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 736982d9528643..a5f54e92528474 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -2568,17 +2568,21 @@ def chmod(self, tarinfo, targetpath): if tarinfo.mode is None: return try: - os.chmod(targetpath, tarinfo.mode) - except OSError as e: - if hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE: - # On FreeBSD, chmod fails when trying to set the sticky bit on - # a file as a normal user. It's a noop in most other platforms. + try: + os.chmod(targetpath, tarinfo.mode) + except OSError as e: + if not(hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE): + raise + + # gh-108948: On FreeBSD, chmod() fails when trying to set the + # sticky bit on a file as non-root. But it's a noop in most + # other platforms, and we try and match that behavior here. try: os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) - except OSError as e: - raise ExtractError("could not change mode") from e - else: - raise ExtractError("could not change mode") from e + except OSError as e2: + raise e2 from e + except OSError as e: + raise ExtractError("could not change mode") from e def utime(self, tarinfo, targetpath): """Set modification time of targetpath according to tarinfo. From 400957365010039140bfb807b20077c93895324b Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Wed, 6 Sep 2023 07:57:26 +0000 Subject: [PATCH 07/21] test exception branches --- Lib/test/test_tarfile.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index e2534b94701359..7252b7d61e4679 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -691,6 +691,32 @@ def format_mtime(mtime): tar.close() os_helper.rmtree(DIR) + @unittest.skipUnless(hasattr(errno, "EFTYPE"), "errno.EFTYPE required") + def test_extract_chmod_eftype(self): + # Extracting a file as non-root should skip the sticky bit (gh-108948) + # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But + # we need to take care that any other error is preserved. + with ArchiveMaker() as arc: + arc.add("sticky", mode="?rw-rw-rwt") + tar = arc.open(errorlevel=2) + DIR = os.path.join(TEMPDIR, "chmod") + os.mkdir(DIR) + try: + # this should not raise: + tar.extract("sticky", DIR) + # but we can create a situation where it does raise: + with unittest.mock.patch("os.chmod") as mock: + eftype_error = OSError(errno.EFTYPE, "EFTYPE") + other_error = OSError(errno.EPERM, "different error") + mock.side_effect = [eftype_error, other_error] + with self.assertRaises(tarfile.ExtractError) as excinfo: + tar.extract("sticky", DIR) + self.assertEqual(excinfo.exception.__cause__, other_error) + self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) + finally: + os_helper.rmtree(DIR) + tar.close() + @os_helper.skip_unless_working_chmod def test_extract_directory(self): dirtype = "ustar/dirtype" From 7b784b5ce77ca2802d71fb00efdddd7ddcbe6b57 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Thu, 7 Sep 2023 13:05:01 +0200 Subject: [PATCH 08/21] Update Lib/test/test_tarfile.py Co-authored-by: Victor Stinner --- Lib/test/test_tarfile.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 7252b7d61e4679..483da1cb2daa88 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3848,8 +3848,8 @@ def test_modes(self): try: os.chmod(tmp_filename, new_mode) except OSError as err: - # FreeBSD fails with EFTYPE if sticky bit cannot be set, instead - # of ignoring it. + # gh-108948: FreeBSD fails with EFTYPE if sticky bit cannot be set, + # instead of ignoring it. if hasattr(errno, "EFTYPE") and err.errno == errno.EFTYPE: os.chmod(tmp_filename, new_mode & ~stat.S_ISVTX) else: From 02d7c217d253948b23ed1a244258ac2bfa47cd90 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Fri, 15 Sep 2023 15:45:37 +0200 Subject: [PATCH 09/21] add explanatory comment Co-authored-by: Victor Stinner --- Lib/tarfile.py | 1 + Lib/test/test_tarfile.py | 1 + 2 files changed, 2 insertions(+) diff --git a/Lib/tarfile.py b/Lib/tarfile.py index a5f54e92528474..53ea4f0f23def0 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -2578,6 +2578,7 @@ def chmod(self, tarinfo, targetpath): # sticky bit on a file as non-root. But it's a noop in most # other platforms, and we try and match that behavior here. try: + # Retry without the sticky bit os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) except OSError as e2: raise e2 from e diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 483da1cb2daa88..0d17062f4e5614 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3851,6 +3851,7 @@ def test_modes(self): # gh-108948: FreeBSD fails with EFTYPE if sticky bit cannot be set, # instead of ignoring it. if hasattr(errno, "EFTYPE") and err.errno == errno.EFTYPE: + # Retry without the sticky bit os.chmod(tmp_filename, new_mode & ~stat.S_ISVTX) else: raise From 94efbbb42c7830f402f27c1d5c2d2cf1672ff0d3 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Fri, 15 Sep 2023 16:59:21 +0200 Subject: [PATCH 10/21] apply review suggestions --- Lib/tarfile.py | 28 +++++++++++++++++----------- Lib/test/test_tarfile.py | 23 ++++++++++++++--------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 53ea4f0f23def0..42543940ee8aac 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -2570,18 +2570,24 @@ def chmod(self, tarinfo, targetpath): try: try: os.chmod(targetpath, tarinfo.mode) - except OSError as e: - if not(hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE): - raise - + except OSError as exc1: # gh-108948: On FreeBSD, chmod() fails when trying to set the - # sticky bit on a file as non-root. But it's a noop in most - # other platforms, and we try and match that behavior here. - try: - # Retry without the sticky bit - os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) - except OSError as e2: - raise e2 from e + # sticky bit on a file as non-root. On other platforms, the bit + # is silently ignored if it cannot be set. We make FreeBSD + # behave like other platforms by catching the error and trying + # again without the sticky bit. + if (hasattr(errno, "EFTYPE") and exc1.errno == errno.EFTYPE + and (tarinfo.mode & stat.S_ISVTX)): + try: + # Retry without the sticky bit + os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) + except OSError as exc2: + # The error from the second attempt is the direct cause + # of the ExtractError, but we keep the original error + # around for good information. + raise exc2 from exc1 + else: + raise except OSError as e: raise ExtractError("could not change mode") from e diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 0d17062f4e5614..7823fb74e66f98 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -696,26 +696,30 @@ def test_extract_chmod_eftype(self): # Extracting a file as non-root should skip the sticky bit (gh-108948) # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But # we need to take care that any other error is preserved. + mode = "-rw-rw-rwt" with ArchiveMaker() as arc: - arc.add("sticky", mode="?rw-rw-rwt") + arc.add("sticky1", mode=mode) + arc.add("sticky2", mode=mode) tar = arc.open(errorlevel=2) DIR = os.path.join(TEMPDIR, "chmod") os.mkdir(DIR) - try: + self.addCleanup(os_helper.rmtree, DIR) + with tar: # this should not raise: - tar.extract("sticky", DIR) + tar.extract("sticky1", DIR) + got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) + expected_mode = "-rw-rw-rw-" if os.geteuid() != 0 else "-rw-rw-rwt" + self.assertEqual(got_mode, expected_mode) + # but we can create a situation where it does raise: with unittest.mock.patch("os.chmod") as mock: eftype_error = OSError(errno.EFTYPE, "EFTYPE") other_error = OSError(errno.EPERM, "different error") mock.side_effect = [eftype_error, other_error] with self.assertRaises(tarfile.ExtractError) as excinfo: - tar.extract("sticky", DIR) + tar.extract("sticky2", DIR) self.assertEqual(excinfo.exception.__cause__, other_error) self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) - finally: - os_helper.rmtree(DIR) - tar.close() @os_helper.skip_unless_working_chmod def test_extract_directory(self): @@ -3848,8 +3852,9 @@ def test_modes(self): try: os.chmod(tmp_filename, new_mode) except OSError as err: - # gh-108948: FreeBSD fails with EFTYPE if sticky bit cannot be set, - # instead of ignoring it. + # gh-108948: While chmod on most platforms silently ignores the + # sticky bit if it cannot be set (i.e. setting it on a file as + # non-root), chmod on FreeBSD raises EFTYPE to indicate the case. if hasattr(errno, "EFTYPE") and err.errno == errno.EFTYPE: # Retry without the sticky bit os.chmod(tmp_filename, new_mode & ~stat.S_ISVTX) From d1824c8db7eecbe02856b69101dde9b348c7fc10 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Fri, 15 Sep 2023 17:06:01 +0200 Subject: [PATCH 11/21] fix expected mode --- Lib/test/test_tarfile.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 7823fb74e66f98..065e7cf9f1b4e8 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -696,7 +696,7 @@ def test_extract_chmod_eftype(self): # Extracting a file as non-root should skip the sticky bit (gh-108948) # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But # we need to take care that any other error is preserved. - mode = "-rw-rw-rwt" + mode = "-rwxrwxrwt" with ArchiveMaker() as arc: arc.add("sticky1", mode=mode) arc.add("sticky2", mode=mode) @@ -708,7 +708,7 @@ def test_extract_chmod_eftype(self): # this should not raise: tar.extract("sticky1", DIR) got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) - expected_mode = "-rw-rw-rw-" if os.geteuid() != 0 else "-rw-rw-rwt" + expected_mode = "-rwxrwxrwx" if os.geteuid() != 0 else "-rwxrwxrwt" self.assertEqual(got_mode, expected_mode) # but we can create a situation where it does raise: From bd1dba7aaa2f65f136018efc84394c77282800c1 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Fri, 15 Sep 2023 17:07:06 +0200 Subject: [PATCH 12/21] avoid deprecation warnings --- Lib/test/test_tarfile.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 065e7cf9f1b4e8..2d1ac971da493f 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -706,7 +706,7 @@ def test_extract_chmod_eftype(self): self.addCleanup(os_helper.rmtree, DIR) with tar: # this should not raise: - tar.extract("sticky1", DIR) + tar.extract("sticky1", DIR, filter="fully_trusted") got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) expected_mode = "-rwxrwxrwx" if os.geteuid() != 0 else "-rwxrwxrwt" self.assertEqual(got_mode, expected_mode) @@ -717,7 +717,7 @@ def test_extract_chmod_eftype(self): other_error = OSError(errno.EPERM, "different error") mock.side_effect = [eftype_error, other_error] with self.assertRaises(tarfile.ExtractError) as excinfo: - tar.extract("sticky2", DIR) + tar.extract("sticky2", DIR, filter="fully_trusted") self.assertEqual(excinfo.exception.__cause__, other_error) self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) From 417e31adb71cebeb5d0311c7cd02838f5849e65d Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 14:43:10 +0200 Subject: [PATCH 13/21] Update Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst Co-authored-by: Ethan Furman --- .../next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst index 36bfcf6c41e6f9..1d1dcb12356524 100644 --- a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst +++ b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst @@ -1 +1 @@ -On FreeBSD, :mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. +:mod:`tarfile`: On FreeBSD, ``tarfile`` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. From 83c08fca9d053e026e3533b47e0d5126ed7078cf Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 12:07:37 +0200 Subject: [PATCH 14/21] move tarfile eftype test test_extract_chmod_eftype was run once per testtar.* file but it did not use those files --- Lib/test/test_tarfile.py | 63 +++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 2d1ac971da493f..7df186f31e8802 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -691,36 +691,6 @@ def format_mtime(mtime): tar.close() os_helper.rmtree(DIR) - @unittest.skipUnless(hasattr(errno, "EFTYPE"), "errno.EFTYPE required") - def test_extract_chmod_eftype(self): - # Extracting a file as non-root should skip the sticky bit (gh-108948) - # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But - # we need to take care that any other error is preserved. - mode = "-rwxrwxrwt" - with ArchiveMaker() as arc: - arc.add("sticky1", mode=mode) - arc.add("sticky2", mode=mode) - tar = arc.open(errorlevel=2) - DIR = os.path.join(TEMPDIR, "chmod") - os.mkdir(DIR) - self.addCleanup(os_helper.rmtree, DIR) - with tar: - # this should not raise: - tar.extract("sticky1", DIR, filter="fully_trusted") - got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) - expected_mode = "-rwxrwxrwx" if os.geteuid() != 0 else "-rwxrwxrwt" - self.assertEqual(got_mode, expected_mode) - - # but we can create a situation where it does raise: - with unittest.mock.patch("os.chmod") as mock: - eftype_error = OSError(errno.EFTYPE, "EFTYPE") - other_error = OSError(errno.EPERM, "different error") - mock.side_effect = [eftype_error, other_error] - with self.assertRaises(tarfile.ExtractError) as excinfo: - tar.extract("sticky2", DIR, filter="fully_trusted") - self.assertEqual(excinfo.exception.__cause__, other_error) - self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) - @os_helper.skip_unless_working_chmod def test_extract_directory(self): dirtype = "ustar/dirtype" @@ -3087,6 +3057,39 @@ def test_keyword_only(self, mock_geteuid): tarfl.extract, filename_1, TEMPDIR, False, True) +@os_helper.skip_unless_working_chmod +class FileModesTest(unittest.TestCase): + @unittest.skipUnless(hasattr(errno, "EFTYPE"), "errno.EFTYPE required") + def test_extract_chmod_eftype(self): + # Extracting a file as non-root should skip the sticky bit (gh-108948) + # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But + # we need to take care that any other error is preserved. + mode = "-rwxrwxrwt" + with ArchiveMaker() as arc: + arc.add("sticky1", mode=mode) + arc.add("sticky2", mode=mode) + tar = arc.open(errorlevel=2) + DIR = os.path.join(TEMPDIR, "chmod") + os.mkdir(DIR) + self.addCleanup(os_helper.rmtree, DIR) + with tar: + # this should not raise: + tar.extract("sticky1", DIR, filter="fully_trusted") + got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) + expected_mode = "-rwxrwxrwx" if os.geteuid() != 0 else "-rwxrwxrwt" + self.assertEqual(got_mode, expected_mode) + + # but we can create a situation where it does raise: + with unittest.mock.patch("os.chmod") as mock: + eftype_error = OSError(errno.EFTYPE, "EFTYPE") + other_error = OSError(errno.EPERM, "different error") + mock.side_effect = [eftype_error, other_error] + with self.assertRaises(tarfile.ExtractError) as excinfo: + tar.extract("sticky2", DIR, filter="fully_trusted") + self.assertEqual(excinfo.exception.__cause__, other_error) + self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) + + class ReplaceTests(ReadTest, unittest.TestCase): def test_replace_name(self): member = self.tar.getmember('ustar/regtype') From 54f5548b0e7dbefd1d12e43cfdf96e5903099e2a Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 12:36:02 +0200 Subject: [PATCH 15/21] eftype test should handle macos --- Lib/test/test_tarfile.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 7df186f31e8802..414fb26a15d314 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3057,6 +3057,32 @@ def test_keyword_only(self, mock_geteuid): tarfl.extract, filename_1, TEMPDIR, False, True) +_can_chmod_set_sticky = None + + +def can_chmod_set_sticky(): + def can_chmod_set_sticky_inner(): + if not os_helper.can_chmod(): + return False + filename = os_helper.TESTFN + "-chmod-sticky" + try: + with open(filename, "w") as f: + os.chmod(f.fileno(), 0o666) + try: + os.chmod(f.fileno(), 0o666 | stat.S_ISVTX) + except OSError: + return False + mode = os.stat(f.fileno()).st_mode + return bool(mode & stat.S_ISVTX) + finally: + os.unlink(filename) + + global _can_chmod_set_sticky + if _can_chmod_set_sticky is None: + _can_chmod_set_sticky = can_chmod_set_sticky_inner() + return _can_chmod_set_sticky + + @os_helper.skip_unless_working_chmod class FileModesTest(unittest.TestCase): @unittest.skipUnless(hasattr(errno, "EFTYPE"), "errno.EFTYPE required") @@ -3076,7 +3102,10 @@ def test_extract_chmod_eftype(self): # this should not raise: tar.extract("sticky1", DIR, filter="fully_trusted") got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) - expected_mode = "-rwxrwxrwx" if os.geteuid() != 0 else "-rwxrwxrwt" + if can_chmod_set_sticky(): + expected_mode = "-rwxrwxrwt" + else: + expected_mode = "-rwxrwxrwx" self.assertEqual(got_mode, expected_mode) # but we can create a situation where it does raise: From b60ada655f4f5564c90a2016ef7641c112e4c78b Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 12:42:30 +0200 Subject: [PATCH 16/21] use can_chmod_set_sticky helper where possible --- Lib/test/test_tarfile.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 414fb26a15d314..b6fa089b74598c 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3879,19 +3879,10 @@ def test_modes(self): tmp_filename = os.path.join(TEMPDIR, "tmp.file") with open(tmp_filename, 'w'): pass - new_mode = (os.stat(tmp_filename).st_mode - | stat.S_ISVTX | stat.S_ISGID | stat.S_ISUID) - try: - os.chmod(tmp_filename, new_mode) - except OSError as err: - # gh-108948: While chmod on most platforms silently ignores the - # sticky bit if it cannot be set (i.e. setting it on a file as - # non-root), chmod on FreeBSD raises EFTYPE to indicate the case. - if hasattr(errno, "EFTYPE") and err.errno == errno.EFTYPE: - # Retry without the sticky bit - os.chmod(tmp_filename, new_mode & ~stat.S_ISVTX) - else: - raise + new_mode = os.stat(tmp_filename).st_mode | stat.S_ISGID | stat.S_ISUID + if can_chmod_set_sticky(): + new_mode |= stat.S_ISVTX + os.chmod(tmp_filename, new_mode) got_mode = os.stat(tmp_filename).st_mode _t_file = 't' if (got_mode & stat.S_ISVTX) else 'x' _suid_file = 's' if (got_mode & stat.S_ISUID) else 'x' From 5a51aabf35891b03c671c1977987df044d3068c7 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 13:02:02 +0200 Subject: [PATCH 17/21] bit of polishing --- Lib/test/test_tarfile.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index b6fa089b74598c..dd1be18d70d7ff 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3061,6 +3061,8 @@ def test_keyword_only(self, mock_geteuid): def can_chmod_set_sticky(): + """Can chmod set the sticky bit on a file?""" + def can_chmod_set_sticky_inner(): if not os_helper.can_chmod(): return False @@ -3085,27 +3087,23 @@ def can_chmod_set_sticky_inner(): @os_helper.skip_unless_working_chmod class FileModesTest(unittest.TestCase): - @unittest.skipUnless(hasattr(errno, "EFTYPE"), "errno.EFTYPE required") + @unittest.skipUnless(hasattr(errno, "EFTYPE"), "requires errno.EFTYPE") def test_extract_chmod_eftype(self): # Extracting a file as non-root should skip the sticky bit (gh-108948) # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But # we need to take care that any other error is preserved. mode = "-rwxrwxrwt" + expected_mode = mode if can_chmod_set_sticky() else "-rwxrwxrwx" with ArchiveMaker() as arc: arc.add("sticky1", mode=mode) arc.add("sticky2", mode=mode) - tar = arc.open(errorlevel=2) DIR = os.path.join(TEMPDIR, "chmod") os.mkdir(DIR) self.addCleanup(os_helper.rmtree, DIR) - with tar: + with arc.open(errorlevel=2) as tar: # this should not raise: tar.extract("sticky1", DIR, filter="fully_trusted") got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) - if can_chmod_set_sticky(): - expected_mode = "-rwxrwxrwt" - else: - expected_mode = "-rwxrwxrwx" self.assertEqual(got_mode, expected_mode) # but we can create a situation where it does raise: From cac6595cbf6ced94478d561d58a57a9c4f703b12 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 14:05:11 +0200 Subject: [PATCH 18/21] split and refactor tests --- Lib/test/test_tarfile.py | 82 +++++++++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 27 deletions(-) diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index dd1be18d70d7ff..ce8f78e8b57092 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3086,35 +3086,63 @@ def can_chmod_set_sticky_inner(): @os_helper.skip_unless_working_chmod -class FileModesTest(unittest.TestCase): - @unittest.skipUnless(hasattr(errno, "EFTYPE"), "requires errno.EFTYPE") - def test_extract_chmod_eftype(self): - # Extracting a file as non-root should skip the sticky bit (gh-108948) - # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD). But - # we need to take care that any other error is preserved. - mode = "-rwxrwxrwt" - expected_mode = mode if can_chmod_set_sticky() else "-rwxrwxrwx" +class ExtractStickyFileTest(unittest.TestCase): + + # to use in mock tests, on platforms that don't have it + fake_EFTYPE = getattr(errno, "EFTYPE", max(errno.errorcode) + 100) + + def setUp(self): + self.test_dir = os.path.join(TEMPDIR, "chmod") + os.mkdir(self.test_dir) + self.addCleanup(os_helper.rmtree, self.test_dir) + self.sticky_mode = "-rwxrwxrwt" + self.non_sticky_mode = "-rwxrwxrwx" + + def extract_sticky_file(self, file_name="sticky"): with ArchiveMaker() as arc: - arc.add("sticky1", mode=mode) - arc.add("sticky2", mode=mode) - DIR = os.path.join(TEMPDIR, "chmod") - os.mkdir(DIR) - self.addCleanup(os_helper.rmtree, DIR) + arc.add(file_name, mode=self.sticky_mode) with arc.open(errorlevel=2) as tar: - # this should not raise: - tar.extract("sticky1", DIR, filter="fully_trusted") - got_mode = stat.filemode(os.stat(os.path.join(DIR, "sticky1")).st_mode) - self.assertEqual(got_mode, expected_mode) - - # but we can create a situation where it does raise: - with unittest.mock.patch("os.chmod") as mock: - eftype_error = OSError(errno.EFTYPE, "EFTYPE") - other_error = OSError(errno.EPERM, "different error") - mock.side_effect = [eftype_error, other_error] - with self.assertRaises(tarfile.ExtractError) as excinfo: - tar.extract("sticky2", DIR, filter="fully_trusted") - self.assertEqual(excinfo.exception.__cause__, other_error) - self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) + tar.extract(file_name, self.test_dir, filter="fully_trusted") + self.extracted_path = os.path.join(self.test_dir, file_name) + + def test_extract_chmod_eftype_success(self): + # Extracting a file as non-root should skip the sticky bit (gh-108948) + # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD) + expected_mode = self.sticky_mode if can_chmod_set_sticky() else self.non_sticky_mode + self.extract_sticky_file() + got_mode = stat.filemode(os.stat(self.extracted_path).st_mode) + self.assertEqual(got_mode, expected_mode) + + @unittest.mock.patch.object(errno, "EFTYPE", fake_EFTYPE, create=True) + def test_extract_chmod_eftype_mock(self): + # Like test_extract_chmod_eftype_success but mock chmod so we can + # explicitly test the case we want, regardless of the OS. + with unittest.mock.patch("os.chmod") as mock: + eftype_error = OSError(errno.EFTYPE, "EFTYPE") + mock.side_effect = [eftype_error, None] + self.extract_sticky_file() + self.assertTrue(mock.call_count, 2) + _, chmod_mode1 = mock.call_args_list[0].args + _, chmod_mode2 = mock.call_args_list[1].args + self.assertTrue(chmod_mode1 & stat.S_ISVTX) + self.assertFalse(chmod_mode2 & stat.S_ISVTX) + self.assertEqual(chmod_mode1 | chmod_mode2, chmod_mode1) + + @unittest.mock.patch.object(errno, "EFTYPE", fake_EFTYPE, create=True) + def test_extract_chmod_eftype_error(self): + # Take care that any other error is preserved in the EFTYPE case. + # It's hard to create a situation where chmod() fails with EFTYPE and, + # retrying without the sticky bit, it fails with a different error. But + # we can mock it, so we can exercise all branches. + with unittest.mock.patch("os.chmod") as mock: + eftype_error = OSError(errno.EFTYPE, "EFTYPE") + other_error = OSError(errno.EPERM, "different error") + mock.side_effect = [eftype_error, other_error] + with self.assertRaises(tarfile.ExtractError, + msg="could not change mode") as excinfo: + self.extract_sticky_file() + self.assertEqual(excinfo.exception.__cause__, other_error) + self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) class ReplaceTests(ReadTest, unittest.TestCase): From 23a6fd57c5bcf9126b92d1a37093f52d1198763c Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 14:17:34 +0200 Subject: [PATCH 19/21] make TarFile.chmod slightly more readable --- Lib/tarfile.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 42543940ee8aac..b5fe9cb71a7b07 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -2586,10 +2586,13 @@ def chmod(self, tarinfo, targetpath): # of the ExtractError, but we keep the original error # around for good information. raise exc2 from exc1 + else: + # The second chmod() without sticky bit succeeded + pass else: raise - except OSError as e: - raise ExtractError("could not change mode") from e + except OSError as exc3: + raise ExtractError("could not change mode") from exc3 def utime(self, tarinfo, targetpath): """Set modification time of targetpath according to tarinfo. From 060bcb208bf04073b58ed271adf4b1ee41ab0910 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Sat, 16 Sep 2023 14:41:17 +0200 Subject: [PATCH 20/21] document tarfile behavior with sticky bit on files --- Doc/library/tarfile.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Doc/library/tarfile.rst b/Doc/library/tarfile.rst index 62d67bc577c7d0..b5ca8424432f2e 100644 --- a/Doc/library/tarfile.rst +++ b/Doc/library/tarfile.rst @@ -939,6 +939,13 @@ reused in custom filters: This implements the ``'fully_trusted'`` filter. + .. note:: + + Although the filter preserves all permission bits unchanged, the sticky + bit (:const:`~stat.S_ISVTX`) will not be set on extracted files when the + system does not allow it. For compatibility reasons, no error is raised + in this case. + .. function:: tar_filter(member, path) Implements the ``'tar'`` filter. From 5ae3e30f9935b3a5b3a5216413ba9b8c5688a784 Mon Sep 17 00:00:00 2001 From: Davide Rizzo Date: Mon, 18 Sep 2023 12:04:16 +0200 Subject: [PATCH 21/21] apply review suggestions --- Doc/library/tarfile.rst | 6 +++--- Lib/test/support/os_helper.py | 33 +++++++++++++++++++++++++++++++++ Lib/test/test_tarfile.py | 35 +++++------------------------------ 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/Doc/library/tarfile.rst b/Doc/library/tarfile.rst index b5ca8424432f2e..36f0643085c838 100644 --- a/Doc/library/tarfile.rst +++ b/Doc/library/tarfile.rst @@ -942,9 +942,9 @@ reused in custom filters: .. note:: Although the filter preserves all permission bits unchanged, the sticky - bit (:const:`~stat.S_ISVTX`) will not be set on extracted files when the - system does not allow it. For compatibility reasons, no error is raised - in this case. + bit (:const:`~stat.S_ISVTX`) will not be set on an extracted file if the + file system does not allow it. For compatibility reasons, no exception is + raised in this case. .. function:: tar_filter(member, path) diff --git a/Lib/test/support/os_helper.py b/Lib/test/support/os_helper.py index 821a4b1ffd5077..326dd0ebdadb0f 100644 --- a/Lib/test/support/os_helper.py +++ b/Lib/test/support/os_helper.py @@ -275,6 +275,39 @@ def skip_unless_working_chmod(test): return test if ok else unittest.skip(msg)(test) +_can_chmod_set_sticky_bit = None + + +def can_chmod_set_sticky_bit(): + """Return True if chmod allows to set the sticky bit on a regular file. + + Return False if chmod fails with an error, or it succeeds but does not set + the sticky bit. + """ + + def can_chmod_set_sticky_bit_inner(): + if not can_chmod(): + return False + filename = TESTFN + try: + with open(filename, "w") as f: + # assume it won't fail because can_chmod() returned True: + os.chmod(f.fileno(), 0o666) + try: + os.chmod(f.fileno(), 0o666 | stat.S_ISVTX) + except OSError: + return False + mode = os.stat(f.fileno()).st_mode + return bool(mode & stat.S_ISVTX) + finally: + unlink(filename) + + global _can_chmod_set_sticky_bit + if _can_chmod_set_sticky_bit is None: + _can_chmod_set_sticky_bit = can_chmod_set_sticky_bit_inner() + return _can_chmod_set_sticky_bit + + # Check whether the current effective user has the capability to override # DAC (discretionary access control). Typically user root is able to # bypass file read, write, and execute permission checks. The capability diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index ce8f78e8b57092..382932cf7ff00d 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -3057,34 +3057,6 @@ def test_keyword_only(self, mock_geteuid): tarfl.extract, filename_1, TEMPDIR, False, True) -_can_chmod_set_sticky = None - - -def can_chmod_set_sticky(): - """Can chmod set the sticky bit on a file?""" - - def can_chmod_set_sticky_inner(): - if not os_helper.can_chmod(): - return False - filename = os_helper.TESTFN + "-chmod-sticky" - try: - with open(filename, "w") as f: - os.chmod(f.fileno(), 0o666) - try: - os.chmod(f.fileno(), 0o666 | stat.S_ISVTX) - except OSError: - return False - mode = os.stat(f.fileno()).st_mode - return bool(mode & stat.S_ISVTX) - finally: - os.unlink(filename) - - global _can_chmod_set_sticky - if _can_chmod_set_sticky is None: - _can_chmod_set_sticky = can_chmod_set_sticky_inner() - return _can_chmod_set_sticky - - @os_helper.skip_unless_working_chmod class ExtractStickyFileTest(unittest.TestCase): @@ -3108,7 +3080,10 @@ def extract_sticky_file(self, file_name="sticky"): def test_extract_chmod_eftype_success(self): # Extracting a file as non-root should skip the sticky bit (gh-108948) # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD) - expected_mode = self.sticky_mode if can_chmod_set_sticky() else self.non_sticky_mode + if os_helper.can_chmod_set_sticky_bit(): + expected_mode = self.sticky_mode + else: + expected_mode = self.non_sticky_mode self.extract_sticky_file() got_mode = stat.filemode(os.stat(self.extracted_path).st_mode) self.assertEqual(got_mode, expected_mode) @@ -3906,7 +3881,7 @@ def test_modes(self): with open(tmp_filename, 'w'): pass new_mode = os.stat(tmp_filename).st_mode | stat.S_ISGID | stat.S_ISUID - if can_chmod_set_sticky(): + if os_helper.can_chmod_set_sticky_bit(): new_mode |= stat.S_ISVTX os.chmod(tmp_filename, new_mode) got_mode = os.stat(tmp_filename).st_mode