Skip to content

Commit

Permalink
pythongh-94692: Only catch OSError in shutil.rmtree() (python#112756)
Browse files Browse the repository at this point in the history
Previously a symlink attack resistant version of shutil.rmtree() could ignore
or pass to the error handler arbitrary exception when invalid arguments
were provided.
  • Loading branch information
serhiy-storchaka committed Dec 5, 2023
1 parent bc68f4a commit 563ccde
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 19 deletions.
4 changes: 2 additions & 2 deletions Lib/shutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -768,13 +768,13 @@ def onexc(*args):
# lstat()/open()/fstat() trick.
try:
orig_st = os.lstat(path, dir_fd=dir_fd)
except Exception as err:
except OSError as err:
onexc(os.lstat, path, err)
return
try:
fd = os.open(path, os.O_RDONLY, dir_fd=dir_fd)
fd_closed = False
except Exception as err:
except OSError as err:
onexc(os.open, path, err)
return
try:
Expand Down
33 changes: 16 additions & 17 deletions Lib/test/test_shutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ def test_rmtree_works_on_junctions(self):
self.assertTrue(os.path.exists(dir3))
self.assertTrue(os.path.exists(file1))

def test_rmtree_errors_onerror(self):
def test_rmtree_errors(self):
# filename is guaranteed not to exist
filename = tempfile.mktemp(dir=self.mkdtemp())
self.assertRaises(FileNotFoundError, shutil.rmtree, filename)
Expand All @@ -326,15 +326,28 @@ def test_rmtree_errors_onerror(self):

# existing file
tmpdir = self.mkdtemp()
write_file((tmpdir, "tstfile"), "")
filename = os.path.join(tmpdir, "tstfile")
write_file(filename, "")
with self.assertRaises(NotADirectoryError) as cm:
shutil.rmtree(filename)
self.assertEqual(cm.exception.filename, filename)
self.assertTrue(os.path.exists(filename))
# test that ignore_errors option is honored
shutil.rmtree(filename, ignore_errors=True)
self.assertTrue(os.path.exists(filename))

self.assertRaises(TypeError, shutil.rmtree, None)
self.assertRaises(TypeError, shutil.rmtree, None, ignore_errors=True)
exc = TypeError if shutil.rmtree.avoids_symlink_attacks else NotImplementedError
with self.assertRaises(exc):
shutil.rmtree(filename, dir_fd='invalid')
with self.assertRaises(exc):
shutil.rmtree(filename, dir_fd='invalid', ignore_errors=True)

def test_rmtree_errors_onerror(self):
tmpdir = self.mkdtemp()
filename = os.path.join(tmpdir, "tstfile")
write_file(filename, "")
errors = []
def onerror(*args):
errors.append(args)
Expand All @@ -350,23 +363,9 @@ def onerror(*args):
self.assertEqual(errors[1][2][1].filename, filename)

def test_rmtree_errors_onexc(self):
# filename is guaranteed not to exist
filename = tempfile.mktemp(dir=self.mkdtemp())
self.assertRaises(FileNotFoundError, shutil.rmtree, filename)
# test that ignore_errors option is honored
shutil.rmtree(filename, ignore_errors=True)

# existing file
tmpdir = self.mkdtemp()
write_file((tmpdir, "tstfile"), "")
filename = os.path.join(tmpdir, "tstfile")
with self.assertRaises(NotADirectoryError) as cm:
shutil.rmtree(filename)
self.assertEqual(cm.exception.filename, filename)
self.assertTrue(os.path.exists(filename))
# test that ignore_errors option is honored
shutil.rmtree(filename, ignore_errors=True)
self.assertTrue(os.path.exists(filename))
write_file(filename, "")
errors = []
def onexc(*args):
errors.append(args)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
:func:`shutil.rmtree` now only catches OSError exceptions. Previously a
symlink attack resistant version of ``shutil.rmtree()`` could ignore or pass
to the error handler arbitrary exception when invalid arguments were
provided.

0 comments on commit 563ccde

Please sign in to comment.