classification
Title: Python 3.8 regression: endless loop in shutil.copytree
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cboltz, giampaolo.rodola, kinow
Priority: normal Keywords: 3.8regression, patch

Created on 2019-11-04 21:21 by cboltz, last changed 2019-11-27 04:50 by giampaolo.rodola. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 17098 merged kinow, 2019-11-09 13:25
PR 17397 merged kinow, 2019-11-27 02:11
Messages (10)
msg355981 - (view) Author: Christian Boltz (cboltz) Date: 2019-11-04 21:21
The following test script works with Python 3.7 (and older), but triggers an endless loop with Python 3.8:


#!/usr/bin/python3

import shutil
import os

os.mkdir('/dev/shm/t')
os.mkdir('/dev/shm/t/pg')

with open('/dev/shm/t/pg/pol', 'w+') as f:
    f.write('pol')

shutil.copytree('/dev/shm/t/pg', '/dev/shm/t/pg/somevendor/1.0')


The important point is probably that 'pg' gets copied into a subdirectory of itsself. While this worked in Python up to 3.7, doing the same in Python 3.8 runs into an endless loop:

# python3 /home/abuild/rpmbuild/SOURCES/test.py
Traceback (most recent call last):
  File "/home/abuild/rpmbuild/SOURCES/test.py", line 15, in <module> 
    shutil.copytree('/dev/shm/t/pg', '/dev/shm/t/pg/somevendor/1.0')
  File "/usr/lib/python3.8/shutil.py", line 547, in copytree 
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
  File "/usr/lib/python3.8/shutil.py", line 486, in _copytree
    copytree(srcobj, dstname, symlinks, ignore, copy_function,
...
    copytree(srcobj, dstname, symlinks, ignore, copy_function,
  File "/usr/lib/python3.8/shutil.py", line 547, in copytree 
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
  File "/usr/lib/python3.8/shutil.py", line 449, in _copytree
    os.makedirs(dst, exist_ok=dirs_exist_ok)
  File "/usr/lib/python3.8/os.py", line 206, in makedirs 
    head, tail = path.split(name)
  File "/usr/lib/python3.8/posixpath.py", line 104, in split 
    sep = _get_sep(p)
  File "/usr/lib/python3.8/posixpath.py", line 42, in _get_sep 
    if isinstance(path, bytes):
RecursionError: maximum recursion depth exceeded while calling a Python object

I also reported this at https://bugzilla.opensuse.org/show_bug.cgi?id=1155839
msg356302 - (view) Author: Bruno P. Kinoshita (kinow) * Date: 2019-11-09 13:26
Hi,

I did a quick `git bisect` using the example provided, and looks like this regression was added in the fix for bpo-33695, commit 19c46a4c96553b2a8390bf8a0e138f2b23e28ed6.

It looks to me that the iterator returned by with os.scandir(...) is including the newly created dst directory (see the call for `os.makedirs(dst, exist_ok=dirs_exist_ok)` in https://github.com/python/cpython/blob/e27449da92b13730a5e11182f329d5da98a5e05b/Lib/shutil.py#L449).

This results in the function finding an extra directory, and repeating the steps for this folder and its subfolder recursively. This only happens because in the example in this issue, dst is a subdirectory of src.

The bpo-33695 commit had more changes, so I've reverted just this block of the copytree as a tentative fix, and added a unit test: https://github.com/python/cpython/pull/17098

--

Here's a simplified version of what's going on:

```
import os
import shutil

shutil.rmtree('/tmp/test/', True)
os.makedirs('/tmp/test')
with open('/tmp/test/foo', 'w+') as f:
  f.write('foo')

# now we have /tmp/test/foo, let's simulate what happens in copytree on master

with os.scandir('/tmp/test') as entries:
  # up to this point, /tmp/test/foo is the only entry
  os.makedirs('/tmp/test/1/2/3/', exist_ok=True) # <---- when the iterator starts below in `f in entries`, it will find 1 too
  # now 1 will have been added too
  for f in entries:
    print(f)
```
msg357134 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-11-21 06:53
PR-17098 basically reverts https://bugs.python.org/issue33695. Not good. =) 
I think we can simply consume the iterator immediately as in:

    def copytree(src, ...):
        with os.scandir(src) as itr:
            entries = list(itr)
        return _copytree(entries=entries, ...)
msg357141 - (view) Author: Bruno P. Kinoshita (kinow) * Date: 2019-11-21 08:03
Hi Giampaolo,

I think it is more or less the same as the previous code, which was using os.list to return a list in memory. My first tentative fix was:

    def copytree(src, ...):
        entries = os.list(src)
        return _copytree(entries=entries, ...)

But the previous PR also altered _copytree to use the return of os.scandir DirEntry's, so the change above results in AttributeError: 'str' object has no attribute 'name'.

Would be better to avoid using iterator to populate a list, and also using the DirEntry in _copytree, and just stick with the previous code with (i.e. os.listdir() and a single copytree method as before)? Or if you think we should go with your suggestion, I'm good with it as well - especially as it'd be a much simpler PR :)

Thanks
Bruno
msg357146 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-11-21 09:37
The speedup introduced in issue33695 is mostly because the number of os.stat() syscall was reduced from 6 to 1 per file (both by using scandir() and because stat() results are cached and passed around between function calls). As such, even if we immediately consume scandir() iterator I believe it won't have a meaningful impact in terms of speed. FWIW issue33695 has a benchmark script attached (but it's not very stable).
msg357149 - (view) Author: Bruno P. Kinoshita (kinow) * Date: 2019-11-21 10:14
I really liked that improvement, and didn't think it needed to be removed. That's why the PR reverts it partially. I think the os.stat improvements were in the other methods changed, and should not be changed in my PR - unless I changed it by accident.

So with the current PR for this issue, or with your suggested patch, both should have similar performance I think. (I hadn't seen that script to measure performance, thanks.)

I am happy if we revert partially, or if we build the fix on top of the current code consuming the iterator. Should I update the PR with your suggested fix then?
msg357156 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-11-21 10:38
PR-17098 as it stands re-introduces some stat() syscall. I suggest to just consume the iterator: it's a small change and it should fix the issue.
msg357157 - (view) Author: Bruno P. Kinoshita (kinow) * Date: 2019-11-21 10:49
Done. Rebased on master too, and edited commit message & GH PR title. Thanks Giampaolo!
msg357544 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-11-27 01:10
New changeset 9bbcbc9f6dfe1368fe7330b117707f828e6a2c18 by Giampaolo Rodola (Bruno P. Kinoshita) in branch 'master':
bpo-38688, shutil.copytree: consume iterator and create list of entries to prevent infinite recursion (GH-17098)
https://github.com/python/cpython/commit/9bbcbc9f6dfe1368fe7330b117707f828e6a2c18
msg357556 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2019-11-27 04:49
New changeset 65c92c5870944b972a879031abd4c20c4f0d7981 by Giampaolo Rodola (Bruno P. Kinoshita) in branch '3.8':
[3.8] bpo-38688, shutil.copytree: consume iterator and create list of entries to prevent infinite recursion (GH-17397)
https://github.com/python/cpython/commit/65c92c5870944b972a879031abd4c20c4f0d7981
History
Date User Action Args
2019-11-27 04:50:18giampaolo.rodolasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-11-27 04:49:43giampaolo.rodolasetmessages: + msg357556
2019-11-27 02:11:04kinowsetstage: commit review -> patch review
pull_requests: + pull_request16878
2019-11-27 01:13:16giampaolo.rodolasetstage: patch review -> commit review
versions: + Python 3.9
2019-11-27 01:10:53giampaolo.rodolasetmessages: + msg357544
2019-11-21 10:49:41kinowsetmessages: + msg357157
2019-11-21 10:38:36giampaolo.rodolasetmessages: + msg357156
2019-11-21 10:14:39kinowsetmessages: + msg357149
2019-11-21 09:37:33giampaolo.rodolasetmessages: + msg357146
2019-11-21 08:03:36kinowsetmessages: + msg357141
2019-11-21 06:53:01giampaolo.rodolasetmessages: + msg357134
2019-11-09 13:26:16kinowsetnosy: + kinow
messages: + msg356302
2019-11-09 13:25:26kinowsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request16604
2019-11-08 23:33:11terry.reedysetkeywords: + 3.8regression
type: crash -> behavior
stage: needs patch
2019-11-04 21:24:13vstinnersetnosy: + giampaolo.rodola
2019-11-04 21:21:38cboltzcreate