GT-3063 sevenzip file system, code review fixes

Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/FileSystemService.java

@@ -448,15 +448,16 @@ public class FileSystemService {
 	 * lambda will be called and it will be responsible for returning an {@link InputStream}
 	 * which has the derived contents, which will be added to the file cache for next time.
 	 * <p>
-	 * @param fsrl {@link FSRL} of the source file that this derived file is based on.
+	 * @param fsrl {@link FSRL} of the source (or container) file that this derived file is based on
-	 * @param derivedName a unique string identifying the derived file.
+	 * @param derivedName a unique string identifying the derived file inside the source (or container) file
 	 * @param producer a {@link DerivedFileProducer callback or lambda} that returns an
 	 * {@link InputStream} that will be streamed into a file and placed into the file cache.
+	 * Example: <pre>(file) -> { return new XYZDecryptorInputStream(file); }</pre>
 	 * @param monitor {@link TaskMonitor} that will be monitor for cancel requests and updated
-	 * with file io progress.
+	 * with file io progress
-	 * @return {@link FileCacheEntry} with file and md5 fields.
+	 * @return {@link FileCacheEntry} with file and md5 fields
-	 * @throws CancelledException if the user cancels.
+	 * @throws CancelledException if the user cancels
-	 * @throws IOException if there was an io error.
+	 * @throws IOException if there was an io error
 	 */
 	public FileCacheEntry getDerivedFile(FSRL fsrl, String derivedName,
 			DerivedFileProducer producer, TaskMonitor monitor)
@@ -467,19 +468,16 @@ public class FileSystemService {
 		// case should be okay as the only bad result will be extra
 		// work being performed recreating the contents of the same derived file a second
 		// time.
-		FileCacheEntry srcCFI = getCacheFile(fsrl, monitor);
+		FileCacheEntry cacheEntry = getCacheFile(fsrl, monitor);
-		String derivedMD5 = fileCacheNameIndex.get(srcCFI.md5, derivedName);
+		String derivedMD5 = fileCacheNameIndex.get(cacheEntry.md5, derivedName);
 		FileCacheEntry derivedFile = (derivedMD5 != null) ? fileCache.getFile(derivedMD5) : null;
 		if (derivedFile == null) {
 			monitor.setMessage(derivedName + " " + fsrl.getName());
-			try (InputStream is = producer.produceDerivedStream(srcCFI.file)) {
+			try (InputStream is = producer.produceDerivedStream(cacheEntry.file)) {
 				derivedFile = fileCache.addStream(is, monitor);
-				fileCacheNameIndex.add(srcCFI.md5, derivedName, derivedFile.md5);
+				fileCacheNameIndex.add(cacheEntry.md5, derivedName, derivedFile.md5);
 			}
 		}
-		else {
-			Msg.info(null, "Found derived file in cache: " + fsrl + ", " + derivedName);
-		}
 		return derivedFile;
 	}
 
@@ -492,15 +490,15 @@ public class FileSystemService {
 	 * lambda will be called and it will be responsible for producing and writing the derived
 	 * file's bytes to a {@link OutputStream}, which will be added to the file cache for next time.
 	 * <p>
-	 * @param fsrl {@link FSRL} of the source file that this derived file is based on.
+	 * @param fsrl {@link FSRL} of the source (or container) file that this derived file is based on
-	 * @param derivedName a unique string identifying the derived file.
+	 * @param derivedName a unique string identifying the derived file inside the source (or container) file
 	 * @param pusher a {@link DerivedFilePushProducer callback or lambda} that recieves a {@link OutputStream}.
 	 * Example: <pre>(os) -> { ...write to outputstream os here...; }</pre>
 	 * @param monitor {@link TaskMonitor} that will be monitor for cancel requests and updated
-	 * with file io progress.
+	 * with file io progress
-	 * @return {@link FileCacheEntry} with file and md5 fields.
+	 * @return {@link FileCacheEntry} with file and md5 fields
-	 * @throws CancelledException if the user cancels.
+	 * @throws CancelledException if the user cancels
-	 * @throws IOException if there was an io error.
+	 * @throws IOException if there was an io error
 	 */
 	public FileCacheEntry getDerivedFilePush(FSRL fsrl, String derivedName,
 			DerivedFilePushProducer pusher, TaskMonitor monitor)
@@ -511,16 +509,13 @@ public class FileSystemService {
 		// case should be okay as the only bad result will be extra
 		// work being performed recreating the contents of the same derived file a second
 		// time.
-		FileCacheEntry srcCFI = getCacheFile(fsrl, monitor);
+		FileCacheEntry cacheEntry = getCacheFile(fsrl, monitor);
-		String derivedMD5 = fileCacheNameIndex.get(srcCFI.md5, derivedName);
+		String derivedMD5 = fileCacheNameIndex.get(cacheEntry.md5, derivedName);
 		FileCacheEntry derivedFile = (derivedMD5 != null) ? fileCache.getFile(derivedMD5) : null;
 		if (derivedFile == null) {
 			monitor.setMessage("Caching " + fsrl.getName() + " " + derivedName);
 			derivedFile = fileCache.pushStream(pusher, monitor);
-			fileCacheNameIndex.add(srcCFI.md5, derivedName, derivedFile.md5);
+			fileCacheNameIndex.add(cacheEntry.md5, derivedName, derivedFile.md5);
-		}
-		else {
-			//Msg.info(null, "Found derived file in cache: " + fsrl + ", " + derivedName);
 		}
 		return derivedFile;
 	}
@@ -537,8 +532,8 @@ public class FileSystemService {
 	 */
 	public boolean hasDerivedFile(FSRL fsrl, String derivedName, TaskMonitor monitor)
 			throws CancelledException, IOException {
-		FileCacheEntry srcCFI = getCacheFile(fsrl, monitor);
+		FileCacheEntry cacheEntry = getCacheFile(fsrl, monitor);
-		String derivedMD5 = fileCacheNameIndex.get(srcCFI.md5, derivedName);
+		String derivedMD5 = fileCacheNameIndex.get(cacheEntry.md5, derivedName);
 		return derivedMD5 != null;
 	}
 

Ghidra/Features/FileFormats/src/main/java/ghidra/file/formats/sevenzip/SevenZipFileSystem.java

@@ -33,7 +33,7 @@ import utilities.util.FileUtilities;
 @FileSystemInfo(type = "7zip", description = "7Zip", factory = SevenZipFileSystemFactory.class)
 public class SevenZipFileSystem implements GFileSystem {
 
-	private FileSystemService fsService;
+	private FileSystemService fileSystemService;
 	private FileSystemIndexHelper<ISimpleInArchiveItem> fsIndexHelper;
 	private FSRLRoot fsrl;
 	private FileSystemRefManager refManager = new FileSystemRefManager(this);
@@ -45,11 +45,21 @@ public class SevenZipFileSystem implements GFileSystem {
 	public SevenZipFileSystem(FSRLRoot fsrl) {
 		this.fsrl = fsrl;
 		this.fsIndexHelper = new FileSystemIndexHelper<>(this, fsrl);
-		this.fsService = FileSystemService.getInstance();
+		this.fileSystemService = FileSystemService.getInstance();
 	}
 
-	public void mount(File f, TaskMonitor monitor) throws CancelledException, IOException {
+	/**
-		randomAccessFile = new RandomAccessFile(f, "r");
+	 * Opens the specified sevenzip container file and initializes this file system with the
+	 * contents.
+	 *  
+	 * @param containerFile file to open
+	 * @param monitor {@link TaskMonitor} to allow the user to monitor and cancel
+	 * @throws CancelledException if user cancels
+	 * @throws IOException if error when reading data
+	 */
+	public void mount(File containerFile, TaskMonitor monitor)
+			throws CancelledException, IOException {
+		randomAccessFile = new RandomAccessFile(containerFile, "r");
 		try {
 			archive = SevenZip.openInArchive(null, new RandomAccessFileInStream(randomAccessFile));
 			archiveInterface = archive.getSimpleInterface();
@@ -139,7 +149,7 @@ public class SevenZipFileSystem implements GFileSystem {
 		return (entry != null) ? FSUtilities.infoMapToString(getInfoMap(entry)) : null;
 	}
 
-	public Map<String, String> getInfoMap(ISimpleInArchiveItem entry) {
+	private Map<String, String> getInfoMap(ISimpleInArchiveItem entry) {
 		Map<String, String> info = new LinkedHashMap<>();
 		try {
 			info.put("Name", entry.getPath());
@@ -151,7 +161,8 @@ public class SevenZipFileSystem implements GFileSystem {
 				compressedSize != null ? NumericUtilities.toHexString(compressedSize) : "NA");
 			info.put("Uncompressed Size", NumericUtilities.toHexString(getSize(entry)));
 			Integer crc = getCRC(entry);
-			info.put("CRC", crc != null ? NumericUtilities.toHexString(crc) : "NA");
+			info.put("CRC",
+				crc != null ? NumericUtilities.toHexString(crc.intValue() & 0xffffffffL) : "NA");
 			info.put("Compression Method", entry.getMethod());
 			Date creationTime = getCreateDate(entry);
 			info.put("Time", creationTime != null ? creationTime.toGMTString() : "NA");
@@ -249,16 +260,10 @@ public class SevenZipFileSystem implements GFileSystem {
 		}
 	}
 
-	private static long getSize(ISimpleInArchiveItem entry) {
+	private static long getSize(ISimpleInArchiveItem entry) throws SevenZipException {
-		try {
 		Long tempSize = entry.getSize();
 		return tempSize == null ? -1 : tempSize.intValue();
 	}
-		catch (SevenZipException e) {
-			//don't care
-		}
-		return -1;
-	}
 
 	private static Long getCompressedSize(ISimpleInArchiveItem entry) {
 		try {
@@ -318,8 +323,8 @@ public class SevenZipFileSystem implements GFileSystem {
 
 		private TaskMonitor monitor;
 		private int currentIndex;
-		private File currentTmpFile;
+		private File currentTempFile;
-		private OutputStream currentTmpOs;
+		private OutputStream currentTempFileOutputStream;
 
 		public SZExtractCallback(TaskMonitor monitor) {
 			this.monitor = monitor;
@@ -328,8 +333,10 @@ public class SevenZipFileSystem implements GFileSystem {
 		@Override
 		public ISequentialOutStream getStream(int index, ExtractAskMode extractAskMode)
 				throws SevenZipException {
+			// STEP 1: SevenZip calls this method to get a object it can use to write the bytes to.
+			// If we return null, SZ treats it as a skip.
 			try {
-				if (!fsService.hasDerivedFile(fsrl.getContainer(), Integer.toString(index),
+				if (!fileSystemService.hasDerivedFile(fsrl.getContainer(), Integer.toString(index),
 					monitor)) {
 					this.currentIndex = index;
 					return this;
@@ -343,10 +350,12 @@ public class SevenZipFileSystem implements GFileSystem {
 
 		@Override
 		public void prepareOperation(ExtractAskMode extractAskMode) throws SevenZipException {
+			// STEP 2: SevenZip calls this method to further prepare to operate on the file.
+			// In our case, we only handle extract operations.
 			if (extractAskMode == ExtractAskMode.EXTRACT) {
 				try {
-					currentTmpFile = File.createTempFile("ghidra_sevenzip_", ".tmp");
+					currentTempFile = File.createTempFile("ghidra_sevenzip_", ".tmp");
-					currentTmpOs = new FileOutputStream(currentTmpFile);
+					currentTempFileOutputStream = new FileOutputStream(currentTempFile);
 				}
 				catch (IOException e) {
 					throw new SevenZipException(e);
@@ -354,35 +363,12 @@ public class SevenZipFileSystem implements GFileSystem {
 			}
 		}
 
-		@Override
-		public void setOperationResult(ExtractOperationResult extractOperationResult)
-				throws SevenZipException {
-			if (currentTmpOs != null) {
-				try {
-					currentTmpOs.close();
-					extractOperationResultToException(extractOperationResult);
-					fsService.getDerivedFilePush(fsrl.getContainer(),
-						Integer.toString(currentIndex), (os) -> {
-							try (InputStream is = new FileInputStream(currentTmpFile)) {
-								FileUtilities.copyStreamToStream(is, os, monitor);
-							}
-						}, monitor);
-					currentTmpFile.delete();
-				}
-				catch (IOException | CancelledException e) {
-					throw new SevenZipException(e);
-				}
-				finally {
-					currentTmpFile = null;
-					currentTmpOs = null;
-				}
-			}
-		}
-
 		@Override
 		public int write(byte[] data) throws SevenZipException {
+			// STEP 3: SevenZip calls this multiple times for all the bytes in the file.
+			// We write them to our temp file.
 			try {
-				currentTmpOs.write(data);
+				currentTempFileOutputStream.write(data);
 				return data.length;
 			}
 			catch (IOException e) {
@@ -390,6 +376,32 @@ public class SevenZipFileSystem implements GFileSystem {
 			}
 		}
 
+		@Override
+		public void setOperationResult(ExtractOperationResult extractOperationResult)
+				throws SevenZipException {
+			// STEP 4: SevenZip calls this to signal that the extract is done for this file.
+			if (currentTempFileOutputStream != null) {
+				try {
+					currentTempFileOutputStream.close();
+					extractOperationResultToException(extractOperationResult);
+					fileSystemService.getDerivedFilePush(fsrl.getContainer(),
+						Integer.toString(currentIndex), (os) -> {
+							try (InputStream is = new FileInputStream(currentTempFile)) {
+								FileUtilities.copyStreamToStream(is, os, monitor);
+							}
+						}, monitor);
+					currentTempFile.delete();
+				}
+				catch (IOException | CancelledException e) {
+					throw new SevenZipException(e);
+				}
+				finally {
+					currentTempFile = null;
+					currentTempFileOutputStream = null;
+				}
+			}
+		}
+
 		//@formatter:off
 		@Override public void setTotal(long total) throws SevenZipException { /* nada */ }
 		@Override public void setCompleted(long complete) throws SevenZipException {/* nada */ }