FOP-3193: Avoid recursion when folders contain symlinks

Author: Joao Goncalves

fop-core/src/main/java/org/apache/fop/fonts/DefaultFontConfigurator.java

@@ -19,6 +19,7 @@
 
 package org.apache.fop.fonts;
 
+import java.io.File;
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -118,7 +119,7 @@ private void addDirectories(DefaultFontConfig fontInfoConfig, FontAdder fontAdde
             FontFileFinder fontFileFinder = new FontFileFinder(directory.isRecursive() ? -1 : 1, listener);
             List<URL> fontURLList;
             try {
-                fontURLList = fontFileFinder.find(directory.getDirectory());
+                fontURLList = fontFileFinder.find(new File(directory.getDirectory()));
                 fontAdder.add(fontURLList, fontInfoList);
             } catch (IOException e) {
                 LogUtil.handleException(log, e, strict);

fop-core/src/main/java/org/apache/fop/fonts/FontDetectorFactory.java

@@ -93,7 +93,7 @@ public void detect(FontManager fontManager, FontAdder fontAdder, boolean strict,
                 URI fontBaseURI = fontManager.getResourceResolver().getBaseURI();
                 File fontBase = FileUtils.toFile(fontBaseURI.toURL());
                 if (fontBase != null) {
-                    List<URL> fontURLList = fontFileFinder.find(fontBase.getAbsolutePath());
+                    List<URL> fontURLList = fontFileFinder.find(fontBase);
                     fontAdder.add(fontURLList, fontInfoList);
 
                     //Can only use the font base URL if it's a file URL

fop-core/src/main/java/org/apache/fop/fonts/autodetect/FontFileFinder.java

@@ -20,11 +20,16 @@
 package org.apache.fop.fonts.autodetect;
 
 import java.io.File;
+import java.io.FileFilter;
 import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.nio.file.Files;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 
 import org.apache.commons.io.DirectoryWalker;
 import org.apache.commons.io.IOCase;
@@ -150,28 +155,89 @@ public List<URL> find() throws IOException {
             }
         }
         List<File> fontDirs = fontDirFinder.find();
-        List<URL> results = new java.util.ArrayList<URL>();
+        List<URL> results = new java.util.ArrayList<>();
         for (File dir : fontDirs) {
-            super.walk(dir, results);
+            walkDirectory(dir, results);
         }
         return results;
     }
 
     /**
      * Searches a given directory for font files
      *
-     * @param dir directory to search
+     * @param directory directory to search
      * @return list of font files
      * @throws IOException thrown if an I/O exception of some sort has occurred
      */
-    public List<URL> find(String dir) throws IOException {
-        List<URL> results = new java.util.ArrayList<URL>();
-        File directory = new File(dir);
+    public List<URL> find(File directory) throws IOException {
+        List<URL> results = new java.util.ArrayList<>();
         if (!directory.isDirectory()) {
-            eventListener.fontDirectoryNotFound(this, dir);
+            eventListener.fontDirectoryNotFound(this, directory.getAbsolutePath());
         } else {
-            super.walk(directory, results);
+            walkDirectory(directory, results);
         }
         return results;
     }
+
+    private void walkDirectory(File startDirectory, Collection<URL> results) throws IOException {
+        Objects.requireNonNull(startDirectory, "startDirectory");
+
+        try {
+            walk(startDirectory, 0, results, new HashMap<>());
+        } catch (CancelException cancel) {
+            handleCancelled(startDirectory, results, cancel);
+        }
+
+    }
+
+    private void walk(File directory, int depth, Collection<URL> results,
+                      Map<String, String> visitedSymlinks) throws IOException {
+        IOFileFilter fileFilter = FileFilterUtils.makeFileOnly(getFileFilter());
+        IOFileFilter directoryFilter = FileFilterUtils.makeDirectoryOnly(getDirectoryFilter());
+        FileFilter filter = directoryFilter.or(fileFilter);
+
+        checkIfCancelled(directory, depth, results);
+
+        int childDepth = depth + 1;
+
+        File[] childFiles = directory.listFiles(filter);
+        if (childFiles != null) {
+            for (File childFile : childFiles) {
+                if (childFile.isDirectory()) {
+                    if (Files.isSymbolicLink(childFile.toPath()) && hasSymlinkBeenWalked(childFile, visitedSymlinks)) {
+                        continue;
+                    }
+                    walk(childFile, childDepth, results, visitedSymlinks);
+                } else {
+                    checkIfCancelled(childFile, childDepth, results);
+                    handleFile(childFile, childDepth, results);
+                    checkIfCancelled(childFile, childDepth, results);
+                }
+            }
+        }
+
+        handleDirectoryEnd(directory, depth, results);
+        checkIfCancelled(directory, depth, results);
+    }
+
+    private boolean hasSymlinkBeenWalked(File symlink, Map<String, String> visitedSymlinks) {
+        String symlinkPath;
+        try {
+            symlinkPath = symlink.toPath().toRealPath().toString();
+        } catch (IOException e) {
+            log.warn("Failed to get symlink path: " + e.getMessage());
+            symlinkPath = symlink.getAbsolutePath();
+        }
+        for (Map.Entry<String, String> entry : visitedSymlinks.entrySet()) {
+            //startsWith being used in the event there's an exception in the call above
+            //if there's no exception, startsWith will work as an equals
+            if (symlinkPath.startsWith(entry.getKey()) && entry.getValue().equals(symlink.getName())) {
+                return true;
+            }
+        }
+
+        visitedSymlinks.put(symlinkPath, symlink.getName());
+
+        return false;
+    }
 }

fop-core/src/test/java/org/apache/fop/fonts/autodetect/FontFileFinderTestCase.java

@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/* $Id$ */
+
+package org.apache.fop.fonts.autodetect;
+
+import java.io.File;
+import java.io.FileFilter;
+import java.io.IOException;
+import java.nio.file.FileSystem;
+import java.nio.file.Path;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.nio.file.spi.FileSystemProvider;
+
+import org.junit.Test;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class FontFileFinderTestCase {
+
+    @Test
+    public void testSymlinkRecursion() throws IOException {
+        FontFileFinder finder = new FontFileFinder(null);
+
+        File targetDirectory = createMockDirectory("target");
+        File linkDirectory = createMockDirectory("link");
+
+        setupChildFiles(targetDirectory, linkDirectory);
+        setupChildFiles(linkDirectory, targetDirectory);
+
+        setupSymLink(linkDirectory.toPath());
+
+        finder.find(targetDirectory);
+
+        //targetDir -> linkDir -> targetDir -> linkDir (skipped as it has been seen)
+        //it's repeating the folder because we need to access the symlink once only.
+        //in this scenario, the symlink is recursive, hence the repetition
+        verify(targetDirectory, times(2)).listFiles(any(FileFilter.class));
+        verify(linkDirectory, times(1)).listFiles(any(FileFilter.class));
+
+        //this is only accessed for symlinks
+        // it's used to make sure we don't access the same synlink twice
+        verify(targetDirectory.toPath(), times(0)).toRealPath();
+        verify(linkDirectory.toPath(), times(2)).toRealPath();
+    }
+
+    private File createMockDirectory(String path) throws IOException {
+        Path mockRealPath = mock(Path.class);
+        when(mockRealPath.toString()).thenReturn(path);
+
+        Path mockPath = mock(Path.class);
+        when(mockPath.toRealPath()).thenReturn(mockRealPath);
+        setupPathFileSystem(mockPath, false);
+
+        File mockDir = mock(File.class);
+        when(mockDir.isDirectory()).thenReturn(true);
+        when(mockDir.toPath()).thenReturn(mockPath);
+        when(mockDir.getName()).thenReturn(path);
+
+        return mockDir;
+    }
+
+    private void setupChildFiles(File dir, File childFile) {
+        when(dir.listFiles(any(FileFilter.class))).thenReturn(new File[]{childFile});
+    }
+
+    private void setupSymLink(Path linkPath) throws IOException {
+        setupPathFileSystem(linkPath, true);
+    }
+
+    private void setupPathFileSystem(Path path, boolean symlink) throws IOException {
+        BasicFileAttributes mockBasicFileAttributes = mock(BasicFileAttributes.class);
+        when(mockBasicFileAttributes.isSymbolicLink()).thenReturn(symlink);
+
+        FileSystemProvider mockProvider = mock(FileSystemProvider.class);
+        when(mockProvider.readAttributes(any(), any(Class.class), any())).thenReturn(mockBasicFileAttributes);
+
+        FileSystem mockFileSystem = mock(FileSystem.class);
+        when(mockFileSystem.provider()).thenReturn(mockProvider);
+        when(path.getFileSystem()).thenReturn(mockFileSystem);
+    }
+}