[Bug 62849] New: DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

            Bug ID: 62849
           Summary: DirectoryScanner does not efficiently handle
                    filesystem loops
           Product: Ant
           Version: 1.9.13
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Core
          Assignee: notifications@ant.apache.org
          Reporter: mikeb01@gmail.com
  Target Milestone: ---

If you have a large file structure being scanned and the file system contains a
loop caused by a symbolic link then it is possible for the directory scanner to
run out of memory before the 'maxLevelsOfSymlinks' protection kicks in.

This affects us in Jenkins which uses the DirectoryScanner to pick up artefacts
for publishing/archiving.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #3 from Michael Barker <mi...@gmail.com> ---
Created attachment 36210
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36210&action=edit
Patch to handle file system loop in directory scanner v2

Updated patch that contains more useful log message.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #8 from Michael Barker <mi...@gmail.com> ---
Created attachment 36231
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36231&action=edit
Patch to handle file system loop in directory scanner v4

This changes the delete task to similarly handle file system loops.  It won't
follow a symlink on delete if that symlink results in a file system loop.  It
will follow it in other cases.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #13 from Michael Barker <mi...@gmail.com> ---
I've fixed the problems caused by this issue elsewhere (Jenkins).  Feel free to
close.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #5 from Michael Barker <mi...@gmail.com> ---
Created attachment 36211
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36211&action=edit
Patch to handle file system loop in directory scanner v3

Use FileSystemUtils.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #10 from Michael Barker <mi...@gmail.com> ---
The names from those tests suggest that the functionality I've added has
changed the behaviour such that those tests are violated.  We may need to
remove those tests if we want to us my patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #6 from Jaikiran Pai <ja...@apache.org> ---
Hello Michael,

The v3 patch mostly looks fine to me. However, I'm not too sure about the
changes you have in the Delete task where you no longer delete a directory if
it is a symbolic link. I'll need inputs from Stefan and/or others on this one
before merging it.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #9 from Jaikiran Pai <ja...@apache.org> ---
Michael, I have now merged this patch (with minor changes that include change
to the error messages and a comment to the changed code) to our 1.9.x and
master branches.

It seems to have introduced a failure in an existing test[1]. I'll look into it
to see what needs to be done. I'll let this issue stay open until that test
failure is sorted out.

[1]
https://builds.apache.org/job/Ant-Build-Matrix-1.9.x-Linux/OS=xenial,jdk=JDK%201.5%20(latest)/286/

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #7 from Michael Barker <mi...@gmail.com> ---
The reason to change the delete task is that that unit test would get stuck in
a loop when I added the cases where there was a file system loop.  There are a
couple of options that I could take with the delete task:

1) Use the same loop detection as in the Directory scanner an not follow links
that create a file system loop.

2) Change the delete task to take a parameter of something like followSymLinks
(defaulted to true) so that existing behaviour remains the default and the new
behaviour is optional.

My preference is for #1 as it requires less input from a user to prevent it
from breaking.  I'll have a look at code some of this up this weekend.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #2 from Michael Barker <mi...@gmail.com> ---
I've attached a proposed patch for this issue.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #12 from Michael Barker <mi...@gmail.com> ---
The current scenario we are running into:

- We have a build with roughly the following structure.  This is an artefact of
the build tool we happen to be using (BUCK).

<project root>
  <source directory>
  ...
  <source directory>
  <build output>
     <symlink -> project root> A
     <output directory>
        <symlink -> A>
     ...
     <output directory>
        <symlink -> A>

The build has a total of around 1M files, of which ~800K are in the output
directory.  Note that we don't use Ant to build any of this.  Where this hits
us is that we use Jenkins for CI  and the Ant DirectoryScanner is used find
artefacts from the build to archive.

When Jenkins runs the scanner is continues to recurse the project through the
project root symlink.  Because the DirectoryScanner holds onto all of the files
it visits (not just the ones that match) it rapidly builds a very large list of
files.  Some heap debugging shows ~50M entries in the directory scanner before
the Jenkins archive task starts to fail with OOM errors.

I'll have a look later this week about trying to build a test case for this.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #11 from Jaikiran Pai <ja...@apache.org> ---
Hello Michael,

I have been away a bit, so couldn't respond earlier. Coming back to this, you
mention OOM issue that initiated this patch. Would it be possible to attach a
build file that reproduces this and also the exception stacktrace that you see
(without the patches)? That way, we can see if this can be solved in a
different manner.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #1 from Michael Barker <mi...@gmail.com> ---
Created attachment 36206
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36206&action=edit
Patch to handle file system loop in directory scanner.

This patch will skip over directories that create a file system loop.  It does
this by checking if the canonical path of a child directory is a prefix of the
canonical path of the parent.

There is a test included.

The test also highlighted that the delete task will follow symlinks, which can
also fall afoul of file system loops, so I've changed the delete task to only
follow directories that are not symlinks and just delete the symlink itself
instead.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 62849] DirectoryScanner does not efficiently handle filesystem loops

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62849

--- Comment #4 from Jaikiran Pai <ja...@apache.org> ---
Hello Michael,

I haven't thoroughly reviewed the patch or checked the implication of the
change. However, this change:

--- src/main/org/apache/tools/ant/DirectoryScanner.java (date 1531197234000)
+++ src/main/org/apache/tools/ant/DirectoryScanner.java (date 1540366425000)
@@ -1258,6 +1258,18 @@ 
             final String name = vpath + newFile;
             final TokenizedPath newPath = new TokenizedPath(path, newFile);
             final File file = new File(dir, newFile);
+
+            try {
+                String fileCanonicalPath = file.getCanonicalPath();
+                String dirCanonicalPath = dir.getCanonicalPath();
+                if (dirCanonicalPath.startsWith(fileCanonicalPath)) {
+                    continue;
+                }
+            } catch (IOException e) {
+                System.err.println("Failed to get canoncial paths to determine
" +
+                    "if filesyste loop exists, continuning");
+            }
+


Could you instead try something like this (just pseudo-code, haven't checked
for compile errors):

            try {
                if
(FileUtils.getFileUtils().isLeadingPath(file.getAbsoluteFile(),
dir.getAbsoluteFile(), true)) {
                    continue;
                }
            } catch (IOException e) {
                // log it
            }

The FileUtils#isLeadingPath with resolveSymlink = true would do the same as
what you have attempted in the patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.