File.delete() potentially damages file systems.
| Project: | JNode FS |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | patch (code needs review) |
Description
While fixing the problems in 'del' I accidentally made a change that caused it to try to delete a non-empty directory. Much my surprise it "worked"; i.e. the File.delete(file) call returned "true" and directory was gone, according to "dir". This deserved some investigation. (If it really worked, why would the DeleteCommand class need to go to the trouble of recursing over the subdir tree???)
It turns out that File.delete(new File("someDir")) does not check that the directory is empty. More specifically, the code in FileSystemAPIImpl calls parentDir.remove(...) without doing any checks, and the one FSDirectory type that I looked at (Ext2) didn't check either!
If my reading of the code is correct, calling File.delete(..) on a non-empty directory is going to turn the directory's contents into orphans .... at least for the Ext2 file system. This is bad, since file systems are not garbage collected. Not withstanding what DeleteCommand does, the File.delete(..) method must either refuse to delete non-empty directories (preferred IMO) or do a proper job of deleting them. And the behavior should be consistent across all file system types.
- Login to post comments
#1
I changed this bug to allow comments. Moved to FS.
#2
Java's own behaviour is to return false if the directory has anything in it.
This is probably one of those things which should be fixed outside the FS drivers so that it works the same in all cases.
#3
#4
I have a patch for this issue. This patch adds a 'checkDeletable' method to FSDirectory and implements if for all relevant FS types. This new method is called in FileSystemAPIImpl prior to deleting a directory. The patch also includes a new testcase in FSTestSuite.
The patch needs review before committing. First, the changes don't deal with possible race conditions where one application adds a file to a directory while another is attempting to delete the directory. (I don't know enough about the way that the FS service and drivers synchronize filesystem updates.) Second, the new testcase is failing for Ext2 file systems because of a separate bug.
#5
I'm in the middle of a task but I will review it soon.
#6
The new checkDeletable() interface method is used at one place only and probably it will be so in in the future too, while its implementations produce a lot of duplication of code. The implementations basically verifyies three conditions: is the directory the root directory of the file system, is the file system read-only, is the directory empty. I think these three conditions could be checked in the method where checkDeletable() is used and an IOException thrown with the apropriate message. Hence we could avoid the need for a new interface method with very limited use, a new exception class and a lot of duplicate code which implement the method. Is better to keep the interfaces simple and guarantee a consistent and reliable implementation for the methods the interface already has.
The concurency issue is a more generic problem and will need a special analysis in a wider context.
#7
I designed the code that way because of the possibility of a file system where "." and ".." have other names, or a file system where there are other entry names that need to be ignored when deciding if a directory is empty.
Anyway, I'll leave it to someone else to redesign the code to address Levente's criticisms.
#8
I see your point about the current directory and the parent directory (are there other similar directory entries too?). They can be problematic in other situations too. That's why it would be important to apply a desing principle here which would enforce common a behaviour for all JNode file system implementations.
#9
I think that the links in the ext file systems are also concerned since they are aliasing a file or a directory.
#10
There are 2 others points to keep in mind :
1) in the future, operations like delete, creation, etc... should take care about access rights.
2) we could implements something like rm -fr in unix thus it could be possible to delete a non-empty directory.
My 2 cents,
Fabien L.
#11
I don't think [hard] links are even a "special case" for Ext2, It's just when you happen to have two directory entries pointing at the same file, it's called a hard link.
Internally, the logic for deleting files in Ext2 is (and I'm sure this has already been done correctly in Jnode) you delete its directory entry, decrement its reference counter, and then if it has hit zero, delete the file entry. Well... on top of this there was also logic for whether there is a descriptor open (i.e. you can delete a file while someone is still using it, and they won't be affected but when they close it, it will be deleted) but I've never really been sure how that actually works.
But as far as the checking side goes, the logic for Ext2 would be the same as always - a file can be deleted straight away (unless someone has it locked?), a directory can be deleted when it's empty. What actually goes on to perform the delete is a separate issue, occurring some time after checking whether you can delete it. I don't think it affects the logic for the check at all.
The need for special cases for "." and ".." is slightly baffling. In my opinion these entries shouldn't be exposed through the API, as it forces the caller to use "magic strings" to determine whether an entry is the current directory entry or the parent directory entry. Seeing as "." is trivially determinable (it's the current directory!) and there is (I think) a proper API method for getting the parent directory, I'm not sure why we even output them in the iterator. It seems like it would be far easier to remove them, and only expose the actual entries in the directory.
(And yes, I know that in some filesystems, "." and ".." are "actual" entries...)
#12
Re Fabien L's point 2: in UNIX, "rm -rf" is implemented in the "rm" program by recursively traversing the directory tree to be deleted. The delete syscall (unlink) will only delete one file or directory at a time. If the directory to be deleted is non-empty, the syscall fails, even if you are 'root'.
Re Daniel's query about why the iterator exposes the '.' and '..' entries: the Sun implementation of File.list* on UNIX returns all entries including '.' and '..'. If the JNode implementation hid real '.' and '..' entries and/or didn't fake them for FS types that don't have '.' or '..' entries, we could end up with some rather confusing (and non-portable) behavior. For example an 'ls -a' might say '.' does not exist, but 'cd .' would work ... or 'cd .' would fail on some file systems.
#13
It would be relatively easy to make "cd ." work on all filesystems though, even if they don't have a "." record.
I see your point about File.list() though, I've often had a similar objection to that API (as well as the objection that when it fails you get null, instead of a proper exception...) It just results in code where you're forced to check for "." and ".." every time you list a directory.
#14
It would be relatively easy to make "cd ." work on all file systems though, even if they don't have a "." record.
"cd ." is just an example. The current implementation of JNode's "cd" command just sets the global "user.dir" property, and getting "cd ." to work is relatively easy. (You need a JNode specific API that CdCommand can use to find out if "." has the UNIX meaning in a given directory.)
But what about "cd foo/./bar" or "cat ./bar" or "cat < ./bar" and so on. If "." was not supported uniformly in the File layer or beneath, any application that did anything with pathnames would have to deal with the potential non-uniformity. That would be bad for a number of reasons ... which I won't bore you by reciting
#15
Right, I'm just arguing that uniformity is best implemented once, rather than having every filesystem implement the same thing over and over. As of right now, if someone makes a new filesystem they would have to put in hack code to expose "." and ".." as entries even if they don't exist, and the caller will just be removing them again. It all seems rather silly to me, that's all. Sucks that Sun's API decided to output them, I'm sure they're the ones to blame for the whole problem.