Commons IO
  1. Commons IO
  2. IO-319

FileUtils.sizeOfDirectory follows symbolic links.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.4
    • Component/s: None
    • Labels:
      None

      Description

      First of all Thanks tons Apache Commons folks for all the amazing work! My first JIRA. Yayyy. I contributed B-)

      A symbolic link may create a cycle and so sizeOfDirectory crashes with an IllegalArgumentException. e.g.

      $ tree test
      test
      ├── file
      └── ravi
          ├── cycle -> ../../test
          └── file
      

      causes FileUtils.sizeOfDirectory to crash like so

      java TestJAVA
      Exception in thread "main" java.lang.IllegalArgumentException: <somepath>/test/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle/ravi/cycle does not exist
              at org.apache.commons.io.FileUtils.sizeOf(FileUtils.java:2053)
              at org.apache.commons.io.FileUtils.sizeOfDirectory(FileUtils.java:2089)
              at org.apache.commons.io.FileUtils.sizeOf(FileUtils.java:2057)
              at org.apache.commons.io.FileUtils.sizeOfDirectory(FileUtils.java:2089)
              at org.apache.commons.io.FileUtils.sizeOf(FileUtils.java:2057)
              at org.apache.commons.io.FileUtils.sizeOfDirectory(FileUtils.java:2089)
              at org.apache.commons.io.FileUtils.sizeOf(FileUtils.java:2057)
              at org.apache.commons.io.FileUtils.sizeOfDirectory(FileUtils.java:2089)
      

      We faced the same issue in Hadoop . Checkout https://issues.apache.org/jira/browse/HADOOP-6963 for our solution

      1. commons-io-319.patch
        1 kB
        Ravi Prakash
      2. commons-io-319.patch
        2 kB
        Ravi Prakash

        Activity

        Hide
        Gary Gregory added a comment -

        Can you provide a patch for [io] trunk please?

        Show
        Gary Gregory added a comment - Can you provide a patch for [io] trunk please?
        Hide
        Ravi Prakash added a comment -

        I'll be happy to. I'm working on it now. I'm not sure how platform-independent the test code needs to be, but I'll give it a fair shot and hopefully you'll be able to guide me to a better iteration.

        Show
        Ravi Prakash added a comment - I'll be happy to. I'm working on it now. I'm not sure how platform-independent the test code needs to be, but I'll give it a fair shot and hopefully you'll be able to guide me to a better iteration.
        Hide
        Gary Gregory added a comment -

        Great, thank you Ravi.

        Show
        Gary Gregory added a comment - Great, thank you Ravi.
        Hide
        Ravi Prakash added a comment -

        Hi Gary,

        Could you please review this patch? I wasn't able to find a way to create symlinks under Windows (FAT doesn't seem to support it) so the test code only checks under non-windows systems.

        Show
        Ravi Prakash added a comment - Hi Gary, Could you please review this patch? I wasn't able to find a way to create symlinks under Windows (FAT doesn't seem to support it) so the test code only checks under non-windows systems.
        Hide
        Gary Gregory added a comment - - edited

        On Windows, the sym link is called an NTFS junction point. This has been available since Windows 2000 according to https://en.wikipedia.org/wiki/NTFS_symbolic_link

        MKLINK [[/D] | [/H] | [/J]] Link Target
        
                /D      Creates a directory symbolic link.  Default is a file
                        symbolic link.
                /H      Creates a hard link instead of a symbolic link.
                /J      Creates a Directory Junction.
                Link    specifies the new symbolic link name.
                Target  specifies the path (relative or absolute) that the new link
                        refers to.
        

        Can you inlude Windows support in your patch? I am on Windows myself.

        Thank you!

        Show
        Gary Gregory added a comment - - edited On Windows, the sym link is called an NTFS junction point. This has been available since Windows 2000 according to https://en.wikipedia.org/wiki/NTFS_symbolic_link MKLINK [[/D] | [/H] | [/J]] Link Target /D Creates a directory symbolic link. Default is a file symbolic link. /H Creates a hard link instead of a symbolic link. /J Creates a Directory Junction. Link specifies the new symbolic link name. Target specifies the path (relative or absolute) that the new link refers to. Can you inlude Windows support in your patch? I am on Windows myself. Thank you!
        Hide
        Ravi Prakash added a comment -

        Thanks for the review and pointer Gary! I've updated the patch for windows. I'm afraid I do not have a Windows machine to test this on. Could you please?

        Show
        Ravi Prakash added a comment - Thanks for the review and pointer Gary! I've updated the patch for windows. I'm afraid I do not have a Windows machine to test this on. Could you please?
        Hide
        Gary Gregory added a comment -

        Thank you Ravi, the patch applies and tests OK but... How can this really work when FileUtils.isSymlink(File file) always returns true on Windows?

        Show
        Gary Gregory added a comment - Thank you Ravi, the patch applies and tests OK but... How can this really work when FileUtils.isSymlink(File file) always returns true on Windows?
        Hide
        Ravi Prakash added a comment -

        Thanks Gary! Aah! I did not notice that. You probably meant isSymLink always returns false

        Note: the current implementation always returns false if the system is detected as Windows using FilenameUtils#isSystemWindows()

        causing isSymLink to always be true on Windows. I guess the real fix would be to make the isSymlink() method not do that. Could you please append to the patch and fix it on Windows? I'm sorry I don't have a Windows machine

        Show
        Ravi Prakash added a comment - Thanks Gary! Aah! I did not notice that. You probably meant isSymLink always returns false Note: the current implementation always returns false if the system is detected as Windows using FilenameUtils#isSystemWindows() causing isSymLink to always be true on Windows. I guess the real fix would be to make the isSymlink() method not do that. Could you please append to the patch and fix it on Windows? I'm sorry I don't have a Windows machine
        Hide
        Sebb added a comment -

        Symbolic links are likely to be very rare on Windows.
        IMO it does not matter if the patch does not fix the crash for Windows hosts, so long as it does not otherwise change the behaviour on Windows.

        Show
        Sebb added a comment - Symbolic links are likely to be very rare on Windows. IMO it does not matter if the patch does not fix the crash for Windows hosts, so long as it does not otherwise change the behaviour on Windows.
        Hide
        Gary Gregory added a comment -

        Ok, I'll plan to apply the patch after 2.3 is out or if 2.3 requires a new RC.

        Show
        Gary Gregory added a comment - Ok, I'll plan to apply the patch after 2.3 is out or if 2.3 requires a new RC.
        Hide
        Gary Gregory added a comment -

        In SVN.

        Show
        Gary Gregory added a comment - In SVN.

          People

          • Assignee:
            Unassigned
            Reporter:
            Ravi Prakash
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development