gh-108948: tarfile should handle sticky bit in FreeBSD

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.

Lib/tarfile.py

@@ -37,6 +37,7 @@
 # Imports
 #---------
 from builtins import open as bltn_open
+import errno
 import sys
 import os
 import io
@@ -2567,9 +2568,31 @@ def chmod(self, tarinfo, targetpath):
         if tarinfo.mode is None:
             return
         try:
-            os.chmod(targetpath, tarinfo.mode)
-        except OSError as e:
-            raise ExtractError("could not change mode") from e
+            try:
+                os.chmod(targetpath, tarinfo.mode)
+            except OSError as exc1:
+                # gh-108948: On FreeBSD, chmod() fails when trying to set the
+                # 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:
+                        # The second chmod() without sticky bit succeeded
+                        pass
+                else:
+                    raise
+        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.

Lib/test/test_tarfile.py

@@ -9,6 +9,7 @@
 import re
 import warnings
 import stat
+import errno
 
 import unittest
 import unittest.mock
@@ -3056,6 +3057,94 @@ 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):
+
+    # 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(file_name, mode=self.sticky_mode)
+        with arc.open(errorlevel=2) as tar:
+            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):
     def test_replace_name(self):
         member = self.tar.getmember('ustar/regtype')
@@ -3816,8 +3905,9 @@ 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)
+        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'

Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst

@@ -0,0 +1 @@
+: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.