bpo-43153: Don't mask PermissionError with NotADirectoryError during tempdirectory cleanup

[Fidget-Spinner]

Co-Authored-By: andrei kulakov andrei.avk@gmail.com

https://bugs.python.org/issue43153

#87319

[Fidget-Spinner]

I've tested locally and it works on Windows.

[akulakov]

Approving with a minor request to update a comment

[akulakov]

I think this would also happen on other OSes, not just Windows. The bug can only be reproduced on windows because PermissionError in the bug was caused by an open file, but PermissionError in theory can be caused by actual permission / ownership settings. It shouldn't matter much because this is a temp dir (and files in it) created by current user, but this comment can be misunderstood, so it's better to drop ref. to Windows here.

It may be left in the if case below because it can avoid the isdir() check.

It would be good to also add "Issue #43153" to the comment.

[eryksun]

Note that calling unlink() on a directory in Windows always raises PermissionError. Windows maps the NT status code STATUS_FILE_IS_A_DIRECTORY to the WinAPI error code ERROR_ACCESS_DENIED.

[Fidget-Spinner]

I think this would also happen on other OSes, not just Windows. The bug can only be reproduced on windows

I'm confused, should we continue the _os.name == 'nt' check then? I'm not worried about the additional performance hit from isdir (though I would like to avoid it). Correctness is more important. If this does happen on other OSes, then we should remove that OS check.

[akulakov]

We can remove the os check. It can happen on other OSes. With the check removed, the logic becomes "if we are not permitted to remove the file, return or reraise", which makes perfect sense.

[akulakov]

It might also be good to run this on all windows build bots to confirm that unit test doesn't fail on any windows version we support. The code itself should be fine but unit test may fail if some windows version doesn't raise permission error in this case.

[akulakov]

Please also update the PR title IsAdir.. => NotADir...

[Fidget-Spinner]

Azure pipelines failure is unrelated and fixed by GH-29963.

[akulakov]

We can probably just keep the unit test for windows and skip it on other OSes. Other OSes would need different ways of causing a PermissionError, so it's not worth the effort. [Edit: for example in my recent testing, on MacOS it's not enough to have a file with no permissions, it is the containing dir that has to lack permissions for the file to raise PermissionError].

[Fidget-Spinner]

We can probably just keep the unit test for windows and skip it on other OSes. Other OSes would need different ways of causing a PermissionError, so it's not worth the effort. [Edit: for example in my recent testing, on MacOS it's not enough to have a file with no permissions, it is the containing dir that has to lack permissions for the file to raise PermissionError).

Yep, I noticed the other tests are starting to fail. Seems like it's not possible to repro this on the other OSes. I'll make sure to test with buildbots before we merge this PR (and once that docfix PR lands).

BTW, thanks for guiding me throughout this PR :). I'm admittedly not familiar with this part of the stdlib. Thanks to Victor and Eryk too!

[akulakov]

BTW, thanks for guiding me throughout this PR :). I'm admittedly not familiar with this part of the stdlib.

Glad I could help! I haven't done that much PR reviews yet so it was great to get that experience too, especially with the logic I was already a bit familiar with. Please add me as a reviewer in the future on similar PRs!

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 499e424 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[serhiy-storchaka]

os.path.isdir() returns True for a symlink to directory. Either check os.path.islink() before os.path.isdir() or use os.stat() explicitly.

[eryksun]

Failing to delete a symlink should be rare in a temp directory. But if it's worth handling here, then it should be consistent with shutil._rmtree_isdir(). Unfortunately the latter is written for os.DirEntry instances. Here's an updated implementation for shutil that can also check a path. I factored out the check for a mount point into a common _st_ismount(st) function, which avoids duplicated code and, I think, makes it easier to read.

if not hasattr(os.stat_result, 'st_reparse_tag'):
    def _rmtree_islink(path):
        return os.path.islink(path)

    def _rmtree_isdir(path):
        try:
            if isinstance(path, os.DirEntry):
                return entry.is_dir(follow_symlinks=False)
            return stat.S_ISDIR(os.lstat(path).st_mode)
        except OSError:
            return False
else:
    # In general mount points in Windows are directories, but os.unlink()
    # supports them, so rmtree() handles them as symlinks.
    def _st_ismount(st):
        return (st.st_file_attributes & stat.FILE_ATTRIBUTE_REPARSE_POINT
                and st.st_reparse_tag == stat.IO_REPARSE_TAG_MOUNT_POINT)

    def _rmtree_islink(path):
        try:
            if isinstance(path, os.DirEntry):
                st = entry.stat(follow_symlinks=False)
            else:
                st = os.lstat(path)
            return stat.S_ISLNK(st.st_mode) or _st_ismount(st)
        except OSError:
            return False

    def _rmtree_isdir(path):
        try:
            if isinstance(path, os.DirEntry):
                st = entry.stat(follow_symlinks=False)
            else:
                st = os.lstat(path)
            return stat.S_ISDIR(st.st_mode) and not _st_ismount(st)
        except OSError:
            return False

[eryksun]

Note that this check is inconsistent with shutil.rmtree(). os.unlink() deletes a mount point in Windows, but otherwise a mount point is handled as a directory, i.e. islink() will be false and isdir() will be true. However, they're special cased by shutil._rmtree_isdir() and shutil._rmtree_islink(), and thus shutil.rmtree() will immediately fail with OSError("Cannot call rmtree on a symbolic link"), as would happen for a regular symbolic link, which is exactly what this check is supposed to prevent.

[Fidget-Spinner]

To clarify, so are you suggesting we:

1. Modify shutil._rmtree_isdir with your suggested changes here bpo-43153: Don't mask PermissionError with NotADirectoryError during tempdirectory cleanup #29940 (comment).
2. Use shutil._rmtree_isdir for a more consistent check instead of our current method?

[eryksun]

I know it's a bad practice to rely on a private function from another module. But duplicating the code would be a maintenance burden, and it's not useful enough to expose in os.path. Maybe _rmtree_isdir() and _rmtree_islink() should be refactored into a common private module. I'm curious to know what @serhiy-storchaka thinks should be done about the consistency problem between these functions and os.path.islink() and os.path.isdir().

[Fidget-Spinner]

I'm perfectly fine with using private functions from another module as long as it's within the stdlib itself and not exposed somewhere :). Thank you for the clarification, I wanted to summarize your suggestions since I wasn't sure I understood you correctly.

[akulakov]

I think it should be okay to use shutil._rmtree_isdir() here because we're asking shutil if it's valid to use rmtree on a path, and then telling shutil to use shutil._rmtree(). So these two steps of logic stay completely internal to shutil even though used outside of it. And it's also consistent with current logic of using internal shutil._rmtree().

[serhiy-storchaka]

_rmtree_isdir() and _rmtree_islink() was changed between versions, and this fix should be applied to multiple Python versions. So it is better to inline them here. If possible, we should use public API (add it if it does not exist).

[miss-islington-app]

Thanks @Fidget-Spinner for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

[bedevere-app]

GH-112753 is a backport of this pull request to the 3.12 branch.

[bedevere-app]

GH-112754 is a backport of this pull request to the 3.11 branch.

[Fidget-Spinner]

Thanks @serhiy-storchaka for bringing this over the finish line!

[serhiy-storchaka]

Unfortunately, I failed to create tests for junction points (which would fail without special handling of junction points in tempfile). But the code looks right.

[proc-2]

Why not go with an "ask-for-forgiveness-rather-than-permission" type of solution, e.g.:

try:
    _os.unlink(path)
except IsADirectoryError:
    cls._rmtree(path, ignore_errors=ignore_errors)
except PermissionError as pe:
    # PermissionError is raised on FreeBSD for directories
    # and by Windows on lock files used by other processes
    try:
        cls._rmtree(path, ignore_errors=ignore_errors)
    except NotADirectoryError:
        # NOTE: This is raised if PermissionError did not
        # correspond to a IsADirectoryError, e.g. on
        # Windows.
        if not ignore_errors:
            raise pe

[serhiy-storchaka]

rmtree() is a complex function, and we cannot be sure whether NotADirectoryError was raised by the toplevel scandir() or somewhere deeper in the tree.

[proc-2]

Mmmh...

[proc-2]

Seems like a responsibility inversion to me. Sure there could be a bug deep down in _rmtree that ends up bubbling an unrelated NotADirectoryError, but that would technically be a bug in _rmtree. Maybe there are valid cases where such an unrelated error would be raised?

Also I see the diff I commented on is not the final version so I'll look at the final version.

[serhiy-storchaka]

Not necessary a bug. It may be a race condition. Although the LBYL approach is also prone to race conditions, I think it has less chance to override error with a wrong exception.

I myself prefer the EAFP approach, but I do not think that it has advantages in this case.