classification
Title: TemporaryDirectory clean-up fails with unsearchable directories
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: eryksun, lilydjwg, serhiy.storchaka, vidartf, xtreak
Priority: normal Keywords: patch

Created on 2018-11-02 08:40 by lilydjwg, last changed 2020-09-23 04:29 by eryksun.

Pull Requests
URL Status Linked Edit
PR 10320 merged serhiy.storchaka, 2018-11-04 14:44
Messages (9)
msg329116 - (view) Author: lilydjwg (lilydjwg) * Date: 2018-11-02 08:40
If the title doesn't explain clearly, here's a demo program that will fail:

import tempfile
import pathlib

def test():
  with tempfile.TemporaryDirectory(prefix='test-bad-') as tmpdir:
    tmpdir = pathlib.Path(tmpdir)
    subdir = tmpdir / 'sub'
    subdir.mkdir()
    with open(subdir / 'file', 'w'):
      pass
    subdir.chmod(0o600)

if __name__ == '__main__':
  test()

I didn't expect this, and I didn't find an easy way to handle this except not using TemporaryDirectory at all:

import tempfile
import pathlib
import shutil
import os

def rmtree_error(func, path, excinfo):
  if isinstance(excinfo[1], PermissionError):
    os.chmod(os.path.dirname(path), 0o700)
    os.unlink(path)
  print(func, path, excinfo)

def test():
  tmpdir = tempfile.mkdtemp(prefix='test-good-')
  try:
    tmpdir = pathlib.Path(tmpdir)
    subdir = tmpdir / 'sub'
    subdir.mkdir()
    with open(subdir / 'file', 'w'):
      pass
    subdir.chmod(0o600)
  finally:
    shutil.rmtree(tmpdir, onerror=rmtree_error)

if __name__ == '__main__':
  test()

This works around the issue, but the dirfd is missing in the onerror callback.

I have this issue because my program extracts tarballs to a temporary directory for examination. I expected that TemporaryDirectory cleaned up things when it could.

What do you think? rm -rf can't remove such a directory either but this is annoying and I think Python can do better.
msg329127 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2018-11-02 11:41
Seems like a related issue : issue26660 . Maybe TemporaryDirectory can allow an onerror argument that is passed internally to rmtree during cleanup and state the same in the documentation that TemporaryDirectory can't cleanup read-only files?
msg329184 - (view) Author: lilydjwg (lilydjwg) * Date: 2018-11-03 06:50
Yes issue26660 is similar but not the same. On Windows it seems a read-only file cannot be deleted while on Linux a file resident in a non-searchable directory cannot be deleted.

An onerror for TemporaryDirectory will work. Also I'd like to add an optional dir_fd argument for onerror.
msg329185 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-03 06:52
I am working on this. Left to test on Windows and analyze possible security issues.
msg344036 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-05-31 08:30
New changeset e9b51c0ad81da1da11ae65840ac8b50a8521373c by Serhiy Storchaka in branch 'master':
bpo-26660, bpo-35144: Fix permission errors in TemporaryDirectory cleanup. (GH-10320)
https://github.com/python/cpython/commit/e9b51c0ad81da1da11ae65840ac8b50a8521373c
msg377309 - (view) Author: Vidar Fauske (vidartf) * Date: 2020-09-22 10:41
On Python 3.8.5 on Windows using the code from the above patch I recently got a stack overflow:

Thread 0x00002054 (most recent call first):
  File "...\lib\concurrent\futures\thread.py", line 78 in _worker
  File "...\lib\threading.py", line 870 in run
  File "...\lib\threading.py", line 932 in _bootstrap_inner
  File "...\lib\threading.py", line 890 in _bootstrap

Thread 0x00000de4 (most recent call first):
  File "...\lib\concurrent\futures\thread.py", line 78 in _worker
  File "...\lib\threading.py", line 870 in run
  File "...\lib\threading.py", line 932 in _bootstrap_inner
  File "...\lib\threading.py", line 890 in _bootstrap

Current thread 0x00004700 (most recent call first):
  File "...\lib\tempfile.py", line 803 in onerror
  File "...\lib\shutil.py", line 619 in _rmtree_unsafe
  File "...\lib\shutil.py", line 737 in rmtree
  File "...\lib\tempfile.py", line 814 in _rmtree
  File "...\lib\tempfile.py", line 806 in onerror
  File "...\lib\shutil.py", line 619 in _rmtree_unsafe
  File "...\lib\shutil.py", line 737 in rmtree
  ... repeating

-------------------------------------------


In my case, the outer `exc_info` from rmtree is:

PermissionError(13, 'The process cannot access the file because it is being used by another process')


And the inner exception from `_os.unlink(path)` is:

PermissionError(13, 'Access is denied')



I would say that expected behavior in this case would be to let the 'file is in use' error raise, instead of killing the process with an SO.
msg377316 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-09-22 12:27
Thank you Vidar! I wasn't sure about Windows, but was not able to reproduce possible failures. Your report gives a clue.
msg377329 - (view) Author: Vidar Fauske (vidartf) * Date: 2020-09-22 14:39
A somewhat easy repro:

Create the temporary directory, add a subdir (not sure if subdir truly necessary at this point), use `os.chdir()` to set the cwd to that subdir. Clean up the temp dir. The cwd should prevent the deletion because it will be "in use".
msg377358 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-09-23 04:29
It seems to me that if `path == name`, then resetperms(path) and possibly a recursive call are only needed on the first call. In subsequent calls, if `path == name`, then we know that resetperms(path) was already called, so it shouldn't handle PermissionError. If resetperms was ineffective (e.g. in Windows, a sharing violation or custom discretionary/mandatory permissions), or if something else changed the permissions in the mean time, just give up instead of risking a RecursionError or stack overflow. For example:


    @classmethod
    def _rmtree(cls, name, first_call=True):
        resetperms_funcs = (_os.unlink, _os.rmdir, _os.scandir, _os.open)

        def resetperms(path):
            try:
                _os.chflags(path, 0)
            except AttributeError:
                pass
            _os.chmod(path, 0o700)

        def onerror(func, path, exc_info):
            if (issubclass(exc_info[0], PermissionError) and
                  func in resetperms_funcs and (first_call or path != name)):
                try:
                    if path != name:
                        resetperms(_os.path.dirname(path))
                    resetperms(path)
                    try:
                        _os.unlink(path)
                    # PermissionError is raised on FreeBSD for directories
                    except (IsADirectoryError, PermissionError):
                        cls._rmtree(path, first_call=False)
                except FileNotFoundError:
                    pass
            elif issubclass(exc_info[0], FileNotFoundError):
                pass
            else:
                raise

        _shutil.rmtree(name, onerror=onerror)
History
Date User Action Args
2020-09-23 04:29:36eryksunsetnosy: + eryksun
messages: + msg377358
2020-09-22 14:39:10vidartfsetmessages: + msg377329
2020-09-22 12:27:21serhiy.storchakasetmessages: + msg377316
2020-09-22 10:41:45vidartfsetnosy: + vidartf
messages: + msg377309
2019-05-31 08:30:40serhiy.storchakasetmessages: + msg344036
2018-11-04 14:44:15serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request9622
2018-11-03 06:52:53serhiy.storchakasetmessages: + msg329185
2018-11-03 06:50:33lilydjwgsetmessages: + msg329184
2018-11-02 20:32:05terry.reedysettitle: TemporaryDirectory can't be cleaned up if there are unsearchable directories -> TemporaryDirectory clean-up fails with unsearchable directories
2018-11-02 12:24:05serhiy.storchakasetassignee: serhiy.storchaka

nosy: + serhiy.storchaka
2018-11-02 11:41:10xtreaksetmessages: + msg329127
2018-11-02 11:19:05xtreaksetnosy: + xtreak
2018-11-02 08:40:08lilydjwgcreate