Skip to content

bpo-42782: Fail fast for permission errors in shutil.move() #24001

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 9 commits into from
Mar 2, 2021
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))
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is this src to src ? Shouldn't the verification in line 816 be on dst?

  2. A general question - if this applicable only to macos, should we need OS platform check? - I am not certain on this.

Copy link
Contributor Author

@winsonluk winsonluk Feb 21, 2021

Choose a reason for hiding this comment

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

  1. src can't be moved to dst if: (src is not empty) && (user lacks write permission to src). So we're only checking the status of src here.
  2. Good catch, the immutability check is applicable to all platforms, but the empty/permissions check is only applicable to darwin. I've added a sys.platform == 'darwin'.

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.