Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ntkoopman mannequin opened this issue Dec 20, 2007 · 11 comments
Closed

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

ntkoopman mannequin opened this issue Dec 20, 2007 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ntkoopman
Copy link
Mannequin

ntkoopman mannequin commented 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']

    @ntkoopman
    Copy link
    Mannequin Author

    ntkoopman mannequin commented Dec 20, 2007

    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.

    @ntkoopman ntkoopman mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 20, 2007
    @ntkoopman ntkoopman mannequin changed the title shutils.rmtree fails on symlink, after deleting contents shutil.rmtree fails on symlink, after deleting contents Dec 20, 2007
    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 20, 2007

    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?

    @ntkoopman
    Copy link
    Mannequin Author

    ntkoopman mannequin commented Dec 20, 2007

    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.

    @gvanrossum
    Copy link
    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).

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 20, 2007

    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?

    @gvanrossum
    Copy link
    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?

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 20, 2007

    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\>


    @birkenfeld
    Copy link
    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.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Jan 21, 2008

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

    @birkenfeld
    Copy link
    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.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Jan 21, 2008

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants