Merge remote-tracking branch 'origin/GP-576_jpleasu_fix_bundles_concurrent_mod_bug--SQUASHED' into patch

2026-05-25 18:05:53 +08:00 · 2021-01-11 18:40:15 -05:00
parent 65e19b869e 72880eab41
commit d487bdc2e5
2 changed files with 245 additions and 53 deletions
@@ -22,7 +22,6 @@ import java.util.*;
 import java.util.Map.Entry;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.CountDownLatch;
-import java.util.function.Function;
 import java.util.stream.Collectors;

 import org.apache.felix.framework.FrameworkFactory;
@@ -63,8 +62,7 @@ public class BundleHost {
 	private static final String SAVE_STATE_TAG_ACTIVE = "BundleHost_ACTIVE";
 	private static final String SAVE_STATE_TAG_SYSTEM = "BundleHost_SYSTEM";

-	Map<ResourceFile, GhidraBundle> fileToBundleMap = new HashMap<>();
-	Map<String, GhidraBundle> bundleLocationToBundleMap = new HashMap<>();
+	private final BundleMap bundleMap = new BundleMap();

 	BundleContext frameworkBundleContext;
 	Framework felixFramework;
@@ -93,7 +91,7 @@ public class BundleHost {
 	 * @return false if the bundle was already enabled
 	 */
 	public boolean enable(ResourceFile bundleFile) {
-		GhidraBundle bundle = fileToBundleMap.get(bundleFile);
+		GhidraBundle bundle = bundleMap.get(bundleFile);
 		if (bundle == null) {
 			bundle = add(bundleFile, true, false);
 			return true;
@@ -139,7 +137,7 @@ public class BundleHost {
 	 * @return a {@link GhidraBundle} or {@code null}
 	 */
 	public GhidraBundle getExistingGhidraBundle(ResourceFile bundleFile) {
-		GhidraBundle bundle = fileToBundleMap.get(bundleFile);
+		GhidraBundle bundle = bundleMap.get(bundleFile);
 		if (bundle == null) {
 			Msg.showError(this, null, "ghidra bundle cache",
 				"getExistingGhidraBundle expected a GhidraBundle created at " + bundleFile +
@@ -156,7 +154,7 @@ public class BundleHost {
 	 * @return a {@link GhidraBundle} or {@code null}
 	 */
 	public GhidraBundle getGhidraBundle(ResourceFile bundleFile) {
-		return fileToBundleMap.get(bundleFile);
+		return bundleMap.get(bundleFile);
 	}

 	private static GhidraBundle createGhidraBundle(BundleHost bundleHost, ResourceFile bundleFile,
@@ -189,25 +187,11 @@ public class BundleHost {
 	 */
 	public GhidraBundle add(ResourceFile bundleFile, boolean enabled, boolean systemBundle) {
 		GhidraBundle bundle = createGhidraBundle(this, bundleFile, enabled, systemBundle);
-		fileToBundleMap.put(bundleFile, bundle);
-		bundleLocationToBundleMap.put(bundle.getLocationIdentifier(), bundle);
+		bundleMap.add(bundle);
 		fireBundleAdded(bundle);
 		return bundle;
 	}

-	private Set<ResourceFile> dedupeBundleFiles(List<ResourceFile> bundleFiles) {
-		Set<ResourceFile> dedupedBundleFiles = new HashSet<>(bundleFiles);
-		Iterator<ResourceFile> bundleFileIterator = dedupedBundleFiles.iterator();
-		while (bundleFileIterator.hasNext()) {
-			ResourceFile bundleFile = bundleFileIterator.next();
-			if (fileToBundleMap.containsKey(bundleFile)) {
-				bundleFileIterator.remove();
-				Msg.warn(this, "adding an already managed bundle: " + bundleFile.getAbsolutePath());
-			}
-		}
-		return dedupedBundleFiles;
-	}
-
 	/**
 	 * Create new GhidraBundles and add to the list of managed bundles.  All GhidraBundles created 
 	 * with the same {@code enabled} and {@code systemBundle} values. 
@@ -219,17 +203,8 @@ public class BundleHost {
 	 */
 	public Collection<GhidraBundle> add(List<ResourceFile> bundleFiles, boolean enabled,
 			boolean systemBundle) {
-		Set<ResourceFile> dedupedBundleFiles = dedupeBundleFiles(bundleFiles);
-		Map<ResourceFile, GhidraBundle> newBundleMap = dedupedBundleFiles.stream()
-				.collect(Collectors.toUnmodifiableMap(Function.identity(),
-					bundleFile -> createGhidraBundle(BundleHost.this, bundleFile, enabled,
-						systemBundle)));
-		fileToBundleMap.putAll(newBundleMap);
-		bundleLocationToBundleMap.putAll(newBundleMap.values()
-				.stream()
-				.collect(Collectors.toUnmodifiableMap(GhidraBundle::getLocationIdentifier,
-					Function.identity())));
-		Collection<GhidraBundle> newBundles = newBundleMap.values();
+		Collection<GhidraBundle> newBundles = bundleMap.computeAllIfAbsent(bundleFiles,
+			bundleFile -> createGhidraBundle(BundleHost.this, bundleFile, enabled, systemBundle));
 		fireBundlesAdded(newBundles);
 		return newBundles;
 	}
@@ -240,10 +215,7 @@ public class BundleHost {
 	 * @param bundles the bundles to add
 	 */
 	private void add(List<GhidraBundle> bundles) {
-		for (GhidraBundle bundle : bundles) {
-			fileToBundleMap.put(bundle.getFile(), bundle);
-			bundleLocationToBundleMap.put(bundle.getLocationIdentifier(), bundle);
-		}
+		bundleMap.addAll(bundles);
 		fireBundlesAdded(bundles);
 	}

@@ -253,8 +225,7 @@ public class BundleHost {
 	 * @param bundleFile the file of the bundle to remove
 	 */
 	public void remove(ResourceFile bundleFile) {
-		GhidraBundle bundle = fileToBundleMap.remove(bundleFile);
-		bundleLocationToBundleMap.remove(bundle.getLocationIdentifier());
+		GhidraBundle bundle = bundleMap.remove(bundleFile);
 		fireBundleRemoved(bundle);
 	}

@@ -264,8 +235,7 @@ public class BundleHost {
 	 * @param bundleLocation the location id of the bundle to remove
 	 */
 	public void remove(String bundleLocation) {
-		GhidraBundle bundle = bundleLocationToBundleMap.remove(bundleLocation);
-		fileToBundleMap.remove(bundle.getFile());
+		GhidraBundle bundle = bundleMap.remove(bundleLocation);
 		fireBundleRemoved(bundle);
 	}

@@ -275,8 +245,7 @@ public class BundleHost {
 	 * @param bundle the bundle to remove
 	 */
 	public void remove(GhidraBundle bundle) {
-		fileToBundleMap.remove(bundle.getFile());
-		bundleLocationToBundleMap.remove(bundle.getLocationIdentifier());
+		bundleMap.remove(bundle);
 		fireBundleRemoved(bundle);
 	}

@@ -286,10 +255,7 @@ public class BundleHost {
 	 * @param bundles the bundles to remove
 	 */
 	public void remove(Collection<GhidraBundle> bundles) {
-		for (GhidraBundle bundle : bundles) {
-			fileToBundleMap.remove(bundle.getFile());
-			bundleLocationToBundleMap.remove(bundle.getLocationIdentifier());
-		}
+		bundleMap.removeAll(bundles);
 		fireBundlesRemoved(bundles);
 	}

@@ -338,7 +304,7 @@ public class BundleHost {
 	 * @return all the bundles
 	 */
 	public Collection<GhidraBundle> getGhidraBundles() {
-		return fileToBundleMap.values();
+		return bundleMap.getGhidraBundles();
 	}

 	/**
@@ -347,7 +313,7 @@ public class BundleHost {
 	 * @return all the bundle files
 	 */
 	public Collection<ResourceFile> getBundleFiles() {
-		return fileToBundleMap.keySet();
+		return bundleMap.getBundleFiles();
 	}

 	void dumpLoadedBundles() {
@@ -861,7 +827,7 @@ public class BundleHost {
 			boolean isEnabled = bundleIsEnabled[i];
 			boolean isActive = bundleIsActive[i];
 			boolean isSystem = bundleIsSystem[i];
-			GhidraBundle bundle = fileToBundleMap.get(bundleFile);
+			GhidraBundle bundle = bundleMap.get(bundleFile);
 			if (bundle != null) {
 				if (isEnabled != bundle.isEnabled()) {
 					bundle.setEnabled(isEnabled);
@@ -900,14 +866,15 @@ public class BundleHost {
 	 * @param saveState the state object
 	 */
 	public void saveManagedBundleState(SaveState saveState) {
-		int numBundles = fileToBundleMap.size();
+		Collection<GhidraBundle> bundles = bundleMap.getGhidraBundles();
+		int numBundles = bundles.size();
 		String[] bundleFiles = new String[numBundles];
 		boolean[] bundleIsEnabled = new boolean[numBundles];
 		boolean[] bundleIsActive = new boolean[numBundles];
 		boolean[] bundleIsSystem = new boolean[numBundles];

 		int index = 0;
-		for (GhidraBundle bundle : fileToBundleMap.values()) {
+		for (GhidraBundle bundle : bundles) {
 			bundleFiles[index] = generic.util.Path.toPathString(bundle.getFile());
 			bundleIsEnabled[index] = bundle.isEnabled();
 			bundleIsActive[index] = bundle.isActive();
@@ -1081,7 +1048,7 @@ public class BundleHost {
 			GhidraBundle bundle;
 			switch (event.getType()) {
 				case BundleEvent.STARTED:
-					bundle = bundleLocationToBundleMap.get(osgiBundle.getLocation());
+					bundle = bundleMap.getBundleAtLocation(osgiBundle.getLocation());
 					if (bundle != null) {
 						fireBundleActivationChange(bundle, true);
 					}
@@ -1093,7 +1060,7 @@ public class BundleHost {
 					break;
 				// force "inactive" updates for all other states
 				default:
-					bundle = bundleLocationToBundleMap.get(osgiBundle.getLocation());
+					bundle = bundleMap.getBundleAtLocation(osgiBundle.getLocation());
 					if (bundle != null) {
 						fireBundleActivationChange(bundle, false);
 					}
@@ -0,0 +1,225 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed 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.
+ */
+package ghidra.app.plugin.core.osgi;
+
+import java.util.*;
+import java.util.concurrent.locks.*;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import generic.jar.ResourceFile;
+
+/**
+ * A thread-safe container that maps {@link GhidraBundle}s by file and bundle location.
+ */
+public class BundleMap {
+	private final ReadWriteLock lock = new ReentrantReadWriteLock();
+	private final Lock readLock = lock.readLock();
+	private final Lock writeLock = lock.writeLock();
+
+	private final Map<ResourceFile, GhidraBundle> bundlesByFile = new HashMap<>();
+	private final Map<String, GhidraBundle> bundlesByLocation = new HashMap<>();
+
+	/**
+	 * Maps associations between a bundle, its file, and its bundle location.
+	 * 
+	 * @param bundle a GhidraBundle object
+	 */
+	public void add(GhidraBundle bundle) {
+		writeLock.lock();
+		try {
+			bundlesByFile.put(bundle.getFile(), bundle);
+			bundlesByLocation.put(bundle.getLocationIdentifier(), bundle);
+		}
+		finally {
+			lock.writeLock().unlock();
+		}
+	}
+
+	/**
+	 * Maps bundles in a collection.
+	 * 
+	 * <p>This is the same as calling {@link BundleMap#add(GhidraBundle)} for each bundle in {@code bundles}.
+	 * 
+	 * @param bundles a collection of GhidraBundle objects
+	 */
+	public void addAll(Collection<GhidraBundle> bundles) {
+		writeLock.lock();
+		try {
+			for (GhidraBundle bundle : bundles) {
+				bundlesByFile.put(bundle.getFile(), bundle);
+				bundlesByLocation.put(bundle.getLocationIdentifier(), bundle);
+			}
+		}
+		finally {
+			writeLock.unlock();
+		}
+	}
+
+	/**
+	 * Removes the mappings of a bundle. 
+	 * 
+	 * @param bundle a GhidraBundle object
+	 */
+	public void remove(GhidraBundle bundle) {
+		writeLock.lock();
+		try {
+			bundlesByFile.remove(bundle.getFile());
+			bundlesByLocation.remove(bundle.getLocationIdentifier());
+		}
+		finally {
+			writeLock.unlock();
+		}
+	}
+
+	/**
+	 * Removes all mappings of each bundle from a collection.
+	 * 
+	 * This is the same as calling {@link #remove(GhidraBundle)} for each bundle in {@code bundles}.
+	 * 
+	 * @param bundles a collection of GhidraBundle objects
+	 */
+	public void removeAll(Collection<GhidraBundle> bundles) {
+		writeLock.lock();
+		try {
+			for (GhidraBundle bundle : bundles) {
+				bundlesByFile.remove(bundle.getFile());
+				bundlesByLocation.remove(bundle.getLocationIdentifier());
+			}
+		}
+		finally {
+			writeLock.unlock();
+		}
+	}
+
+	/**
+	 * Removes the mapping for a bundle with a given bundle location.
+	 * 
+	 * @param bundleLocation a bundle location
+	 * @return the bundle removed
+	 */
+	public GhidraBundle remove(String bundleLocation) {
+		writeLock.lock();
+		try {
+			GhidraBundle bundle = bundlesByLocation.remove(bundleLocation);
+			bundlesByFile.remove(bundle.getFile());
+			return bundle;
+		}
+		finally {
+			writeLock.unlock();
+		}
+	}
+
+	/**
+	 * Removes the mapping for a bundle with a given file.
+	
+	 * @param bundleFile a bundle file
+	 * @return the bundle removed
+	 */
+	public GhidraBundle remove(ResourceFile bundleFile) {
+		writeLock.lock();
+		try {
+			GhidraBundle bundle = bundlesByFile.remove(bundleFile);
+			bundlesByLocation.remove(bundle.getLocationIdentifier());
+			return bundle;
+		}
+		finally {
+			writeLock.unlock();
+		}
+	}
+
+	/**
+	 * Creates and maps bundles from files in a collection that aren't already mapped.
+	 * 
+	 * @param bundleFiles a collection of bundle files
+	 * @param ctor a constructor for a GhidraBundle given a bundle file
+	 * @return the newly created GhidraBundle objects
+	 */
+	public Collection<GhidraBundle> computeAllIfAbsent(Collection<ResourceFile> bundleFiles,
+			Function<ResourceFile, GhidraBundle> ctor) {
+		writeLock.lock();
+		try {
+			Set<ResourceFile> newBundleFiles = new HashSet<>(bundleFiles);
+			newBundleFiles.removeAll(bundlesByFile.keySet());
+			List<GhidraBundle> newBundles =
+				newBundleFiles.stream().map(ctor).collect(Collectors.toList());
+			addAll(newBundles);
+			return newBundles;
+		}
+		finally {
+			writeLock.unlock();
+		}
+	}
+
+	/**
+	 * Returns the bundle with the given location.
+	 * 
+	 * @param location a bundle location
+	 * @return the bundle found or null
+	 */
+	public GhidraBundle getBundleAtLocation(String location) {
+		readLock.lock();
+		try {
+			return bundlesByLocation.get(location);
+		}
+		finally {
+			readLock.unlock();
+		}
+	}
+
+	/**
+	 * Returns the bundle with the given file.
+	 * 
+	 * @param bundleFile a bundle file
+	 * @return the bundle found or null
+	 */
+	public GhidraBundle get(ResourceFile bundleFile) {
+		readLock.lock();
+		try {
+			return bundlesByFile.get(bundleFile);
+		}
+		finally {
+			readLock.unlock();
+		}
+	}
+
+	/**
+	 * @return the currently mapped bundles
+	 */
+	public Collection<GhidraBundle> getGhidraBundles() {
+		readLock.lock();
+		try {
+			return new ArrayList<>(bundlesByFile.values());
+		}
+		finally {
+			readLock.unlock();
+		}
+	}
+
+	/**
+	 * @return the currently mapped bundle files
+	 */
+	public Collection<ResourceFile> getBundleFiles() {
+		readLock.lock();
+		try {
+			return new ArrayList<>(bundlesByFile.keySet());
+		}
+		finally {
+			readLock.unlock();
+		}
+	}
+
+}