8000 gh-91133: tempfile.TemporaryDirectory: fix symlink bug in cleanup by kwi-dk · Pull Request #99930 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

gh-91133: tempfile.TemporaryDirectory: fix symlink bug in cleanup #99930

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

Merged
merged 15 commits into from
Dec 7, 2023
Merged
Changes from 1 commit
Commits
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
Add a test for file flags.
  • Loading branch information
serhiy-storchaka committed Dec 6, 2023
commit 0be03d13307ebe9483efcaeae09496e00a13874e
60 changes: 56 additions & 4 deletions Lib/test/test_tempfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -1674,8 +1674,8 @@ def test_cleanup_with_symlink_to_a_directory(self):
d2.cleanup()

@os_helper.skip_unless_symlink
def test_cleanup_with_error_deleting_symlink(self):
# cleanup() should not follow symlinks when fixing mode bits etc. (#91133)
def test_cleanup_with_symlink_modes(self):
# cleanup() should not follow symlinks when fixing mode bits (#91133)
with self.do_create(recurse=0) as d2:
file1 = os.path.join(d2, 'file1')
open(file1, 'wb').close()
Expand Down Expand Up @@ -1722,6 +1722,54 @@ def test(target, target_is_directory):
self.assertEqual(new_mode, old_mode,
'%03o != %03o' % (new_mode, old_mode))

@unittest.skipUnless(hasattr(os, 'chflags'), 'requires os.chflags')
@os_helper.skip_unless_symlink
def test_cleanup_with_symlink_flags(self):
# cleanup() should not follow symlinks when fixing flags (#91133)
flags = stat.UF_IMMUTABLE | stat.UF_NOUNLINK
self.check_flags()

with self.do_create(recurse=0) as d2:
file1 = os.path.join(d2, 'file1')
open(file1, 'wb').close()
dir1 = os.path.join(d2, 'dir1')
os.mkdir(dir1)
def test(target, target_is_directory):
d1 = self.do_create(recurse=0)
symlink = os.path.join(d1.name, 'symlink')
os.symlink(target, symlink,
target_is_directory=target_is_directory)
try:
os.chflags(symlink, flags, follow_symlinks=False)
except NotImplementedError:
pass
try:
os.chflags(symlink, flags)
except FileNotFoundError:
pass
os.chflags(d1.name, flags)
d1.cleanup()
self.assertFalse(os.path.exists(d1.name))

with self.subTest('nonexisting file'):
test('nonexisting', target_is_directory=False)
with self.subTest('nonexisting dir'):
test('nonexisting', target_is_directory=True)

with self.subTest('existing file'):
os.chflags(file1, flags)
old_flags = os.stat(file1).st_flags
test(file1, target_is_directory=False)
new_flags = os.stat(file1).st_flags
self.assertEqual(new_flags, old_flags)

with self.subTest('existing dir'):
os.chflags(dir1, flags)
old_flags = os.stat(dir1).st_flags
test(dir1, target_is_directory=True)
new_flags = os.stat(dir1).st_flags
self.assertEqual(new_flags, old_flags)

@support.cpython_only
def test_del_on_collection(self):
# A TemporaryDirectory is deleted when garbage collected
Expand Down Expand Up @@ -1894,8 +1942,7 @@ def test_modes(self):
d.cleanup()
self.assertFalse(os.path.exists(d.name))

@unittest.skipUnless(hasattr(os, 'chflags'), 'requires os.chflags')
def test_flags(self):
def check_flags(self):
flags = stat.UF_IMMUTABLE | stat.UF_NOUNLINK

# skip the test if these flags are not supported (ex: FreeBSD 13)
Expand All @@ -1913,6 +1960,11 @@ def test_flags(self):
finally:
os_helper.unlink(filename)

@unittest.skipUnless(hasattr(os, 'chflags'), 'requires os.chflags')
def test_flags(self):
flags = stat.UF_IMMUTABLE | stat.UF_NOUNLINK
self.check_flags()
Copy link
Member

Choose a reason for hiding this comment

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

You can pass flags to check_flags(), rather than having a copy inside the function. I also suggest moving the comment here:

Suggested change
flags = stat.UF_IMMUTABLE | stat.UF_NOUNLINK
self.check_flags()
# skip the test if these flags are not supported (ex: FreeBSD 13)
flags = stat.UF_IMMUTABLE | stat.UF_NOUNLINK
self.check_flags(flags)

Copy link
Member

Choose a reason for hiding this comment

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

I do not want to repeat the same comment 3 times.


d = self.do_create(recurse=3, dirs=2, files=2)
with d:
# Change files and directories flags recursively.
Expand Down
0