Skip to content

Commit

Permalink
bpo-42782: Fail fast for permission errors in shutil.move() (pythonGH…
Browse files Browse the repository at this point in the history
…-24001)

* Fail fast in shutil.move() to avoid creating destination directories on failure.

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
  • Loading branch information
winsonluk and ZackerySpytz authored Mar 2, 2021
1 parent b36349a commit 132131b
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 0 deletions.
11 changes: 11 additions & 0 deletions Lib/shutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,12 @@ def move(src, dst, copy_function=copy2):
if _destinsrc(src, dst):
raise Error("Cannot move a directory '%s' into itself"
" '%s'." % (src, dst))
if (_is_immutable(src)
or (not os.access(src, os.W_OK) and os.listdir(src)
and sys.platform == 'darwin')):
raise PermissionError("Cannot move the non-empty directory "
"'%s': Lacking write permission to '%s'."
% (src, src))
copytree(src, real_dst, copy_function=copy_function,
symlinks=True)
rmtree(src)
Expand All @@ -830,6 +836,11 @@ def _destinsrc(src, dst):
dst += os.path.sep
return dst.startswith(src)

def _is_immutable(src):
st = _stat(src)
immutable_states = [stat.UF_IMMUTABLE, stat.SF_IMMUTABLE]
return hasattr(st, 'st_flags') and st.st_flags in immutable_states

def _get_gid(name):
"""Returns a gid, given a group name."""
if getgrnam is None or name is None:
Expand Down
37 changes: 37 additions & 0 deletions Lib/test/test_shutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
from test.support.os_helper import TESTFN, FakePath

TESTFN2 = TESTFN + "2"
TESTFN_SRC = TESTFN + "_SRC"
TESTFN_DST = TESTFN + "_DST"
MACOS = sys.platform.startswith("darwin")
AIX = sys.platform[:3] == 'aix'
try:
Expand Down Expand Up @@ -2085,6 +2087,41 @@ def test_move_dir_caseinsensitive(self):
os.rmdir(dst_dir)


@unittest.skipUnless(hasattr(os, 'geteuid') and os.geteuid() == 0
and hasattr(os, 'lchflags')
and hasattr(stat, 'SF_IMMUTABLE')
and hasattr(stat, 'UF_OPAQUE'),
'root privileges required')
def test_move_dir_permission_denied(self):
# bpo-42782: shutil.move should not create destination directories
# if the source directory cannot be removed.
try:
os.mkdir(TESTFN_SRC)
os.lchflags(TESTFN_SRC, stat.SF_IMMUTABLE)

# Testing on an empty immutable directory
# TESTFN_DST should not exist if shutil.move failed
self.assertRaises(PermissionError, shutil.move, TESTFN_SRC, TESTFN_DST)
self.assertFalse(TESTFN_DST in os.listdir())

# Create a file and keep the directory immutable
os.lchflags(TESTFN_SRC, stat.UF_OPAQUE)
os_helper.create_empty_file(os.path.join(TESTFN_SRC, 'child'))
os.lchflags(TESTFN_SRC, stat.SF_IMMUTABLE)

# Testing on a non-empty immutable directory
# TESTFN_DST should not exist if shutil.move failed
self.assertRaises(PermissionError, shutil.move, TESTFN_SRC, TESTFN_DST)
self.assertFalse(TESTFN_DST in os.listdir())
finally:
if os.path.exists(TESTFN_SRC):
os.lchflags(TESTFN_SRC, stat.UF_OPAQUE)
os_helper.rmtree(TESTFN_SRC)
if os.path.exists(TESTFN_DST):
os.lchflags(TESTFN_DST, stat.UF_OPAQUE)
os_helper.rmtree(TESTFN_DST)


class TestCopyFile(unittest.TestCase):

class Faux(object):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fail fast in :func:`shutil.move()` to avoid creating destination directories on
failure.

0 comments on commit 132131b

Please sign in to comment.