inconsistent behavior of removeDirectoryRecursive with symbolic links

[Rufflewind]

The way removeDirectoryRecursive dir works right now is totally inconsistent:

• If there's a directory-like symbolic link, the function removes it without recursing into it, unless the symbolic link is not removable for some reason (e.g. no permission), in which case it recurses into it and wipes out everything inside.
• If dir itself is actually a directory-like symbolic link, it will recurse into it but fail to remove dir itself.

The causes of these two problems are:

• Instead of explicitly checking whether path refers to a true directory, it assumes any unremovable file that also satisfies directoryExists must necessarily be a directory. This is false, because directoryExists dereferences the symbolic link.
• getDirectoryContents should not be called until dir is verified to be a true directory.

Note that there are two possible ways to handle the case where dir is not a true directory:

• One can delete it silently, similar to the behavior of the POSIX command rm -r.
• Or one can raise an error, similar to the behavior of the Python function shutil.rmtree.

See also:

• Pull request Fix incorrect comment about removeDirectoryRecursive and symlinks #1
• https://haskell.org/pipermail/libraries/2012-January/017482.html
• http://comments.gmane.org/gmane.comp.lang.haskell.libraries/22235

[gracjan]

I was also confused by symlinks and removal, see #2 and #3.

[argiopetech]

The assertion has been made that the current code is correct (see here for one example), so, for the sake of future discussion, following is a demonstration of it showing different behavior than coreutils (with destructive results).

First, create a user directory and put some dummy files in it.

elliot /tmp $ mkdir a-directory
elliot /tmp $ touch a-directory/f1
elliot /tmp $ touch a-directory/f2
elliot /tmp $ ls a-directory/
f1  f2

Then, as a different user, create a test directory with the sticky bit containing a symlink to the original directory.

elliot /tmp $ su
root tmp # mkdir test
root tmp # chmod +s test
root tmp # cd test
root test # ln -s ../a-directory a-link
root tmp # exit
elliot /tmp $ ls -ld test
drwsr-sr-x 2 root root 60 Feb 18 14:37 test
elliot /tmp $ ls -l test
total 0
lrwxrwxrwx 1 root root 14 Feb 18 14:37 a-link -> ../a-directory

Coreutils behaves properly:

elliot /tmp $ rm -r test
rm: descend into write-protected directory ‘test’? y
rm: cannot remove ‘test/a-link’: Permission denied
elliot /tmp $ ls test/
a-link
elliot /tmp $ ls a-directory/
f1  f2

All files remain.

Now, with System.Directory:

elliot /tmp $ ghci
Prelude> :m +System.Directory
Prelude System.Directory> removeDirectoryRecursive "test"
*** Exception: test/a-link: removeDirectory: permission denied (Permission denied)
Prelude System.Directory> 
Leaving GHCi.
elliot /tmp $ ls test/
a-link
elliot /tmp $ ls a-directory/

Note that the permissions error is given by System.Directory as with rm; however, the current algorithm uses this to infer that the symlink is a proper directory and subsequently empties it.

Incidentally, this means that db88005 needs to be reverted prior to 7.10.1 and modified to state that removeDirectoryRecursive may follow symlinks under certain circumstances.

[argiopetech]

New message in fd8c99d.

[Rufflewind]

The fact that removeDirectoryRecursive traverses symbolic links under strange circumstances is most certainly a bug due to a flaw in the implementation.

Please take a look at the pull request, which fixes the inconsistency so that it will never* dereference symbolic links. It might be a good idea to expose isRealDirectory as a public API function, as well.

I was wrote some preliminary tests to demonstrate that the problem is fixed. (Unfortunately to run these tests one would need a GHC repo on the side.)

[*] barring unavoidable race conditions

[argiopetech]

Passes the test that directory-1.2.1.0 fails. I don't see a real need to expose removePathRecursive, since I don't believe we've never exposed a true rm -r analog before. I like the idea of exporting isRealDirectory. I know I've had to hack around the lack of that function before.

If this is tested on Windows, I'm in favor of pushing this in before the GHC 7.10 RC3 pull. I'd do it myself, but my Windows box lost a hard drive and is offline for the moment...

[Rufflewind]

I don't see a real need to expose removePathRecursive since I don't believe we've never exposed a true rm -r analog before.

The implementation of removePathRecursive is more elegant than removeDirectoryRecursive so it serves as a useful helper function. It will be left as an internal function until someone asks for it.

I'll test the patch on Windows tonight. Need to also check if isRealDirectory works correctly on Windows with NTFS symlinks.