Skip to content

shutil.rmtree fails on symlink, after deleting contents #46010

Closed
Closed
shutil.rmtree fails on symlink, after deleting contents#46010
Labels
stdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

Description

@ntkoopman
ntkoopman
mannequin
opened on Dec 20, 2007
BPO 1669
Nosy @gvanrossum, @birkenfeld
Files
  • rmtree.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2008-01-20.14:18:25.345>
created_at = <Date 2007-12-20.08:53:21.894>
labels = ['type-bug', 'library']
title = 'shutil.rmtree fails on symlink, after deleting contents'
updated_at = <Date 2008-01-21.17:44:16.274>
user = 'https://bugs.python.org/ntkoopman'

    bugs.python.org fields:

    activity = <Date 2008-01-21.17:44:16.274>
actor = 'draghuram'
assignee = 'none'
closed = True
closed_date = <Date 2008-01-20.14:18:25.345>
closer = 'georg.brandl'
components = ['Library (Lib)']
creation = <Date 2007-12-20.08:53:21.894>
creator = 'ntkoopman'
dependencies = []
files = ['9015']
hgrepos = []
issue_num = 1669
keywords = []
message_count = 11.0
messages = ['58864', '58892', '58900', '58903', '58904', '58908', '58918', '61298', '61400', '61410', '61428']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'georg.brandl', 'draghuram', 'ntkoopman']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1669'
versions = ['Python 2.5']

    Activity

    1. ntkoopman commented on Dec 20, 2007

      @ntkoopman
      ntkoopmanmannequin
      on Dec 20, 2007
      MannequinAuthor

      When using rmtree with a symlink to a directory as path, it will first
      follow the symlink, (try to) remove all the contents of the source
      directory and then raise the exception "OSError: [Errno 20] Not a
      directory".

      Expected behaviour: The function should only raise an exception.

      If the current behaviour is indeed wanted, it should a least be documented.

    2. ntkoopman
      added
      stdlibStandard Library Python modules in the Lib/ directory
      type-bugAn unexpected behavior, bug, or error
      on Dec 20, 2007
    3. ntkoopman
      changed the title [-]shutils.rmtree fails on symlink, after deleting contents[/-] [+]shutil.rmtree fails on symlink, after deleting contents[/+] on Dec 20, 2007

    4. draghuram commented on Dec 20, 2007

      @draghuram
      draghurammannequin
      on Dec 20, 2007
      Mannequin

      Please try to include stack trace in bug reports. I reproduced the error
      on my Linux (SuSE 10).

      marvin:tmp$ ls -l dirlink testdir
      lrwxrwxrwx 1 raghu users 7 2007-12-20 10:10 dirlink -> testdir/

      testdir:
      total 0
      -rw-r--r-- 1 raghu users 0 2007-12-20 10:36 testfile

      shutil.rmtree('dirlink') produces:

      ----------

      Traceback (most recent call last):
  File "t.py", line 4, in <module>
    shutil.rmtree('dirlink')
  File "/localhome/raghu/localwork/cpython/python/Lib/shutil.py", line
194, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/localhome/raghu/localwork/cpython/python/Lib/shutil.py", line
192, in rmtree
    os.rmdir(path)
OSError: [Errno 20] Not a directory: 'dirlink'

      While we are removing the contents of the target directory as expected,
      the final 'rmdir' fails on removing the symbolic link. For the sake of
      consistency, we can explicitly remove the target directory and leave the
      symbolic link intact. Is this what you are expecting?

    5. ntkoopman commented on Dec 20, 2007

      @ntkoopman
      ntkoopmanmannequin
      on Dec 20, 2007
      MannequinAuthor

      While we are removing the contents of the target directory as expected,

      This is not what I expected at all. I expected the function to fail,
      because the target was not a directory, just a symlink to a directory.
      That, or behavior similar to the command "rm -rf" which only removes the
      symlink, but not the contents.

      The current behavior is in my opinion inconsistent; if the symlink is
      treated as a normal directory, it should also get deleted.

      As I said, if the current behavior is apparently expected, it should be
      documented, because I can think of no program that follow symlinks while
      deleting unless specifically instructed to.

    6. gvanrossum commented on Dec 20, 2007

      @gvanrossum
      gvanrossum
      on Dec 20, 2007
      Member

      I agree with Tesiph, more useful behavior would be to raise an error
      immediately because the argument is not a directory. If you wanted to
      remove the think linked to, you could use rmtree("foo/.",
      ignore_errors=True).

    7. draghuram commented on Dec 20, 2007

      @draghuram
      draghurammannequin
      on Dec 20, 2007
      Mannequin

      I am ok with disallowing symlinks in rmtree() because if it were to be
      allowed, the semantics are not clear. In addition, neither 'rmdir' nor
      'rm -rf' delete the target directory.

      The attached patch would raise error if symbolic link is passed to
      rmtree. Even though there is a parameter 'ignore_errors", I don't think
      it should be used in suppressing symbolic link error. Comments?

    8. gvanrossum commented on Dec 20, 2007

      @gvanrossum
      gvanrossum
      on Dec 20, 2007
      Member

      Thanks for the patch. I think it should raise IOError, not ValueError,
      and it should use the onerror() handling used for all other errors.
      Also, can you include an update for the docs in the Doc tree?

    9. draghuram commented on Dec 20, 2007

      @draghuram
      draghurammannequin
      on Dec 20, 2007
      Mannequin

      Index: Lib/shutil.py
      ===================================================================

      --- Lib/shutil.py       (revision 59581)
+++ Lib/shutil.py       (working copy)
@@ -156,6 +156,16 @@
     elif onerror is None:
         def onerror(*args):
             raise
+
+    try:
+        if os.path.islink(path):
+            if ignore_errors:
+                return
+            else:
+                raise IOError('path can not be symbolic link')
+    except IOError, err:
+        onerror(os.path.islink, path, sys.exc_info())
+
     names = []
     try:
         names = os.listdir(path)

      How does this look? The error handling is slightly different for this
      case because it can not continue if 'ignore_errors' is True. I will
      update the doc if the code change is ok.

      On Dec 20, 2007 12:43 PM, Guido van Rossum <report@bugs.python.org> wrote:

      Guido van Rossum added the comment:

      Thanks for the patch. I think it should raise IOError, not ValueError,
      and it should use the onerror() handling used for all other errors.
      Also, can you include an update for the docs in the Doc tree?

      Tracker <report@bugs.python.org>
      <http://bugs.python.org/issue1669\>

    10. birkenfeld commented on Jan 20, 2008

      @birkenfeld
      birkenfeld
      on Jan 20, 2008
      Member

      Committed a variant of the last patch in r60139; it now raises OSError
      like all other functions that are used in rmtree(). Added docs and tests.

    11. birkenfeld
      closed this as completedon Jan 20, 2008

    12. draghuram commented on Jan 21, 2008

      @draghuram
      draghurammannequin
      on Jan 21, 2008
      Mannequin

      If rmtree() always returns in case of symbolic links (as it is
      checked-in), wouldn't it be much simpler to totally do away with
      'onerror' handling? I thought 'onerror' gives the user the option how to
      proceed (which is true in other usages).

    13. birkenfeld commented on Jan 21, 2008

      @birkenfeld
      birkenfeld
      on Jan 21, 2008
      Member

      Guido explicitly said "it should raise IOError, not ValueError,
      and it should use the onerror() handling used for all other errors"
      which makes sense for me too.

    14. draghuram commented on Jan 21, 2008

      @draghuram
      draghurammannequin
      on Jan 21, 2008
      Mannequin

      and it should use the onerror() handling used for all other errors"
      which makes sense for me too.

      Sure. I am ok with using 'onerror'. The point I am trying to make is
      that there is slight difference between 'onerror' in this case and in
      the three other places where it is called. In the case of symlink,
      rmtree() returns even when 'onerror' doesn't raise exception. This is
      not same in the other cases where rmtree() continues. I am just
      wondering if this inconsistency is worth it and if it is, an explicit
      mention in the doc will be nice.

    15. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      stdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions