Opened 6 years ago

Closed 6 years ago

#3097 enhancement closed fixed (fixed)

filepath.FilePath.remove breaks on symlinks

Reported by: cyli Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch: branches/filepath-remove-symlink-3097
(diff, github, buildbot, log)
Author: Launchpad Bug:

Description

FilePath.remove on a symlink whose target is a directory will result in an OSError. FilePath.remove currently checks to see if the path is a directory, and if so, removes all children and then calls os.rmdir on itself. Otherwise, it calls os.remove on itself. The problem is that a link that points to a directory returns true for both islink() and isdir(). Therefore, when you call FilePath.remove on a symlink whose target is a directory, it will both empty that target directory (which probably is not a desirable outcome) and call rmdir on the link, which raises an error.

Attachments (2)

filepath.remove.symlink.patch (1.1 KB) - added by cyli 6 years ago.
patch
filepath.remove.symlink.2.patch (1.7 KB) - added by cyli 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by cyli

  • Keywords review added
  • Owner cyli deleted
  • Priority changed from normal to highest

This solves the problem. I think this is the desired behavior for filepath.FilePath.remove

Another behavior that may be desirable (I didn't think so, so it's not in the patch) is that when filepath.FilePath.remove is called on a symlink that points to a directory: 1) everything inside the target directory is removed, 2) the target directory is removed, and 3) the symlink to that directory is removed. If this is the desired behavior, filepath.FilePath.remove can take an optional argument (ex. followLink) that defaults to False (since I don't think it's usually the desired behavior) but can be True when this behavior is desired.

Changed 6 years ago by cyli

patch

comment:2 Changed 6 years ago by spiv

It's a shame that remove is implemented using look-before-you-leap, which is inherently susceptible to race conditions. Although I guess any attempt to implement "delete this path, whatever sort of thing it is" is going to be racy on POSIX (because there's no atomic "unlink or rmdir" operation, let alone one that recurses), so this perhaps doesn't matter much in practice...

Getting more off-topic, having remove always mean recursively delete seems dubious to me. There certainly ought to be a convenience method with those semantics, but I'd also like there to be a more primitive "remove a file or error" method, like os.unlink. Maybe I'm just too used to thinking in terms of POSIX semantics :)

(These remarks should not be taken as criticism of the proposed fix, btw.)

comment:3 Changed 6 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to cyli
  • Priority changed from highest to high

That looks sensible. My comment on the patch:

  • the new behavior should be tested in a new (different) test, not in testRemove. The lack of os.symlink should result to an explicit skip
  • it would be great if remove could gain a doctring in the process. I won't press you to do it though :).

Thanks.

Changed 6 years ago by cyli

comment:4 Changed 6 years ago by cyli

  • Keywords review added
  • Owner cyli deleted
  • Priority changed from high to highest
  • Summary changed from FilePath.remove breaks on symlinks to filepath.FilePath.remove breaks on symlinks

Docstring put in, and tests moved to a new test that explicitly skips if symlinks are not supported. I won't change it here, since it's not part of the bug, but I could make remove take an optional argument as to whether or not to recursively remove directories.

comment:5 Changed 6 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/filepath-remove-symlink-3097

(In [23048]) Branching to 'filepath-remove-symlink-3097'

comment:6 Changed 6 years ago by exarkun

(In [23050]) simple non-behavioral changes

use getattr instead of hasattr - hasattr can mask exceptions (probably not on a module, but who knows)
write docstring in a newer style than is prevalant in test_paths.py currently
use assertTrue and assertFalse instead of failIf and failUnless
fix a bit of a grammatical thinko in the all-new remove docstring

refs #3097

comment:7 Changed 6 years ago by exarkun

  • author exarkun deleted
  • Keywords review removed

Okay I like this now, I think I'll merge it.

comment:8 Changed 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [23062]) Merge filepath-remove-symlink-3097

Author: cyli
Reviewer: therve, exarkun
Fixes #3097

Change FilePath.remove so that symlinks are only deleted by it, not the contents
of the directories they point to, and so that when a symlink which points to a
directory is encountered, an exception is not raised.

comment:9 Changed 3 years ago by <automation>

Note: See TracTickets for help on using tickets.