Merge pull request #935 from karlmortensen/code_review_comments_photorec

Code review comments for PhotoRec module

Core/src/org/sleuthkit/autopsy/modules/photoreccarver/Bundle.properties

@@ -5,7 +5,6 @@ OpenIDE-Module-Short-Description=Carves unallocated space and feeds carved files
 unallocatedSpaceProcessingSettingsError.message="Process Unallocated Space" is not checked. This module is designed to carve unallocated space. Either allow processing of unallocated space, or do not use this module.
 moduleDisplayName.text=PhotoRec Carver
 moduleDescription.text=Runs PhotoRec carver against unallocated space on the system.
-unrecognizedSettings.message=Settings not instanceof org.sleuthkit.autopsy.modules.photoreccarver
 unsupportedOS.message=Module is not supported for other than Windows platforms
 missingExecutable.message=Unable to locate unallocated carver executable.
 cannotRunExecutable.message=Unable to execute unallocated carver

Core/src/org/sleuthkit/autopsy/modules/photoreccarver/PhotoRecCarverFileIngestModule.java

@@ -75,16 +75,6 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
     private Path rootOutputDirPath;
     private File executableFile;
 
-    /**
-     * Constructs a file ingest module that runs the Unallocated Carver executable with unallocated space files as
-     * input.
-     *
-     * @param None
-     */
-    PhotoRecCarverFileIngestModule() {
-
-    }
-
     /**
      * @inheritDoc
      */
@@ -108,13 +98,7 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
 
         if (PhotoRecCarverFileIngestModule.refCounter.incrementAndGet(this.context.getJobId()) == 1) {
             try {
-                // The first instance of the module for an ingest job creates 
+                // The first instance creates an output subdirectory with a date and time stamp
-                // a time-stamped output subdirectory of the unallocated space
-                // scans subdirectory of the Unallocated Carver module output  
-                // directory for the current case. 
-
-                // Make output subdirectories for the current time and image within
-                // the module output directory for the current case.
                 DateFormat dateFormat = new SimpleDateFormat("MM-dd-yyyy-HH-mm-ss-SSSS");  // NON-NLS
                 Date date = new Date();
                 String folder = this.context.getDataSource().getId() + "_" + dateFormat.format(date);
@@ -154,16 +138,15 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
 
             // Verify initialization succeeded.
             if (null == this.executableFile) {
-                logger.log(Level.SEVERE, "Unallocated Carver unallocated space ingest module called after failed start up");  // NON-NLS
+                logger.log(Level.SEVERE, "PhotoRec carver called after failed start up");  // NON-NLS
                 return IngestModule.ProcessResult.ERROR;
             }
 
             // Check that we have roughly enough disk space left to complete the operation
             long freeDiskSpace = IngestServices.getInstance().getFreeDiskSpace();
             if ((file.getSize() * 2) > freeDiskSpace) {
-                logger.log(Level.SEVERE, "Error processing " + file.getName() + " with "
+                logger.log(Level.SEVERE, "PhotoRec error processing {0} with {1} Not enough space on primary disk to carve unallocated space.",
-                        + PhotoRecCarverIngestModuleFactory.getModuleName()
+                        new Object[]{file.getName(), PhotoRecCarverIngestModuleFactory.getModuleName()}); // NON-NLS
-                        + " Not enough space on primary disk to carve unallocated space."); // NON-NLS
                 return IngestModule.ProcessResult.ERROR;
             }
 
@@ -200,7 +183,7 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
                 if (null != tempFilePath && Files.exists(tempFilePath)) {
                     tempFilePath.toFile().delete();
                 }
-                logger.log(Level.INFO, "Cancelled by user"); // NON-NLS
+                logger.log(Level.INFO, "PhotoRec cancelled by user"); // NON-NLS
                 return IngestModule.ProcessResult.OK;
             }
 
@@ -211,7 +194,7 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
                 if (null != tempFilePath && Files.exists(tempFilePath)) {
                     tempFilePath.toFile().delete();
                 }
-                logger.log(Level.SEVERE, "Unallocated Carver returned error exit value = {0} when scanning {1}",
+                logger.log(Level.SEVERE, "PhotoRec carver returned error exit value = {0} when scanning {1}",
                         new Object[]{exitValue, file.getName()}); // NON-NLS
                 return IngestModule.ProcessResult.ERROR;
             }
@@ -233,11 +216,11 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
             PhotoRecCarverOutputParser parser = new PhotoRecCarverOutputParser(outputDirPath);
             List<LayoutFile> theList = parser.parse(newAuditFile, id, file);
             if (theList != null) { // if there were any results from carving, add the unallocated carving event to the reports list.
-                context.scheduleFiles(new ArrayList<AbstractFile>(theList));
+                context.scheduleFiles(new ArrayList<>(theList));
             }
         }
         catch (IOException ex) {
-            logger.log(Level.SEVERE, "Error processing " + file.getName() + " with Unallocated Carver", ex); // NON-NLS
+            logger.log(Level.SEVERE, "Error processing " + file.getName() + " with PhotoRec carver", ex); // NON-NLS
             return IngestModule.ProcessResult.ERROR;
         }
 
@@ -264,7 +247,7 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
                 FileUtil.deleteDir(new File(paths.getTempDirPath().toString()));
             }
             catch (SecurityException ex) {
-                logger.log(Level.SEVERE, "Error shutting down Unallocated Carver unallocated space module", ex); // NON-NLS
+                logger.log(Level.SEVERE, "Error shutting down PhotoRec carver module", ex); // NON-NLS
             }
         }
     }
@@ -328,28 +311,11 @@ final class PhotoRecCarverFileIngestModule implements FileIngestModule {
             }
         }
         catch (TskCoreException ex) {
-            logger.log(Level.SEVERE, "Exception while trying to get parent of AbstractFile.", ex); //NON-NLS
+            logger.log(Level.SEVERE, "PhotoRec carver exception while trying to get parent of AbstractFile.", ex); //NON-NLS
         }
         return id;
     }
 
-    /**
-     * Determines whether or not a directory is empty.
-     *
-     * @param directoryPath The path to the directory to inspect.
-     * @return True if the directory is empty, false otherwise.
-     * @throws IllegalArgumentException
-     * @throws IOException
-     */
-    private static boolean isDirectoryEmpty(final Path directoryPath) throws IllegalArgumentException, IOException {
-        if (!Files.isDirectory(directoryPath)) {
-            throw new IllegalArgumentException("The directoryPath argument must be a directory path"); // NON-NLS
-        }
-        try (DirectoryStream<Path> dirStream = Files.newDirectoryStream(directoryPath)) {
-            return !dirStream.iterator().hasNext();
-        }
-    }
-
     /**
      * Finds and returns the path to the executable, if able.
      *

Core/src/org/sleuthkit/autopsy/modules/photoreccarver/PhotoRecCarverIngestModuleFactory.java

@@ -25,22 +25,19 @@ import org.sleuthkit.autopsy.ingest.FileIngestModule;
 import org.sleuthkit.autopsy.ingest.IngestModuleFactory;
 import org.sleuthkit.autopsy.ingest.IngestModuleFactoryAdapter;
 import org.sleuthkit.autopsy.ingest.IngestModuleIngestJobSettings;
-import org.sleuthkit.autopsy.ingest.IngestModuleIngestJobSettingsPanel;
 
 /**
  * A factory for creating instances of file ingest modules that carve unallocated space
  */
 @ServiceProvider(service = IngestModuleFactory.class)
-public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapter
+public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapter {
-{
 
     /**
      * Gets the ingest module name for use within this package.
      *
      * @return A name string.
      */
-    static String getModuleName()
+    static String getModuleName() {
-    {
         return NbBundle.getMessage(PhotoRecCarverIngestModuleFactory.class, "moduleDisplayName.text");
     }
 
@@ -48,8 +45,7 @@ public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapte
      * @inheritDoc
      */
     @Override
-    public String getModuleDisplayName()
+    public String getModuleDisplayName() {
-    {
         return PhotoRecCarverIngestModuleFactory.getModuleName();
     }
 
@@ -57,8 +53,7 @@ public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapte
      * @inheritDoc
      */
     @Override
-    public String getModuleDescription()
+    public String getModuleDescription() {
-    {
         return NbBundle.getMessage(PhotoRecCarverIngestModuleFactory.class, "moduleDescription.text");
     }
 
@@ -66,8 +61,7 @@ public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapte
      * @inheritDoc
      */
     @Override
-    public String getModuleVersionNumber()
+    public String getModuleVersionNumber() {
-    {
         return Version.getVersion();
     }
 
@@ -75,8 +69,7 @@ public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapte
      * @inheritDoc
      */
     @Override
-    public boolean isFileIngestModuleFactory()
+    public boolean isFileIngestModuleFactory() {
-    {
         return true;
     }
 
@@ -84,13 +77,7 @@ public class PhotoRecCarverIngestModuleFactory extends IngestModuleFactoryAdapte
      * @inheritDoc
      */
     @Override
-    public FileIngestModule createFileIngestModule(IngestModuleIngestJobSettings settings)
+    public FileIngestModule createFileIngestModule(IngestModuleIngestJobSettings settings) {
-    {
-        if (!(settings instanceof IngestModuleIngestJobSettings))
-        {
-            throw new IllegalArgumentException(NbBundle.getMessage(PhotoRecCarverIngestModuleFactory.class, "unrecognizedSettings.message"));
-        }
         return new PhotoRecCarverFileIngestModule();
     }
-
 }

Core/src/org/sleuthkit/autopsy/modules/photoreccarver/PhotoRecCarverOutputParser.java

@@ -26,6 +26,7 @@ import java.io.IOException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.logging.Level;
 import org.sleuthkit.autopsy.casemodule.Case;
@@ -40,12 +41,12 @@ import org.sleuthkit.datamodel.TskFileRange;
 /**
  * This class parses the xml output from PhotoRec, and creates a list of entries to add back in to be processed.
  */
-public class PhotoRecCarverOutputParser {
+class PhotoRecCarverOutputParser {
 
-    Path basePath;
+    private final Path basePath;
     private static final Logger logger = Logger.getLogger(PhotoRecCarverFileIngestModule.class.getName());
 
-    public PhotoRecCarverOutputParser(Path base) {
+    PhotoRecCarverOutputParser(Path base) {
         basePath = base;
     }
 
@@ -56,12 +57,13 @@ public class PhotoRecCarverOutputParser {
      * @param line The line in which we are looking for the element.
      * @return The String value found
      */
-    public String getValue(String name, String line) {
+    private static String getValue(String name, String line) {
         return line.replaceAll("[\t ]*</?" + name + ">", ""); //NON-NLS
     }
 
     /**
-     * Gets the value inside the XML element and returns it. Ignores leading whitespace.
+     * Parses the given report.xml file, creating a List<LayoutFile> to return. Uses FileManager to add all carved files
+     * that it finds to the TSK database as $CarvedFiles under the passed-in parent id.
      *
      * @param xmlInputFile The XML file we are trying to read and parse
      * @param id The parent id of the unallocated space we are parsing.
@@ -70,8 +72,8 @@ public class PhotoRecCarverOutputParser {
      * @throws FileNotFoundException
      * @throws IOException
      */
-    public List<LayoutFile> parse(File xmlInputFile, long id, AbstractFile af) throws FileNotFoundException, IOException {
+    List<LayoutFile> parse(File xmlInputFile, long id, AbstractFile af) throws FileNotFoundException, IOException {
-        try {
+        try (BufferedReader in = new BufferedReader(new FileReader(xmlInputFile))) {
             String fileName;
             long fileSize;
             String result;
@@ -80,10 +82,7 @@ public class PhotoRecCarverOutputParser {
             FileManager fileManager = Case.getCurrentCase().getServices().getFileManager();
 
             // create and initialize the list to put into the database
-            List<CarvedFileContainer> carvedFileContainer = new ArrayList<CarvedFileContainer>();
+            List<CarvedFileContainer> carvedFileContainer = new ArrayList<>();
-
-            // create a BufferedReader
-            BufferedReader in = new BufferedReader(new FileReader(xmlInputFile));
 
             // create and initialize a line
             String line = in.readLine();
@@ -95,12 +94,13 @@ public class PhotoRecCarverOutputParser {
                 {
                     if (line.equals("</dfxml>")) //NON-NLS
                     { // We have found the end. Break out of both loops and move on to processing.
+                        line = "";
                         break reachedEndOfFile;
                     }
                     line = in.readLine();
                 }
 
-                List<TskFileRange> ranges = new ArrayList<TskFileRange>();
+                List<TskFileRange> ranges = new ArrayList<>();
 
                 // read filename line
                 line = in.readLine();
@@ -130,13 +130,13 @@ public class PhotoRecCarverOutputParser {
                 }
                 carvedFileContainer.add(new CarvedFileContainer(fileName, fileSize, id, ranges));
             }
-
-            in.close(); // close the BufferedReader
             return fileManager.addCarvedFiles(carvedFileContainer);
         }
         catch (IOException | NumberFormatException | TskCoreException ex) {
-            logger.log(Level.SEVERE, "Error parsing PhotoRec output and inserting it into the database" + ex); //NON_NLS
+            logger.log(Level.SEVERE, "Error parsing PhotoRec output and inserting it into the database: {0}", ex); //NON_NLS
         }
-        return null;
+
+        List<LayoutFile> empty = Collections.emptyList();
+        return empty;
     }
 }