Skip to content

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.

Sign up for GitHub

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

Closed
sorcio wants to merge 21 commits into from
Closed

gh-108948: tarfile should handle sticky bit in FreeBSD #108950

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6a4be8f
tarfile: fix sticky bit handling in FreeBSD
sorcio Sep 5, 2023
50729f7
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 5, 2023
3587710
fix blurb
sorcio Sep 5, 2023
41491bb
fix blurb
sorcio Sep 5, 2023
4f6536d
fix chmod exception handler in tarfile
sorcio Sep 5, 2023
fe7dfed
chain exceptions
sorcio Sep 5, 2023
4009573
test exception branches
sorcio Sep 6, 2023
7b784b5
Update Lib/test/test_tarfile.py
sorcio Sep 7, 2023
02d7c21
add explanatory comment
sorcio Sep 15, 2023
94efbbb
apply review suggestions
sorcio Sep 15, 2023
d1824c8
fix expected mode
sorcio Sep 15, 2023
bd1dba7
avoid deprecation warnings
sorcio Sep 15, 2023
417e31a
Update Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.H…
sorcio Sep 16, 2023
83c08fc
move tarfile eftype test
sorcio Sep 16, 2023
54f5548
eftype test should handle macos
sorcio Sep 16, 2023
b60ada6
use can_chmod_set_sticky helper where possible
sorcio Sep 16, 2023
5a51aab
bit of polishing
sorcio Sep 16, 2023
cac6595
split and refactor tests
sorcio Sep 16, 2023
23a6fd5
make TarFile.chmod slightly more readable
sorcio Sep 16, 2023
060bcb2
document tarfile behavior with sticky bit on files
sorcio Sep 16, 2023
5ae3e30
apply review suggestions
sorcio Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
move tarfile eftype test 
test_extract_chmod_eftype was run once per testtar.* file but it did not use those files
  • Loading branch information
@sorcio
sorcio committed Sep 16, 2023
commit 83c08fca9d053e026e3533b47e0d5126ed7078cf
63 changes: 33 additions & 30 deletions Lib/test/test_tarfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Copy Markdown
Member

@vstinner vstinner Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Extracting a file as non-root should skip the sticky bit (gh-108948)
# gh-108948: Extracting a file as non-root should skip the sticky bit

# 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')
Expand Down