Commons IO
  1. Commons IO
  2. IO-168

Symbolic links (symlinks) followed when deleting directory.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 2.0
    • Component/s: Utilities
    • Labels:
      None
    • Environment:

      Linux only (symlinks required for bug to manifest)

      Description

      If 'dlink' is a symbolic link to a directory 'dir', and FileUtils.forceDelete is called on dlink, then here is what happens:

      1) the contents of 'dir' are emptied (the link is followed).
      2) 'dir' continues to exist (but is empty).
      3) 'dlink' is removed.

      The correct behavior is to simply remove 'dlink' without following it and thus without altering the contents of 'dir' (or 'dir' itself).

      1. symlinkFix.patch
        10 kB
        Brydie McCoy

        Activity

        Hide
        Niall Pemberton added a comment -

        Apostolos thanks for your suggestion - I changed the method to always return false for Windows:

        http://svn.apache.org/viewvc?view=revision&revision=1002416

        Show
        Niall Pemberton added a comment - Apostolos thanks for your suggestion - I changed the method to always return false for Windows: http://svn.apache.org/viewvc?view=revision&revision=1002416
        Hide
        Apostolos Lerios added a comment - - edited

        The issue is very close to being resolved. Attila's suggestion was near perfect. It has the following problem though:

        If File f was constructed using a relative path (e.g. new File("dir") where dir is a directory within the current one), then getParent() will return null. So far so good. However, if dir is a directory whose ancestors include folders with long names (e.g. "Documents and Settings"), then under 1.6.0_14 on Windows XP SP3, the canonical path is something like

        C:\Documents and Settings\Administrator\Desktop\test\directory

        And the absolute path is something like

        c:\DOCUME~1\Administrator\DESKTOP\tst\directory

        They are unequal, so isSymlink will return true. Which is incorrect. To avoid this problem, we should obtain f's absolute path before we try to get its parent. That is, inside isSymlink,

        file.getParent()

        should be replaced with

        file.getAbsoluteFile().getParent()

        And similarly file.getParentFile() should become file.getAbsoluteFile().getParentFile().

        Doing this will ensure that we'll obtain f's parent, even if f is specified with a relative file name. This, combined with Attila's fix, will avoid oddities like the DOS names above somewhere in the ancestral hierarchy.

        Show
        Apostolos Lerios added a comment - - edited The issue is very close to being resolved. Attila's suggestion was near perfect. It has the following problem though: If File f was constructed using a relative path (e.g. new File("dir") where dir is a directory within the current one), then getParent() will return null. So far so good. However, if dir is a directory whose ancestors include folders with long names (e.g. "Documents and Settings"), then under 1.6.0_14 on Windows XP SP3, the canonical path is something like C:\Documents and Settings\Administrator\Desktop\test\directory And the absolute path is something like c:\DOCUME~1\Administrator\DESKTOP\tst\directory They are unequal, so isSymlink will return true. Which is incorrect. To avoid this problem, we should obtain f's absolute path before we try to get its parent. That is, inside isSymlink, file.getParent() should be replaced with file.getAbsoluteFile().getParent() And similarly file.getParentFile() should become file.getAbsoluteFile().getParentFile(). Doing this will ensure that we'll obtain f's parent, even if f is specified with a relative file name. This, combined with Attila's fix, will avoid oddities like the DOS names above somewhere in the ancestral hierarchy.
        Hide
        Niall Pemberton added a comment -

        Thanks for testing, marking as resolved.

        Show
        Niall Pemberton added a comment - Thanks for testing, marking as resolved.
        Hide
        Brydie McCoy added a comment -

        Thanks for that Niall, I knew there was something i was forgetting... Anyway, I ran the tests on your changes and they are still passing!

        Thanks again,
        Brydie

        Show
        Brydie McCoy added a comment - Thanks for that Niall, I knew there was something i was forgetting... Anyway, I ran the tests on your changes and they are still passing! Thanks again, Brydie
        Hide
        Niall Pemberton added a comment -

        Brydie, thanks for the patch and Attila for the solution. I have applied a slightly modified form, which includes null checks - which if you could run the tests would be great because I'm on windows

        http://svn.apache.org/viewvc?view=rev&revision=684715
        http://svn.apache.org/viewvc?view=rev&revision=684716

        Show
        Niall Pemberton added a comment - Brydie, thanks for the patch and Attila for the solution. I have applied a slightly modified form, which includes null checks - which if you could run the tests would be great because I'm on windows http://svn.apache.org/viewvc?view=rev&revision=684715 http://svn.apache.org/viewvc?view=rev&revision=684716
        Hide
        Brydie McCoy added a comment -

        Hi Guys,

        I an attempt to get this fix in sooner, I have written up a patch for FileUtils based on Attila's suggestions along with a whole bunch of tests. I have only put the fix in for the regular deleteDirectory and not the deleteDirectoryOnExit method which I am assuming will probably need this check in it as well.

        Hope it helps...

        Cheers,
        Brydie

        Show
        Brydie McCoy added a comment - Hi Guys, I an attempt to get this fix in sooner, I have written up a patch for FileUtils based on Attila's suggestions along with a whole bunch of tests. I have only put the fix in for the regular deleteDirectory and not the deleteDirectoryOnExit method which I am assuming will probably need this check in it as well. Hope it helps... Cheers, Brydie
        Hide
        Attila Szegedi added a comment -

        Folks,

        file.getCanonicalFile().equals(file.getAbsoluteFile())

        won't return true only for a symlink. It will also return true for a file that has at least one symlinked directory in its path.

        This will work as expected though:

        File canonicalDir = file.getParentFile().getCanonicalFile();
        File fileInCanonicalDir = new File(canonicalDir, file.getName());
        return fileInCanonicalDir.getCanonicalFile().equals(fileInCanonicalDir.getAbsoluteFile());

        Attila.

        Show
        Attila Szegedi added a comment - Folks, file.getCanonicalFile().equals(file.getAbsoluteFile()) won't return true only for a symlink. It will also return true for a file that has at least one symlinked directory in its path. This will work as expected though: File canonicalDir = file.getParentFile().getCanonicalFile(); File fileInCanonicalDir = new File(canonicalDir, file.getName()); return fileInCanonicalDir.getCanonicalFile().equals(fileInCanonicalDir.getAbsoluteFile()); Attila.
        Hide
        Ajay Sridhar added a comment -

        Hi Guys,

        Is there an update on this? This seems like a fairly severe bug,

        Cheers,
        Ajay.

        Show
        Ajay Sridhar added a comment - Hi Guys, Is there an update on this? This seems like a fairly severe bug, Cheers, Ajay.
        Hide
        Apostolos Lerios added a comment -

        Yes. Here is how.

        public static final FileType get
        (File file)
        {
        if (file==null)

        { return UNKNOWN; }

        try {
        if (!file.exists())

        { return NONEXISTENT; }

        if (file.getCanonicalFile().equals(file.getAbsoluteFile())) {
        if (file.isDirectory())

        { return DIR; }

        if (file.isFile())

        { return FILE; }

        } else {
        if (file.isDirectory())

        { return LINK_DIR; }

        if (file.isFile())

        { return LINK_FILE; }

        }
        } catch (IOException ex)

        { // CANNOT_GET_TYPE }

        return UNKNOWN;
        }

        Show
        Apostolos Lerios added a comment - Yes. Here is how. public static final FileType get (File file) { if (file==null) { return UNKNOWN; } try { if (!file.exists()) { return NONEXISTENT; } if (file.getCanonicalFile().equals(file.getAbsoluteFile())) { if (file.isDirectory()) { return DIR; } if (file.isFile()) { return FILE; } } else { if (file.isDirectory()) { return LINK_DIR; } if (file.isFile()) { return LINK_FILE; } } } catch (IOException ex) { // CANNOT_GET_TYPE } return UNKNOWN; }
        Hide
        Niall Pemberton added a comment -

        Is there any way to determine a file is a symlink?

        Show
        Niall Pemberton added a comment - Is there any way to determine a file is a symlink?

          People

          • Assignee:
            Niall Pemberton
            Reporter:
            Apostolos Lerios
          • Votes:
            4 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development