Original comment posted by jherico72 on 2010-07-14 at 06:14 PM

Actually, the test itself is useless on the windows platform. A junction (the windows version of a symbolic link) will not return a different canonical vs absolute pathname. This means that the behavior on windows will differ widely from the intended implementation in two different ways, depending on the case of the provided input.

Original comment posted by jherico72 on 2010-07-16 at 01:49 AM

Attached is a patch that should fix the issue by changing the mechanism by which guava detects the symbolic link. However, it adds a dependency on the sun JNA jar. It also slightly

Original comment posted by jherico72 on 2010-07-16 at 03:59 PM

Sorry, my last comment ended up getting cut off. I meant to say that the patch slightly changes the mechanism of the method so that it only avoids following symbolic links at or below the specified path. This patch reverts to the old behavior where the function will revert to a no-op if there is a symbolic link anywhere in the path (though I don't understand the justification for this behavior, since there are plenty of use cases where a file system might contain symlinks for things like /home or /tmp or anythign really).

Original comment posted by jherico72 on 2010-07-20 at 03:03 AM

Adding a new patch that changes the name of the test function and improves support for non-ascii characters in the path

Original comment posted by kevinb@google.com on 2010-07-30 at 03:56 AM

(No comment entered for this change.)

Labels: -Priority-Medium

Original comment posted by kevinb@google.com on 2011-01-12 at 10:32 PM

Issue #498 has been merged into this issue.

Original comment posted by fry@google.com on 2011-01-28 at 04:17 PM

(No comment entered for this change.)

Status: Accepted

Original comment posted by kevinb@google.com on 2011-07-13 at 06:18 PM

(No comment entered for this change.)

Status: Triaged

Original comment posted by cpovirk@google.com on 2011-07-21 at 06:25 PM

Issue #359 has been merged into this issue.

Original comment posted by cpovirk@google.com on 2011-07-21 at 06:25 PM

Issue #637 has been merged into this issue.

Original comment posted by cpovirk@google.com on 2011-07-21 at 06:27 PM

We just got the first report of this internally, which led me to discover that there are 4 external copies of it.

Copying Kevin's comment from issue 637 here:

"""
Fortunately for everyone, JDK 7 has a real filesystem library for the first time; unfortunately for you now perhaps, this means we don't have much motivation to sink more effort into this area of Guava.

If this is very important to anyone, please send us a patch and we will test it out.
"""

Given the many people affected, we'd be thrilled if someone stepped up.

CC: kevinb@google.com

Original comment posted by jherico72 on 2011-07-21 at 10:07 PM

There IS a patch attached to this bug.

Original comment posted by kevinb@google.com on 2011-07-22 at 03:31 AM

Argh. Thank you for not tearing me a new one over that very dumb comment.

I still fear negative consequences of any patch, but will test it out as promised.

Status: Accepted
Labels: Milestone-Release10

Original comment posted by cpovirk@google.com on 2011-08-15 at 03:44 PM

Some internal discussion has revealed that it's outright impossible to implement deleteRecursively properly in pure Java, even under JDK7, so we're going to remove this method. We recommend shelling out to "rm -rf" or your platform's equivalent.

"""
To understand the threat environment, imagine a recursive delete encountering a concurrent execution of:

while true; do mkdir x; rmdir x; ln -s / x; rm x; done

but there's a risk even in the absence of malicious intent.
"""

Status: Obsolete
Labels: -Milestone-Release10

Original comment posted by kevinb@google.com on 2011-08-25 at 03:41 PM

Issue #693 has been merged into this issue.

Original comment posted by cow...@bbs.darktech.org on 2012-02-13 at 09:17 PM

According to http://stackoverflow.com/questions/307437/moving-a-directory-atomically "rm -rf" is not an atomic operation. Even if it was, I'd be surprised if the same was the case under Windows (and other platforms).

In light of this, would you care to reconsider this issue? You can also attach a prominent warning in the Javadoc.

Original comment posted by cpovirk@google.com on 2012-02-13 at 09:28 PM

My recollection is that the problem here is worse than atomicity. The example given in comment 14 shows how a user can cause deleteRecursively("/some/non/root/directory") to remove all files from the filesystem. This can't happen with rm -rf because rm -rf operates at a lower level than the filename interface available to us in Java.

Original comment posted by cow...@bbs.darktech.org on 2012-02-13 at 09:39 PM

I'm sorry, you're going to need to elaborate on that example. Here's my understanding:

mkdir x;
rmdir x;

-> This creates and delete a normal directory (I'm not sure why it's even required for this use-case).

ln -s / x;
rm x;

-> Creates a symbol link "x" that maps to the root directory. Then deletes the symbol link (not root).

Now, I assume you meant that someone is running deleteRecursively(x) before the symbolic link is created. So our check for "is this a symbolic link" passes. We then try to delete "x" recursively. Suddenly the script creates the symbolic link and we mistakenly blow away the root directory.

Now, I fail to see how "rm -rf" is better in this regard. I believe it is liable to run into the exact same race condition.

Original comment posted by wasserman.louis on 2012-02-13 at 09:52 PM

JDK 7 appears to deal with this in a reasonable fashion:

http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) only deletes empty directories, and suggests that you use the walkFileTree method to do recursive things.

http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#walkFileTree(java.nio.file.Path, java.util.Set, int, java.nio.file.FileVisitor) makes several sensible decisions that avoid infinite recursion: having a maximum depth, having an option to check for cycles, and having a rather nice abstraction for recursing into directories.

I'm under the impression that most of common.io will eventually need to be reevaluated for need based on Java 7, so it's not clear how much effort should be put into it right now, if that makes sense, even though Guava is only now moving to Java 6. cowwoc, how does that relate to your individual needs? Are you on Java 7, or do you see yourself moving to Java 7 in the not-too-horribly-distant future?

Original comment posted by cow...@bbs.darktech.org on 2012-02-13 at 09:58 PM

Louis,

All my internal code uses JDK7 already. I mostly need this functionality for libraries I've published that are stuck to JDK6 for now. I'll probably migrate the libraries to JDK7 when Guava does the same :)

Original comment posted by cpovirk@google.com on 2012-02-13 at 10:05 PM

Your understanding is correct. rm -rf won't follow the symlink in this case. To see why, let's start with an example without a concurrent modification:

$ ls -l abc
total 4
-rw-r----- 1 cpovirk eng 0 2012-02-13 16:47 x
lrwxrwxrwx 1 cpovirk eng 1 2012-02-13 16:47 y -> /
drwxr-x--- 2 cpovirk eng 4096 2012-02-13 16:47 z

$ strace rm -rf abc
...
newfstatat(AT_FDCWD, "abc", {st_mode=S_IFDIR|0750, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
unlinkat(AT_FDCWD, "abc", 0) = -1 EISDIR (Is a directory)
openat(AT_FDCWD, "abc", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW) = 3
fstat(3, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
fstat(3, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
fcntl(3, F_GETFL) = 0x28800 (flags O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_NOFOLLOW)
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
fstat(3, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
getdents(3, /* 5 entries /, 32768) = 120
openat(3, "z", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW) = 4
fstat(4, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
fstat(4, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
fcntl(4, F_GETFL) = 0x28800 (flags O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_NOFOLLOW)
fcntl(4, F_SETFD, FD_CLOEXEC) = 0
close(3) = 0
fcntl(4, F_GETFD) = 0x1 (flags FD_CLOEXEC)
fstat(4, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
getdents(4, / 2 entries /, 32768) = 48
getdents(4, / 0 entries /, 32768) = 0
fcntl(4, F_GETFD) = 0x1 (flags FD_CLOEXEC)
openat(4, "..", O_RDONLY) = 3
close(4) = 0
fstat(3, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
unlinkat(3, "z", AT_REMOVEDIR) = 0
fstat(3, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
fcntl(3, F_GETFL) = 0x8000 (flags O_RDONLY|O_LARGEFILE)
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
fstat(3, {st_mode=S_IFDIR|0750, st_size=4096, ...}) = 0
getdents(3, / 4 entries /, 32768) = 96
unlinkat(3, "y", 0) = 0
unlinkat(3, "x", 0) = 0
getdents(3, / 0 entries */, 32768) = 0
fcntl(3, F_GETFD) = 0x1 (flags FD_CLOEXEC)
close(3) = 0
unlinkat(AT_FDCWD, "abc", AT_REMOVEDIR) = 0
...

To remove a directory, rm -rf calls:

openat(3, "z", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW) = 4

If z changes from a directory to a link between the original read of z and the openat call, the call will fail (or maybe rm -rf will try to recovery? Either way, it's informed the the file type has changed). Or, if z changes from a directory to a link after the openat call, the calls to remove the contents of z will still delete the contents of the old z because those calls will use unlinkat, which deletes files relative to the file descriptor, which still points at old z, not to the path, which points to some new directory.

Original comment posted by cpovirk@google.com on 2012-02-13 at 10:07 PM

Martin suggested internally that the JDK7 interfaces still aren't enough to implement rm -rf properly. From a quick glance, I suspect that this is true of walkFileTree, as it, too, operates in terms of paths. I think we need the file-descriptor functionality described above in order to implement this properly.

Original comment posted by cow...@bbs.darktech.org on 2012-02-13 at 10:11 PM

Couldn't you do something like this...?

1. deleteRecursively(x) is invoked
2. let x == original path, x' = canonical path (resolve the link up-front)
3. Walk the tree, deleting nested files as follows:
4. For each file, calculate the path relative to x. Now, apply this relative path to x'.

The point is to resolve the symbolic link once and apply nested paths relative to that moving forward. What do you think?

Original comment posted by cpovirk@google.com on 2012-02-13 at 10:19 PM

I don't think this gets at the heart of the problem, which doesn't requite that x itself be a symlink. If /x contains directory y with file z and we try to remove /x/y/z, then we're in trouble if anyone can replace y with a link to /.

Original comment posted by cow...@bbs.darktech.org on 2012-02-13 at 10:25 PM

You play the same trick, recursively. You store y and y'. You convert z to a path relative to y, then append it to y'.

Original comment posted by cpovirk@google.com on 2012-02-13 at 10:37 PM

But if y is a symlink when we read it, we don't try to remove its contents at all. The only case in which we try to remove its contents is if it's originally a directory. Then problem arises when it changes from a being a directory (where y==y') to not being a directory.

Original comment posted by cow...@bbs.darktech.org on 2012-02-14 at 03:36 AM

I see your point. You want an atomic Files.delete(Path, DeleteOption...) with StandardDeleteOption.FOLLOW_SYMLINK so that you can say "delete this path only if it doesn't contain any symlinks, atomically".

I'm pretty sure you can implement that atomically in C on all platforms (using the file descriptor as you mentioned). Perhaps we need to file a RFE asking for this method to be added?

Original comment posted by cpovirk@google.com on 2012-02-14 at 05:06 PM

I see your point. You want an atomic Files.delete(Path, DeleteOption...)
with StandardDeleteOption.FOLLOW_SYMLINK so that you can say "delete this
path only if it doesn't contain any symlinks, atomically".

Though we do want for any symlinks in the original given path to be followed, except for the final component of the path. This might be workable (not that I trust my Unix knowledge enough to make that call).

I'm pretty sure you can implement that atomically in C on all platforms
(using the file descriptor as you mentioned). Perhaps we need to file a RFE
asking for this method to be added?

I'm surprised that I don't see one already in a quick search.

Original comment posted by cow...@bbs.darktech.org on 2012-02-15 at 02:45 PM

Since you seem to have a better understanding of what is needed, could you please file the RFE? :)

Original comment posted by cow...@bbs.darktech.org on 2012-02-26 at 06:53 PM

I filed a RFE at https://bugs.openjdk.org/browse/JDK-7148952

Original comment posted by cpovirk@google.com on 2012-02-27 at 01:39 PM

Thanks for that, and sorry for slacking on getting it done.

I didn't know about http://download.java.net/jdk7/archive/b123/docs/api/java/nio/file/SecureDirectoryStream.html -- I'd like to think it's good enough for rm -rf, but the bug comment suggests otherwise, so I'll have to find someone who can show me what the problem is. (And we'll see how long it will be before Guava contains JDK7-isms....)

Original comment posted by cow...@bbs.darktech.org on 2012-03-19 at 02:33 AM

The BugParade evaluator mentions that Linux and Solaris return SecureDirectoryStream but other platforms do not. I wonder why that is. Is it a matter of taking the time to add Windows/Mac support? Or do these operating systems really lack the relevant APIs?

Does Windows, for example, have an equivalent of openat() allowing the opening of a file relative to an existing HANDLE?

Original comment posted by cow...@bbs.darktech.org on 2012-05-09 at 06:49 PM

Can someone please reopen this issue?

Original comment posted by wasserman.louis on 2012-05-09 at 07:04 PM

I'm not sure I understand why, if it isn't possible to do this correctly with JDK6. (Even if the RFE has been fixed in JDK 7, we won't be upgrading for a while.)

Original comment posted by cow...@bbs.darktech.org on 2012-05-09 at 07:18 PM

Louis,

I don't think that is a good excuse for removing the method from the API. I am in favor of providing Files.deleteRecursively() as-is and documenting its limitations.

Telling people to invoke "rm -rf" is a cop out for two reasons:

1. It's not cross-platform.
2. It's isn't actually thread-safe (see the link I posted above).

Our goal should be to provide a cross-platform "as safe as possible" solution. If you really want to invoke "rm -rf" then just do it under the hood. Don't make the user do it. The user isn't any more capable of doing this right than we are.

(This got done long ago in MoreFiles.deleteRecursively​ when cgdecker found the necessary magic (which is to call getFileName() on the Path returned by the SecureDirectoryStream).)