More thread safety precautions in IO handling for "last-file-trusted" importing

Author: Col-E

recaf-core/src/main/java/software/coley/recaf/info/properties/builtin/ZipEntryIndexProperty.java

@@ -0,0 +1,79 @@
+package software.coley.recaf.info.properties.builtin;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import software.coley.recaf.info.Info;
+import software.coley.recaf.info.properties.BasicProperty;
+import software.coley.recaf.info.properties.Property;
+
+/**
+ * Built in property to track the original index of the file as it appears in the zip structure.
+ *
+ * @author Matt Coley
+ */
+public class ZipEntryIndexProperty extends BasicProperty<Integer> {
+	public static final String KEY = "zip-entry-index";
+
+	/**
+	 * @param value
+	 * 		The index of the local file in its containing zip archive.
+	 */
+	public ZipEntryIndexProperty(int value) {
+		super(KEY, value);
+	}
+
+
+	@Override
+	public boolean persistent() {
+		return false;
+	}
+
+	/**
+	 * @param info
+	 * 		Info instance.
+	 *
+	 * @return Zip entry index.
+	 * {@code null} when no property value is assigned.
+	 */
+	@Nullable
+	public static Integer get(@Nonnull Info info) {
+		Property<Integer> property = info.getProperty(KEY);
+		if (property != null) {
+			return property.value();
+		}
+		return null;
+	}
+
+	/**
+	 * @param info
+	 * 		Info instance.
+	 * @param fallback
+	 * 		Fallback index if there is no recorded index for the info instance.
+	 *
+	 * @return Zip entry index.
+	 * {@code null} when no property value is assigned.
+	 */
+	public static int getOr(@Nonnull Info info, int fallback) {
+		Integer value = get(info);
+		if (value == null) return fallback;
+		return value;
+	}
+
+	/**
+	 * @param info
+	 * 		Info instance.
+	 * @param value
+	 * 		Zip entry index.
+	 */
+	public static void set(@Nonnull Info info, int value) {
+		info.setProperty(new ZipEntryIndexProperty(value));
+	}
+
+	/**
+	 * @param info
+	 * 		Info instance.
+	 */
+	public static void remove(@Nonnull Info info) {
+		info.removeProperty(KEY);
+	}
+}

recaf-core/src/main/java/software/coley/recaf/services/workspace/io/BasicResourceImporter.java

@@ -30,6 +30,7 @@
 import software.coley.recaf.info.properties.builtin.ZipCommentProperty;
 import software.coley.recaf.info.properties.builtin.ZipCompressionProperty;
 import software.coley.recaf.info.properties.builtin.ZipCreationTimeProperty;
+import software.coley.recaf.info.properties.builtin.ZipEntryIndexProperty;
 import software.coley.recaf.info.properties.builtin.ZipMarkerProperty;
 import software.coley.recaf.info.properties.builtin.ZipModificationTimeProperty;
 import software.coley.recaf.info.properties.builtin.ZipPrefixDataProperty;
@@ -214,7 +215,10 @@ private WorkspaceFileResource handleZip(@Nonnull WorkspaceFileResourceBuilder bu
 				ThreadPoolFactory.newFixedThreadPool("zip-import") :
 				ThreadPoolFactory.newSingleThreadExecutor("zip-import")) {
 			List<Callable<Void>> tasks = new ArrayList<>();
-			for (LocalFileHeader header : archive.getLocalFiles()) {
+			List<LocalFileHeader> localFiles = archive.getLocalFiles();
+			for (int i = 0; i < localFiles.size(); i++) {
+				int entryIndex = i;
+				LocalFileHeader header = localFiles.get(i);
 				tasks.add(() -> {
 					LocalFileHeaderSource headerSource = new LocalFileHeaderSource(header, isAndroid);
 					String entryName = header.getFileNameAsString();
@@ -229,6 +233,7 @@ private WorkspaceFileResource handleZip(@Nonnull WorkspaceFileResourceBuilder bu
 					Info info;
 					try {
 						info = infoImporter.readInfo(entryName, headerSource);
+						ZipEntryIndexProperty.set(info, entryIndex);
 					} catch (IOException ex) {
 						logger.error("IO error reading ZIP entry '{}' - skipping", entryName, ex);
 						return null;
@@ -339,7 +344,6 @@ public FileVisitResult visitFile(@Nonnull Path file, @Nonnull BasicFileAttribute
 				.build();
 	}
 
-	@SuppressWarnings("all") // Ignore synchronizing on parameter
 	private void addInfo(@Nonnull BasicJvmClassBundle classes,
 	                     @Nonnull BasicFileBundle files,
 	                     @Nonnull Map<String, AndroidClassBundle> androidClassBundles,
@@ -348,8 +352,6 @@ private void addInfo(@Nonnull BasicJvmClassBundle classes,
 	                     @Nonnull ByteSource infoSource,
 	                     @Nonnull String pathName,
 	                     @Nonnull Info info) {
-		// TODO: The synchronization here should be more narrow to when each respective bundle/map is interacted with.
-		//  These are all concurrent maps (or sync
 		if (info.isClass()) {
 			addClassInfo(classes, files, versionedJvmClassBundles, pathName, info);
 		} else if (info.isFile()) {
@@ -527,12 +529,19 @@ private void addFileInfo(@Nonnull BasicFileBundle files,
 		// Warn if there are duplicate file entries.
 		// Same cases for why this may occur are described above when handling classes.
 		// The JVM will always use the last item for duplicate entries anyways.
-		if (files.containsKey(pathName)) {
-			logger.warn("Multiple duplicate entries for file '{}', dropping older entry", pathName);
-		}
+		synchronized (files) {
+			FileInfo existingFile = files.get(pathName);
+			if (existingFile != null) {
+				int existingIndex = ZipEntryIndexProperty.getOr(existingFile, -1);
+				int newIndex = ZipEntryIndexProperty.getOr(fileInfo, -1);
+				if (newIndex < existingIndex)
+					return;
+				logger.warn("Multiple duplicate entries for file '{}', dropping older entry", pathName);
+			}
 
-		// Store in bundle.
-		files.initialPut(fileInfo);
+			// Store in bundle.
+			files.initialPut(fileInfo);
+		}
 	}
 
 	/**

recaf-core/src/test/java/software/coley/recaf/services/workspace/io/ResourceImporterTest.java

@@ -1,6 +1,7 @@
 package software.coley.recaf.services.workspace.io;
 
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
 import software.coley.recaf.info.BasicNativeLibraryFileInfo;
 import software.coley.recaf.info.FileInfo;
@@ -230,7 +231,11 @@ void testAlwaysUseLastMultiReleaseClassEntry() throws IOException {
 		assertArrayEquals(emptyBytes, resource.getFileBundle().iterator().next().getRawContent());
 	}
 
-	@Test
+	/**
+	 * This test is repeated to verify it wasn't successful on a race condition <i>(since our IO is multithreaded by default)</i>.
+	 * I know, this isn't an exhaustive/perfect test, but it is better than a single test run.
+	 */
+	@RepeatedTest(100)
 	void testAlwaysUseLastFileEntry() throws IOException {
 		String path = HelloWorld.class.getName().replace(".", "/");
 		byte[] bytes = new byte[]{1, 2, 3};
@@ -255,7 +260,7 @@ void testAlwaysUseLastFileEntry() throws IOException {
 		assertArrayEquals(bytes, resource.getFileBundle().iterator().next().getRawContent());
 	}
 
-	@Test
+	@RepeatedTest(100)
 	void testDeduplicateClasses() throws IOException {
 		String helloWorldPath = HelloWorld.class.getName().replace(".", "/");
 		byte[] helloWorldBytes = TestClassUtils.fromRuntimeClass(HelloWorld.class).getBytecode();