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

winsonluk · 2020-12-29T18:39:40Z

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

https://bugs.python.org/issue42782

shutil.move calls shutil.copytree(), then shutil.rmtree() (in that order). If the user does not have permission to delete the source directory, copytree succeeds but rmtree fails. The user sees an error (Permission Denied), but the destination directory is still created. The expected behavior should be a Permission Denied without the creation of the destination directory.

To reproduce:

$ mkdir foo
$ sudo chown root foo
$ sudo touch foo/child
$ python3
>>> import shutil
>>> shutil.move('foo', 'bar')
PermissionError
$ ls foo
child
$ ls bar
child

If shutil.move() encountered a permission error and failed, bar/ should not have been created. This PR changes the shutil.move behavior to return an error before shutil.copytree() is called, preventing the creation of the destination directory on failure.

…on failure

winsonluk · 2020-12-29T19:32:24Z

@hynek @giampaolo @zooba

hey, saw you folks were active with shutil - I'd really appreciate a review, thanks!

desmondcheongzx · 2021-01-08T16:45:36Z

Lib/shutil.py

@@ -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):


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.

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.

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?

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/[email protected]/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/[email protected]/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 818, in move rmtree(src) File "/usr/local/Cellar/[email protected]/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/[email protected]/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/[email protected]/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

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.

@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.

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.

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.

ZackerySpytz

Please add unit tests.

I share Desmond's doubts regarding this change.

Lib/shutil.py

Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst

Co-authored-by: Zackery Spytz <[email protected]>

winsonluk · 2021-01-12T13:55:15Z

Please add unit tests.

I share Desmond's doubts regarding this change.

Concerns addressed in comments. I don't think it's possible to test for file ownership (only file permissions), since the tests are not run as root, but please correct me if I'm wrong.

cc @ZackerySpytz @desmondcheongzx

gvanrossum · 2021-01-28T21:17:24Z

I'm not familiar with this code any more, but shouldn't this be accompanied by a test? The test repro in the bpo issue seems straightforward enough (and could all be done using the os module).

Lib/shutil.py

… fix-shutil-move

winsonluk · 2021-01-31T08:21:11Z

@ZackerySpytz @gvanrossum @orsenthil I appreciate all your concerns for unit tests 😉 ...I've already mentioned this in another conversation, but I just want to reiterate. A successful test needs (1) a folder created by root, and (2) shutil.move run as a non-root user.

If we run tests as root, we can do (1) and try os.setuid(uid), but the latter command will break the test suite since the command is non-reversible. There's not really any other way to run non-privileged Python as root.
If we run tests as a user, we can't create a root-owned directory for testing. One possibility is testing shutil.move with root system directories (e.g., assertRaises(PermissionError, shutil.move, '/opt', dst), but this isn't a cross-platform or confidence-inspiring solution.

However, I've updated this PR to include immutability flag checks, thanks to concerns from @eryksun . This we can test for since root users are affected by immutability. I'd appreciate another review when you get a chance, thanks!

winsonluk · 2021-01-31T16:40:42Z

Update: This bpo and PR only affects MacOS darwin. Thanks to @orsenthil for flagging this.

winsonluk · 2021-02-19T11:18:34Z

Hey @orsenthil @gvanrossum, I'd appreciate another review when you get the chance, thanks!

We've added test cases to confirm that this is still an issue in the latest build which needs fixing. We've also determined that this bug affects moving immutable directories too, though only on darwin (MacOS).

orsenthil · 2021-02-19T12:08:59Z

Lib/shutil.py

+ or _is_immutable(src)):
+ raise PermissionError("Cannot move the non-empty directory "
+ "'%s': Lacking write permission to '%s'."
+ % (src, src))


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

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

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.

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'.

orsenthil · 2021-03-02T20:52:26Z

Hi Winson, I've review this patch and tests, and changes look good to me. I will merge this. Since this is not a security issue, it will be applied only to current and bugfix releases of Python, that is, master, 3.9, and 3.8 as of March 2021. (Ref: https://devguide.python.org/)

miss-islington · 2021-03-02T20:53:28Z

Thanks @winsonluk for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2021-03-02T20:53:28Z

Thanks @winsonluk for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-03-02T20:53:50Z

GH-24709 is a backport of this pull request to the 3.8 branch.

…-24001) * Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz <[email protected]> (cherry picked from commit 132131b) Co-authored-by: Winson Luk <[email protected]>

bedevere-bot · 2021-03-02T20:53:57Z

GH-24710 is a backport of this pull request to the 3.9 branch.

…-24001) * Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz <[email protected]> (cherry picked from commit 132131b) Co-authored-by: Winson Luk <[email protected]>

miss-islington · 2021-03-02T21:16:12Z

Thanks @winsonluk for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2021-03-02T21:16:21Z

GH-24711 is a backport of this pull request to the 3.8 branch.

…-24001) * Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz <[email protected]> (cherry picked from commit 132131b) Co-authored-by: Winson Luk <[email protected]>

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

miss-islington · 2021-03-03T15:16:45Z

Thanks @winsonluk for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2021-03-03T15:16:45Z

Thanks @winsonluk for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington · 2021-03-03T15:16:50Z

Sorry @winsonluk and @orsenthil, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 132131b404e06ee1a19b040a1f96cd1118abed0c 3.9

bedevere-bot · 2021-03-03T15:17:05Z

GH-24724 is a backport of this pull request to the 3.8 branch.

…-24001) * Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz <[email protected]> (cherry picked from commit 132131b) Co-authored-by: Winson Luk <[email protected]>

* Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz <[email protected]> (cherry picked from commit 132131b) Co-authored-by: Winson Luk <[email protected]>

…-24001) (#24725) * Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz <[email protected]> (cherry picked from commit 132131b) Co-authored-by: Winson Luk <[email protected]> Co-authored-by: Winson Luk <[email protected]>

Fail fast in shutil.move() to avoid creating destination directories …

fe8f24a

…on failure

the-knights-who-say-ni added the CLA signed label Dec 29, 2020

bedevere-bot added the awaiting review label Dec 29, 2020

Add NEWS entry

8826d00

winsonluk mentioned this pull request Dec 30, 2020

Fix package permissions pypa/pip#9393

Closed

desmondcheongzx reviewed Jan 8, 2021

View reviewed changes

ZackerySpytz reviewed Jan 9, 2021

View reviewed changes

Lib/shutil.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst

a231ca6

Co-authored-by: Zackery Spytz <[email protected]>

winsonluk changed the title ~~bpo-42782: Fail fast in shutil.move() to avoid creating destination directories on failure~~ bpo-42782: Fail fast for permission errors in shutil.move() Jan 12, 2021

winsonluk requested a review from merwok January 28, 2021 19:06

eryksun reviewed Jan 28, 2021

View reviewed changes

Lib/shutil.py Outdated Show resolved Hide resolved

orsenthil reviewed Jan 29, 2021

View reviewed changes

Lib/shutil.py Outdated Show resolved Hide resolved

winsonluk added 5 commits January 30, 2021 15:46

check if pytest is run as root

ee8073d

Merge branch 'fix-shutil-move' of github.com:avocadoscan/cpython into…

7c11a09

… fix-shutil-move

add unit test

1082d79

update test

6326e7c

test empty immutable directories

79d6801

winsonluk requested a review from orsenthil January 31, 2021 08:21

orsenthil reviewed Feb 19, 2021

View reviewed changes

add platform check

2567fb2

winsonluk requested a review from orsenthil February 21, 2021 05:03

orsenthil merged commit 132131b into python:master Mar 2, 2021

bedevere-bot removed the awaiting review label Mar 2, 2021

orsenthil added needs backport to 3.8 needs backport to 3.9 only security fixes labels Mar 2, 2021

bedevere-bot removed the needs backport to 3.8 label Mar 2, 2021

bedevere-bot removed the needs backport to 3.9 only security fixes label Mar 2, 2021

orsenthil added the needs backport to 3.8 label Mar 2, 2021

bedevere-bot removed the needs backport to 3.8 label Mar 2, 2021

sthagen added a commit to sthagen/python-cpython that referenced this pull request Mar 2, 2021

Merge pull request #413 from python/master

6f846e2

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

orsenthil added needs backport to 3.8 needs backport to 3.9 only security fixes labels Mar 3, 2021

miss-islington assigned orsenthil Mar 3, 2021

bedevere-bot removed the needs backport to 3.8 label Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

winsonluk commented Dec 29, 2020 •

edited

Loading

winsonluk commented Dec 29, 2020

desmondcheongzx Jan 8, 2021

winsonluk Jan 12, 2021

desmondcheongzx Jan 12, 2021

winsonluk Jan 12, 2021

orsenthil Jan 31, 2021

winsonluk Jan 31, 2021

orsenthil Jan 31, 2021

winsonluk Feb 1, 2021

ZackerySpytz left a comment

winsonluk commented Jan 12, 2021

gvanrossum commented Jan 28, 2021

winsonluk commented Jan 31, 2021

winsonluk commented Jan 31, 2021

winsonluk commented Feb 19, 2021

orsenthil Feb 19, 2021

winsonluk Feb 21, 2021 •

edited

Loading

orsenthil commented Mar 2, 2021

miss-islington commented Mar 2, 2021

miss-islington commented Mar 2, 2021

bedevere-bot commented Mar 2, 2021

bedevere-bot commented Mar 2, 2021

miss-islington commented Mar 2, 2021

bedevere-bot commented Mar 2, 2021

miss-islington commented Mar 3, 2021

miss-islington commented Mar 3, 2021

miss-islington commented Mar 3, 2021

bedevere-bot commented Mar 3, 2021

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

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

Conversation

winsonluk commented Dec 29, 2020 • edited Loading

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

winsonluk commented Dec 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackerySpytz left a comment

Choose a reason for hiding this comment

winsonluk commented Jan 12, 2021

gvanrossum commented Jan 28, 2021

winsonluk commented Jan 31, 2021

winsonluk commented Jan 31, 2021

winsonluk commented Feb 19, 2021

Choose a reason for hiding this comment

winsonluk Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

orsenthil commented Mar 2, 2021

miss-islington commented Mar 2, 2021

miss-islington commented Mar 2, 2021

bedevere-bot commented Mar 2, 2021

bedevere-bot commented Mar 2, 2021

miss-islington commented Mar 2, 2021

bedevere-bot commented Mar 2, 2021

miss-islington commented Mar 3, 2021

miss-islington commented Mar 3, 2021

miss-islington commented Mar 3, 2021

bedevere-bot commented Mar 3, 2021

winsonluk commented Dec 29, 2020 •

edited

Loading

winsonluk Feb 21, 2021 •

edited

Loading