Skip to content
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

Jump to bottom

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

Merged
merged 9 commits into from
Mar 2, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fe8f24a
Fail fast in shutil.move() to avoid creating destination directories …
winsonluk Dec 29, 2020
8826d00
Add NEWS entry
winsonluk Dec 29, 2020
a231ca6
Update Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst
winsonluk Jan 12, 2021
ee8073d
check if pytest is run as root
winsonluk Jan 30, 2021
7c11a09
Merge branch 'fix-shutil-move' of github.com:avocadoscan/cpython into…
winsonluk Jan 30, 2021
1082d79
add unit test
winsonluk Jan 31, 2021
6326e7c
update test
winsonluk Jan 31, 2021
79d6801
test empty immutable directories
winsonluk Jan 31, 2021
2567fb2
add platform check
winsonluk Feb 21, 2021
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
4 changes: 4 additions & 0 deletions Lib/shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,10 @@ def move(src, dst, copy_function=copy2):
if _destinsrc(src, dst):
raise Error("Cannot move a directory '%s' into itself"
" '%s'." % (src, dst))
if not os.access(src, os.W_OK) and os.listdir(src):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is the right list of permissions to check for and could potentially raise errors for move() operations that would have otherwise succeeded. I might be wrong, but I believe it's the parent directory's write permissions that matter.

Perhaps another approach would be to put copytree() and rmtree() in try except blocks, then cleaning up and raising an error accordingly on catching a permission error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review - sorry, I should have clarified what this line does.

Background: This check only runs when attempting to move directories, and OS move permissions are tied to the source directory, not its parent directory. The elif os.path.isdir(src): confirms that src is the directory being moved.

  • not os.access(src, os.W_OK) checks if src is write-enabled. We need this check not because we're writing to src, but because we need to delete it. Previously, if src was not write-enabled, rmtree would fail with a permission error.
  • os.listdir(src) checks if src is empty: this is important because if a user doesn't have write access to a directory, but the directory is empty, the directory can be removed by the user. Weird OS quirk, I know. So this check makes sure we don't raise an exception if the directory is empty (os.listdir(src) == []), because src can still be removed with rmtree despite the lack of write permissions.

I'm going to disagree with the try/except suggestion, since permission errors are easier to prevent than catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see that this check happens when handling an OSError after first trying to rename the directory, which was the scenario I thought it might block.

However, in that case, that's probably because the user does not have write access to the parent directory, so copytree() would throw a PermissionError before rmtree() is even run which means that the destination directory isn't created.

I'm not able to reproduce the error with the commands you provided, perhaps you could provide additional context for the permissions on the parent directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this situation only occurs if the user has permission to the parent directory, but not the src directory - this means the user can mkdir and cp within parent (with copytree succeeding), but can't rm src.

if the user doesn't have permission to the parent directory, you're correct that the operation would fail at copytree.

here's the example where copy succeeds, but rm fails

[01/12/21 12:38:47] $ ls -haltR
total 0
drwxr-xr-x@  3 wluk  staff    96B Jan 12 12:37 parent
drwxr-xr-x@  3 wluk  staff    96B Jan 12 12:35 .
drwxr-xr-x@ 39 wluk  staff   1.2K Jan 12 12:34 ..

./parent:
total 0
drwxr-xr-x@ 3 root  staff    96B Jan 12 12:38 src
drwxr-xr-x@ 3 wluk  staff    96B Jan 12 12:37 .
drwxr-xr-x@ 3 wluk  staff    96B Jan 12 12:35 ..

./parent/src:
total 0
drwxr-xr-x@ 3 root  staff    96B Jan 12 12:38 .
-rw-r--r--  1 root  staff     0B Jan 12 12:38 child
drwxr-xr-x@ 3 wluk  staff    96B Jan 12 12:37 ..

Running move():

>>> shutil.move('src', 'dst')
Traceback (most recent call last):
  File "/usr/local/Cellar/python@3.9/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 806, in move
    os.rename(src, real_dst)
PermissionError: [Errno 13] Permission denied: 'src' -> 'dst'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Cellar/python@3.9/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 818, in move
    rmtree(src)
  File "/usr/local/Cellar/python@3.9/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 718, in rmtree
    _rmtree_safe_fd(fd, path, onerror)
  File "/usr/local/Cellar/python@3.9/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 675, in _rmtree_safe_fd
    onerror(os.unlink, fullname, sys.exc_info())
  File "/usr/local/Cellar/python@3.9/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 673, in _rmtree_safe_fd
    os.unlink(entry.name, dir_fd=topfd)
PermissionError: [Errno 13] Permission denied: 'child'

End state:

[01/12/21 12:44:03] ~/.../2020/test $ ls -haltR
total 0
drwxr-xr-x@  3 wluk  staff    96B Jan 12 12:43 .
drwxr-xr-x@ 39 wluk  staff   1.2K Jan 12 12:43 ..
drwxr-xr-x@  4 wluk  staff   128B Jan 12 12:43 parent

./parent:
total 0
drwxr-xr-x@ 4 wluk  staff   128B Jan 12 12:43 .
drwxr-xr-x@ 3 wluk  staff    96B Jan 12 12:43 ..
drwxr-xr-x@ 3 wluk  staff    96B Jan 12 12:43 dst
drwxr-xr-x@ 3 root  staff    96B Jan 12 12:38 src

./parent/dst:
total 0
drwxr-xr-x@ 3 wluk  staff    96B Jan 12 12:43 .
drwxr-xr-x@ 4 wluk  staff   128B Jan 12 12:43 ..
-rw-r--r--  1 wluk  staff     0B Jan 12 12:38 child

./parent/src:
total 0
drwxr-xr-x@ 4 wluk  staff   128B Jan 12 12:43 ..
drwxr-xr-x@ 3 root  staff    96B Jan 12 12:38 .
-rw-r--r--  1 root  staff     0B Jan 12 12:38 child

Copy link
Member

Choose a reason for hiding this comment

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

Hi @winsonluk

I looked a bit deeper, and I understand the theory in your PR, but I am unable to come out with a scenario that will reproduce the error in your original scenario.

The parent permissions seem to satisfy the requirements.

Here I tried creating the dir permissions as above.

$ ls -haltR
.:
total 12K
drwxr-xr-x 3 senthil senthil 4.0K Jan 31 06:32 parent
drwxr-xr-x 3 senthil senthil 4.0K Jan 31 06:29 .
drwxr-xr-x 3 senthil senthil 4.0K Jan 31 06:28 ..

./parent:
total 12K
drwxr-xr-x 3 senthil senthil 4.0K Jan 31 06:32 .
drwxr-xr-x 2 root    senthil 4.0K Jan 31 06:31 src
drwxr-xr-x 3 senthil senthil 4.0K Jan 31 06:29 ..

./parent/src:
total 8.0K
drwxr-xr-x 3 senthil senthil 4.0K Jan 31 06:32 ..
drwxr-xr-x 2 root    senthil 4.0K Jan 31 06:31 .
-rw-r--r-- 1 root    root       0 Jan 31 06:31 child

And

$ cd parent/
$ ~/3.9/cpython/bin/python3
Python 3.9.0+ (heads/3.9:f22f874a66, Oct 22 2020, 10:26:55) 
[GCC 9.2.1 20191008] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import shutil
>>> shutil.move('src', 'dst')
'dst'

Is it possible for you to share set of instructions (including chmod/chown instruction) that can help reproduce this issue to verify? FWIW, I used https://bugs.python.org/msg384023 and didn't see this scenario that you encountered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orsenthil Huh, good catch. I've been testing this on MacOS darwin machines (python3 installed with Homebrew). I just retried this on some spare linux, freebsd, and win32 servers, and the file was moved successfully in all cases.

This is probably a darwin-only bug, so it's only reproducible on MacOS.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably a darwin-only bug, so it's only reproducible on MacOS.

I wonder if this is a MacOS bug. It's strange not to see this behavior in FreeBSD, Linux and Win32 and only in MacOS. And we are not even talking about Python implementation. The problem should be observable with Darwin's mv utility too.

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 not sure if it's a bug or intentional, but I can confirm that Darwin's mv fails when other OS's succeed (e.g., mv src dst). So while this PR is necessary to mitigate Darwin's irregular behavior, it doesn't impact other operating systems.

winsonluk marked this conversation as resolved.
Show resolved Hide resolved
winsonluk marked this conversation as resolved.
Show resolved Hide resolved
raise Error("Cannot move the non-empty directory '%s': "
winsonluk marked this conversation as resolved.
Show resolved Hide resolved
"Lacking write permission to '%s'."
% (src, src))
copytree(src, real_dst, copy_function=copy_function,
symlinks=True)
rmtree(src)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fail fast in shutil.move() to avoid creating destination directories on
winsonluk marked this conversation as resolved.
Show resolved Hide resolved
failure.