FindBugs: Cleanup the File listting logic, listFiles() is evil

jenkinsci · Jun 12, 2017 · 466393e · 466393e
1 parent 7e3b20e
commit 466393e
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 21 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/periodicbackup/ConfigOnly.java b/src/main/java/org/jenkinsci/plugins/periodicbackup/ConfigOnly.java
@@ -35,6 +35,7 @@
 import java.io.FileFilter;
 import java.util.Arrays;
 import java.util.List;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.model.Jenkins;
 
@@ -68,30 +69,35 @@ public Iterable<File> getFilesToBackup() throws PeriodicBackupException {
         return filesToBackup;
     }
 
-    private void addRootFiles(File rootDir, List<File> filesToBackup) {
+    private void addRootFiles(File rootDir, List<File> filesToBackup) throws PeriodicBackupException {
         // First find the xml files in the home directory
-        File[] xmlsInRoot = rootDir.listFiles(Util.extensionFileFilter("xml"));
+        final File[] xmlsInRoot = Util.listFiles(rootDir, Util.extensionFileFilter("xml"));
         filesToBackup.addAll(Arrays.asList(xmlsInRoot));
     }
 
-    private void addJobFiles(File rootDir, List<File> filesToBackup) {
+    private void addJobFiles(File rootDir, List<File> filesToBackup) throws PeriodicBackupException {
         File jobsDir = new File(rootDir, "jobs");
         if (jobsDir.exists() && jobsDir.isDirectory()) {
             // Each job directory should have a config.xml file
-            File[] dirsInJobs = jobsDir.listFiles((FileFilter) FileFilterUtils.directoryFileFilter());
+            File[] dirsInJobs = Util.listFiles(jobsDir, FileFilterUtils.directoryFileFilter());
+
             for (File job : dirsInJobs) {
                 File jobConfig = new File(job, "config.xml");
                 if (jobConfig.exists() && jobConfig.isFile()) {
                     filesToBackup.add(jobConfig);
                     // There might be some config file from the Promotion plugin
                     File promotionDir = new File(job, "promotions");
                     if (promotionDir.exists()) {
-                        File[] promotionDirs = promotionDir.listFiles((FileFilter) FileFilterUtils.directoryFileFilter());
-                        for (File dir : promotionDirs) {
-                            File promotionConfig = new File(dir, "config.xml");
-                            if (promotionConfig.exists() && promotionConfig.isFile()) {
-                                filesToBackup.add(promotionConfig);
+                        try {
+                            File[] promotionDirs = Util.listFiles(promotionDir, FileFilterUtils.directoryFileFilter());
+                            for (File dir : promotionDirs) {
+                                File promotionConfig = new File(dir, "config.xml");
+                                if (promotionConfig.exists() && promotionConfig.isFile()) {
+                                    filesToBackup.add(promotionConfig);
+                                }
                             }
+                        } catch(PeriodicBackupException ex) {
+                            LOGGER.log(Level.WARNING, "Skipping the promotions for Job directory " + promotionDir.getAbsolutePath(), ex);
                         }
                     }
                 }
@@ -102,11 +108,16 @@ private void addJobFiles(File rootDir, List<File> filesToBackup) {
         }
     }
 
-    private void addUserFiles(File rootDir, List<File> filesToBackup) {
+    private void addUserFiles(File rootDir, List<File> filesToBackup) throws PeriodicBackupException {
         File usersDir = new File(rootDir, "users");
         if (usersDir.exists() && usersDir.isDirectory()) {
             // Each user directory should have a config.xml file
             File[] dirsInUsers = usersDir.listFiles((FileFilter) FileFilterUtils.directoryFileFilter());
+            if (dirsInUsers == null) {
+                // First one may happen only due to the race condition
+                throw new PeriodicBackupException("Job directory path does not denote or there is an I/O error");
+            }
+
             for (File user : dirsInUsers) {
                 File userConfig = new File(user, "config.xml");
                 if (userConfig.exists() && userConfig.isFile()) {

diff --git a/src/main/java/org/jenkinsci/plugins/periodicbackup/LocalDirectory.java b/src/main/java/org/jenkinsci/plugins/periodicbackup/LocalDirectory.java
@@ -41,6 +41,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -64,10 +65,19 @@ public Iterable<BackupObject> getAvailableBackups() {
             LOGGER.warning(path.getAbsolutePath() + " is not a existing/writable directory.");
             return Sets.newHashSet();
         }
-        if(path.listFiles().length == 0) {
+
+        FileFilter extensionFileFilter = Util.extensionFileFilter(BackupObject.EXTENSION);
+        final File[] files;
+        try {
+            files = Util.listFiles(path, extensionFileFilter);
+        } catch(PeriodicBackupException ex) {
+            LOGGER.log(Level.WARNING, "Cannot retrieve extension files from " + path.getAbsolutePath(), ex);
+            return Sets.newHashSet();
+        }
+
+        if(files.length == 0) {
             return Sets.newHashSet();
         }
-        File[] files = path.listFiles(Util.extensionFileFilter(BackupObject.EXTENSION));
         List<File> backupObjectFiles = Lists.newArrayList(files);
         // The sorting will be performed according to the timestamp
         Collections.sort(backupObjectFiles);
@@ -100,7 +110,7 @@ public void storeBackupInLocation(Iterable<File> archives, File backupObjectFile
     @Override
     public Iterable<File> retrieveBackupFromLocation(final BackupObject backup, File tempDir) throws IOException, PeriodicBackupException {
         // Get the list of archive files related to the given BackupObject
-        File[] files = path.listFiles(new FileFilter() {
+        File[] files = Util.listFiles(path, new FileFilter() {
             public boolean accept(File pathname) {
                 return (pathname.getName().contains( Util.getFormattedDate(BackupObject.FILE_TIMESTAMP_PATTERN, backup.getTimestamp())) &&
                         !pathname.getName().endsWith(BackupObject.EXTENSION));
@@ -141,7 +151,13 @@ public boolean accept(File pathname) {
     @Override
     public void deleteBackupFiles(BackupObject backupObject) {
         String filenamePart = Util.generateFileNameBase(backupObject.getTimestamp());
-        File[] files = path.listFiles();
+        final File[] files;
+        try {
+            files = Util.listFiles(path);
+        } catch (PeriodicBackupException ex) {
+            LOGGER.log(Level.WARNING, "Cannot enumerate the backup files", ex);
+            return;
+        }
 
         // Delete all the files containing the timestamp of the given BackupObject in their names
         for(File file : files) {

diff --git a/src/main/java/org/jenkinsci/plugins/periodicbackup/PeriodicBackupException.java b/src/main/java/org/jenkinsci/plugins/periodicbackup/PeriodicBackupException.java
@@ -28,4 +28,8 @@ class PeriodicBackupException extends Exception {
     public PeriodicBackupException(String msg) {
         super(msg);
     }
+
+    public PeriodicBackupException(String msg, Throwable cause) {
+        super(msg, cause);
+    }
 }
diff --git a/src/main/java/org/jenkinsci/plugins/periodicbackup/ReplaceRestorePolicy.java b/src/main/java/org/jenkinsci/plugins/periodicbackup/ReplaceRestorePolicy.java
@@ -31,6 +31,7 @@
 import java.io.IOException;
 import java.util.List;
 import java.util.logging.Logger;
+import javax.annotation.Nonnull;
 import jenkins.model.Jenkins;
 
 /**
@@ -55,9 +56,9 @@ public void restore(File tempDir) throws IOException, PeriodicBackupException {
         filesReplaced = 0;
         filesKept = 0;
 
-        deleteAccessible(hudsonRoot.listFiles());
+        deleteAccessible(Util.listFiles(hudsonRoot));
         LOGGER.info(filesDeleted + " files have been deleted from " + hudsonRoot.getAbsolutePath());
-        replaceAccessible(tempDir.listFiles(), tempDir);
+        replaceAccessible(Util.listFiles(tempDir), tempDir);
         LOGGER.info("Replacing of files finished.\nAfter deleting " + filesDeleted + " files from " +
                 hudsonRoot.getAbsolutePath() + "\n" + filesReplaced + " files have been restored from backup and "
                 + filesKept + " files have been kept.");
@@ -68,8 +69,9 @@ public void restore(File tempDir) throws IOException, PeriodicBackupException {
      * Attempt to recursively delete all accessible files from the given files array.
      *
      * @param files array of File objects given in order to be deleted
+     * @throws PeriodicBackupException Issue with listing one of the child directories
      */
-    private void deleteAccessible(File[] files) {
+    private void deleteAccessible(@Nonnull File[] files) throws PeriodicBackupException {
         String relativePath;
         for(File file : files) {
             if(!file.isDirectory()) {
@@ -90,7 +92,7 @@ private void deleteAccessible(File[] files) {
                 }
             }
             else {
-                deleteAccessible(file.listFiles());
+                deleteAccessible(Util.listFiles(file));
             }
         }
     }
@@ -103,8 +105,9 @@ private void deleteAccessible(File[] files) {
      * @param files to copy
      * @param tempDir temporary directory where @files are placed
      * @throws IOException If an IO problem occurs
+     * @throws PeriodicBackupException Issue with listing one of the directories
      */
-    private void replaceAccessible(File[] files, File tempDir) throws IOException {
+    private void replaceAccessible(File[] files, File tempDir) throws IOException, PeriodicBackupException {
         String relativePath;
         File destinationFile;
         for(File file : files) {
@@ -125,7 +128,7 @@ else if(autoExclusionList != null && autoExclusionList.contains(relativePath)) {
                 }
             }
             else {
-                replaceAccessible(file.listFiles(), tempDir);
+                replaceAccessible(Util.listFiles(file), tempDir);
             }
         }
     }

diff --git a/src/main/java/org/jenkinsci/plugins/periodicbackup/RestoreExecutor.java b/src/main/java/org/jenkinsci/plugins/periodicbackup/RestoreExecutor.java
@@ -64,7 +64,15 @@ public void run() {
         File finalResultDir = new File(tempDir, "finalResult");
 
         // The /finalResult directory should be empty at this point
-        File[] finalResultDirFileList = finalResultDir.listFiles();
+        File[] finalResultDirFileList;
+        try {
+            finalResultDirFileList = Util.listFiles(finalResultDir);
+        } catch (PeriodicBackupException ex) {
+            LOGGER.log(Level.WARNING, "Restoration Failure! Cannot list contents of " + finalResultDir.getAbsolutePath(), ex);
+            PeriodicBackupLink.get().setMessage("");
+            return;
+        }
+
         if(finalResultDir.exists() && finalResultDirFileList.length > 0) {
             LOGGER.warning("The final result directory " + finalResultDir.getAbsolutePath() + " is not empty, deleting...");
             try {

diff --git a/src/main/java/org/jenkinsci/plugins/periodicbackup/Util.java b/src/main/java/org/jenkinsci/plugins/periodicbackup/Util.java
@@ -32,6 +32,10 @@
 import java.io.IOException;
 import java.text.SimpleDateFormat;
 import java.util.Date;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 public class Util {
     /**
@@ -156,4 +160,44 @@ public static String getExtension(File f) {
     public static boolean isWritableDirectory(File directory) {
         return (directory.exists() && directory.isDirectory() && directory.canWrite());
     }
+
+    public static File[] listFiles(@Nonnull File directory) throws PeriodicBackupException {
+        return listFiles(directory, null);
+    }
+
+    /**
+     * Secure version of the listFiles() logic
+     * @param directory Directory to be listed
+     * @param fileFilter Optional file filter
+     * @return Files in the directory
+     * @throws PeriodicBackupException Whatever error
+     */
+    @Nonnull
+    @Restricted(NoExternalUse.class)
+    public static File[] listFiles(@Nonnull File directory, @CheckForNull FileFilter fileFilter) throws PeriodicBackupException {
+        if (!directory.isDirectory()) {
+            throw new PeriodicBackupException(formatMessage(directory, "File is not a directory"));
+        }
+
+        final File[] files;
+        try {
+            files = fileFilter == null ? directory.listFiles() : directory.listFiles(fileFilter);
+        } catch(SecurityException ex) {
+            throw new PeriodicBackupException(formatMessage(directory, "Securiy exception while listing files"), ex);
+        }
+
+        if (files == null) {
+            throw new PeriodicBackupException(formatMessage(directory, "It is not a valid directory or there is an I/O error"));
+        }
+
+        return files;
+    }
+
+    private static String formatMessage(@Nonnull File directory, @Nonnull String extra) {
+        StringBuilder bldr = new StringBuilder("Cannot list files of ");
+        bldr.append(directory.getAbsolutePath());
+        bldr.append(". ");
+        bldr.append(extra);
+        return bldr.toString();
+    }
 }