-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-108948: tarfile should handle sticky bit in FreeBSD #108950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6a4be8f
50729f7
3587710
41491bb
4f6536d
fe7dfed
4009573
7b784b5
02d7c21
94efbbb
d1824c8
bd1dba7
417e31a
83c08fc
54f5548
b60ada6
5a51aab
cac6595
23a6fd5
060bcb2
5ae3e30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, I suggest you to mimick code around in os_helper.py: other "can" functions also have a cache (global variable), but they don't use an "inner" function. |
||
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 | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ | |||||||||
import re | ||||||||||
import warnings | ||||||||||
import stat | ||||||||||
import errno | ||||||||||
|
||||||||||
import unittest | ||||||||||
import unittest.mock | ||||||||||
|
@@ -3056,6 +3057,69 @@ def test_keyword_only(self, mock_geteuid): | |||||||||
tarfl.extract, filename_1, TEMPDIR, False, True) | ||||||||||
|
||||||||||
|
||||||||||
@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): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On most platforms, this test willl be run without EFTYPE. I suggest to remove it:
Suggested change
|
||||||||||
# Extracting a file as non-root should skip the sticky bit (gh-108948) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# even on platforms where chmod fails with EFTYPE (i.e. FreeBSD) | ||||||||||
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) | ||||||||||
|
||||||||||
@unittest.mock.patch.object(errno, "EFTYPE", fake_EFTYPE, create=True) | ||||||||||
def test_extract_chmod_eftype_mock(self): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# Like test_extract_chmod_eftype_success but mock chmod so we can | ||||||||||
# explicitly test the case we want, regardless of the OS. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to describe which code path is tested: Test chmod() failing with OSError(EFTYPE) and then pass when re-run without the sticky bit. |
||||||||||
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): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# Take care that any other error is preserved in the EFTYPE case. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to add first (but preserve your longer comment): Test chmod() failing with OSError(EFTYPE) and then raise PermissionError() when re-run without the sticky bit. |
||||||||||
# 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) | ||||||||||
Comment on lines
+3119
to
+3120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
|
||||||||||
class ReplaceTests(ReadTest, unittest.TestCase): | ||||||||||
def test_replace_name(self): | ||||||||||
member = self.tar.getmember('ustar/regtype') | ||||||||||
|
@@ -3816,8 +3880,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 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 | ||||||||||
_t_file = 't' if (got_mode & stat.S_ISVTX) else 'x' | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. |
Uh oh!
There was an error while loading. Please reload this page.