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
wants to merge 21 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 6 commits
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
15 changes: 14 additions & 1 deletion Lib/tarfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
# Imports
#---------
from builtins import open as bltn_open
import errno
import sys
import os
import io
Expand Down Expand Up @@ -2567,7 +2568,19 @@ def chmod(self, tarinfo, targetpath):
if tarinfo.mode is None:
return
try:
os.chmod(targetpath, tarinfo.mode)
try:
os.chmod(targetpath, tarinfo.mode)
except OSError as e:
Copy link
Member

Choose a reason for hiding this comment

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

Please use variable names longer than 1 character: you can use exc and exc2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in favor of this but let me double check: throughout the file the convention seems to be to use except ... as e:. Is it okay to break the convention?

Copy link
Member

Choose a reason for hiding this comment

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

Please break the convention, it's a bad convention :-)

if not(hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe only retry chmod() when it's needed? Or is this test redundant with checking for EFTYPE?

Suggested change
if not(hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE):
if not(hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE and (tarinfo.mode & stat.S_ISVTX)):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, at least on FreeBSD. The documentation is very explicit about the condition causing the error:

The effective user ID is not the super-user, the mode includes the sticky bit (S_ISVTX), and path does not refer to a directory.

I don't know of other systems with the same error, so the hasattr() check might be enough. But it might be a good idea to be defensive. It's not like it's an expensive check.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, no need to test for the bit in this case.

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.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean that on other platforms chmod() silently ignores the sticky bit for non-root users? I'm not sure that I understood correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. I will reword more clearly. I was being stingy on the number of lines to take for this comment :) Same in the similar comment in the test case. I will reword there too.

try:
os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX)
except OSError as e2:
raise e2 from e
except OSError as e:
raise ExtractError("could not change mode") from e
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except OSError as e:
raise ExtractError("could not change mode") from e
except OSError as exc3:
raise ExtractError("could not change mode") from exc3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


Expand Down
11 changes: 10 additions & 1 deletion Lib/test/test_tarfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import re
import warnings
import stat
import errno

import unittest
import unittest.mock
Expand Down Expand Up @@ -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'
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +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.
Copy link
Member

Choose a reason for hiding this comment

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

Hum. I suggest to explain it differently.

:mod:`tarfile`: On FreeBSD, if the sticky bit cannot be set,
if the user is not root for example, clear the sticky bit,
matching the behavior of other platforms.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be worth it to add a note about the sticky bit in https://docs.python.org/dev/library/tarfile.html#tarfile.TarFile.extract documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with Ethan's suggestion for the blurb.

I also added a note, but under the filters documentation, so if a user searches for "sticky" or "bits" or similar keywords they find relevant information right there. I can change it or add more, of course.