GT-3405 - Fixed NPE when deleting bookmarks

Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/bookmark/BookmarkPluginTest.java

@@ -624,6 +624,21 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest {
 		assertEquals(addr("1001010"), bookmark.getAddress());
 	}
 
+	@Test
+	public void testRemoveBookmarksByType() {
+
+		List<Bookmark> list = getBookmarks(program.getBookmarkManager());
+		assertFalse(list.isEmpty());
+
+		BookmarkManager bm = program.getBookmarkManager();
+		tx(program, () -> {
+			bm.removeBookmarks(BookmarkType.ALL_TYPES);
+		});
+
+		list = getBookmarks(program.getBookmarkManager());
+		assertTrue(list.isEmpty());
+	}
+
 //==================================================================================================
 // Private Methods
 //==================================================================================================
@@ -636,7 +651,6 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest {
 	}
 
 	private void loadDefaultTool() throws Exception {
-//		env = new TestEnv();
 		program = buildProgram();
 
 		// UNUSUAL CODE ALERT!: 

Ghidra/Framework/FileSystem/src/main/java/ghidra/util/NamingUtilities.java

@@ -116,7 +116,7 @@ public final class NamingUtilities {
 	 * @see #isValidName(String)
 	 * @deprecated this method may be removed in a subsequent release due to 
 	 * limited use and applicability (project names and project file names have
-	 * different naming resrictions).
+	 * different naming restrictions).
 	 */
 	@Deprecated
 	public static char findInvalidChar(String name) {
@@ -169,7 +169,7 @@ public final class NamingUtilities {
 	 * chars in a row are replace by a single MANGLE_CHAR.
 	 * 
 	 * @param mangledName mangled name string
-	 * @return demagle name
+	 * @return demangle name
 	 */
 	public static String demangle(String mangledName) {
 		int len = mangledName.length();

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/bookmark/BookmarkDBManager.java

@@ -21,6 +21,8 @@ import java.util.*;
 
 import javax.swing.ImageIcon;
 
+import org.apache.commons.lang3.StringUtils;
+
 import db.*;
 import db.util.ErrorHandler;
 import generic.util.*;
@@ -32,11 +34,10 @@ import ghidra.program.database.util.EmptyRecordIterator;
 import ghidra.program.model.address.*;
 import ghidra.program.model.listing.*;
 import ghidra.program.util.ChangeManager;
-import ghidra.util.*;
+import ghidra.util.Lock;
 import ghidra.util.datastruct.ObjectArray;
 import ghidra.util.exception.*;
 import ghidra.util.task.TaskMonitor;
-import ghidra.util.task.TaskMonitorAdapter;
 
 public class BookmarkDBManager implements BookmarkManager, ErrorHandler, ManagerDB {
 
@@ -60,7 +61,7 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 	 * @param openMode either READ_ONLY, UPDATE, or UPGRADE
 	 * @param lock the program synchronization lock
 	 * @param monitor the task monitor use while upgrading.
-	 * @throws VersionException if the database is incompatable with the current
+	 * @throws VersionException if the database is incompatible with the current
 	 * schema
 	 * @throws IOException if there is a problem accessing the database.
 	 */
@@ -75,11 +76,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		cache = new DBObjectCache<BookmarkDB>(100);
 	}
 
-	/**
-	 * Set associated program.  This must be invoked once prior to invoking any
-	 * other method.
-	 * @param program
-	 */
 	@Override
 	public void setProgram(ProgramDB program) {
 		if (this.program != null) {
@@ -97,19 +93,17 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 				OldBookmarkManager oldMgr = new OldBookmarkManager(program);
 				((BookmarkTypeDBAdapterNoTable) bookmarkTypeAdapter).setOldBookmarkManager(oldMgr);
 				((BookmarkDBAdapterV0) bookmarkAdapter).setOldBookmarkManager(oldMgr, addrMap,
-					TaskMonitorAdapter.DUMMY_MONITOR);
+					TaskMonitor.DUMMY);
 			}
 
 			Record[] typeRecords = bookmarkTypeAdapter.getRecords();
-			for (int i = 0; i < typeRecords.length; i++) {
+			for (Record rec : typeRecords) {
-				Record rec = typeRecords[i];
 				int typeId = (int) rec.getKey();
 				BookmarkTypeDB type =
 					new BookmarkTypeDB(typeId, rec.getString(BookmarkTypeDBAdapter.TYPE_NAME_COL));
 				type.setHasBookmarks(true);
 				typesByName.put(type.getTypeString(), type);
 				typesArray.put(typeId, type);
-//				bookmarkAdapter.addType((int)typeId);
 			}
 		}
 		catch (IOException e) {
@@ -179,10 +173,8 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		}
 	}
 
-	/**
+	/*
 	 * Upgrade old property-based bookmarks to the new storage schema.
-	 * @param programDB
-	 * @param monitor
 	 */
 	private void upgradeOldBookmarks(ProgramDB programDB) {
 
@@ -191,8 +183,8 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		if (oldTypes.length == 0) {
 			return;
 		}
-		for (int i = 0; i < oldTypes.length; i++) {
+		for (Record oldType : oldTypes) {
-			String type = oldTypes[i].getString(BookmarkTypeDBAdapter.TYPE_NAME_COL);
+			String type = oldType.getString(BookmarkTypeDBAdapter.TYPE_NAME_COL);
 			AddressIterator iter = oldMgr.getBookmarkAddresses(type);
 			while (iter.hasNext()) {
 				OldBookmark bm = oldMgr.getBookmark(iter.next(), type);
@@ -202,12 +194,8 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		}
 	}
 
-	/**
+	/*
-	 * Get or create bookmark type.
+	 * Get or create bookmark type
-	 * @param type bookmark type
-	 * @param create if true and type does exist it will be created
-	 * @return bookmark type or null if it does not exist and was not created.
-	 * @throws IOException
 	 */
 	private BookmarkTypeDB getBookmarkType(String type, boolean create) throws IOException {
 		BookmarkTypeDB bmt = (BookmarkTypeDB) typesByName.get(type);
@@ -239,37 +227,26 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		return n;
 	}
 
-	/**
+	/*
-	 * Get existing bookmark type.
+	 * Get existing bookmark type
-	 * @param typeId bookmark type ID
-	 * @return bookmark type or null if it does not exist
 	 */
 	BookmarkTypeDB getBookmarkType(int typeID) {
 		return (BookmarkTypeDB) typesArray.get(typeID);
 	}
 
-	/**
-	 * Define a bookmark type with its marker icon and color.  The icon and color
-	 * values are not permanently stored.  Therefor, this method must be re-invoked
-	 * by a plugin each time a program is opened if a custom icon and color 
-	 * are desired.
-	 * @param type bookmark type
-	 * @param icon marker icon which may get scaled
-	 * @param color marker color
-	 * @return bookmark type object
-	 */
 	@Override
 	public BookmarkType defineType(String type, ImageIcon icon, Color color, int priority) {
 		lock.acquire();
 		try {
-			if (type == null || type.length() == 0 || !NamingUtilities.isValidName(type) ||
+			String validatedType = StringUtils.trim(type);
-				icon == null || color == null) {
+			if (StringUtils.isBlank(validatedType) || icon == null || color == null) {
 				throw new IllegalArgumentException(
 					"Invalid bookmark type parameters were specified");
 			}
+
 			BookmarkTypeDB bmt = null;
 			try {
-				bmt = getBookmarkType(type, false);
+				bmt = getBookmarkType(validatedType, false);
 				bmt.setIcon(icon);
 				bmt.setMarkerColor(color);
 				bmt.setMarkerPriority(priority);
@@ -284,9 +261,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		}
 	}
 
-	/**
-	 * Returns list of known bookmark types.
-	 */
 	@Override
 	public BookmarkType[] getBookmarkTypes() {
 		lock.acquire();
@@ -306,23 +280,11 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		return program;
 	}
 
-	/**
-	 * Get a bookmark type
-	 * @param type bookmark type name
-	 * @return bookmark type or null if type is unknown
-	 */
 	@Override
 	public BookmarkType getBookmarkType(String type) {
 		return typesByName.get(type);
 	}
 
-	/**
-	 * Set a bookmark.
-	 * @param addr
-	 * @param type
-	 * @param category
-	 * @param comment
-	 */
 	@Override
 	public Bookmark setBookmark(Address addr, String type, String category, String comment) {
 		lock.acquire();
@@ -354,11 +316,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		return null;
 	}
 
-	/**
-	 * Get the bookmark object corresponding to the specified bookmark record.
-	 * @param bookmarkRecord
-	 * @return bookmark object
-	 */
 	private BookmarkDB getBookmark(Record bookmarkRecord) {
 		BookmarkDB bm = cache.get(bookmarkRecord);
 		if (bm == null) {
@@ -367,13 +324,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		return bm;
 	}
 
-	/**
-	 * Get a specific bookmark
-	 * @param addr
-	 * @param type
-	 * @param category
-	 * @return
-	 */
 	@Override
 	public Bookmark getBookmark(Address addr, String type, String category) {
 		lock.acquire();
@@ -401,10 +351,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		return null;
 	}
 
-	/**
-	 * Remove bookmark
-	 * @param bookmark
-	 */
 	@Override
 	public void removeBookmark(Bookmark bookmark) {
 		lock.acquire();
@@ -438,39 +384,34 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 
 	}
 
-	/**
-	 * Remove bookmark(s)
-	 * @param type bookmark type or null for all bookmarks
-	 */
 	@Override
 	public void removeBookmarks(String type) {
 		lock.acquire();
 		try {
-			// Handle case where type/category are specified
-			if (type != null) {
-				try {
-					BookmarkTypeDB bmt = (BookmarkTypeDB) typesByName.get(type);
-					if (bmt.hasBookmarks()) {
-						int typeId = bmt.getTypeId();
-						bookmarkAdapter.deleteType(typeId);
-						bookmarkTypeAdapter.deleteRecord(typeId);
-						bmt.setHasBookmarks(false);
-						// fire event
-						program.setObjChanged(ChangeManager.DOCR_BOOKMARK_TYPE_REMOVED, bmt, null,
-							null);
-					}
-				}
-				catch (IOException e) {
-					dbError(e);
-				}
-			}
 
-			// Remove all bookmarks if type is null
+			boolean isSpecificType = type != null && type != BookmarkType.ALL_TYPES;
-			else {
+			if (!isSpecificType) {
+				// no type specified; remove all
 				Iterator<String> iter = typesByName.keySet().iterator();
 				while (iter.hasNext()) {
 					removeBookmarks(iter.next());
 				}
+				return;
+			}
+
+			try {
+				BookmarkTypeDB bmt = (BookmarkTypeDB) typesByName.get(type);
+				if (bmt.hasBookmarks()) {
+					int typeId = bmt.getTypeId();
+					bookmarkAdapter.deleteType(typeId);
+					bookmarkTypeAdapter.deleteRecord(typeId);
+					bmt.setHasBookmarks(false);
+					program.setObjChanged(ChangeManager.DOCR_BOOKMARK_TYPE_REMOVED, bmt, null,
+						null);
+				}
+			}
+			catch (IOException e) {
+				dbError(e);
 			}
 		}
 		finally {
@@ -567,9 +508,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		}
 	}
 
-	/*
-	 * @see ghidra.program.model.listing.BookmarkManager#getBookmarks(ghidra.program.model.address.Address, java.lang.String)
-	 */
 	@Override
 	public Bookmark[] getBookmarks(Address address, String type) {
 		lock.acquire();
@@ -589,9 +527,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		}
 	}
 
-	/*
-	 * @see ghidra.program.model.listing.BookmarkManager#hasBookmarks()
-	 */
 	@Override
 	public boolean hasBookmarks(String type) {
 		lock.acquire();
@@ -607,9 +542,6 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 		}
 	}
 
-	/*
-	 * @see ghidra.program.model.listing.BookmarkManager#getCategories(java.lang.String)
-	 */
 	@Override
 	public String[] getCategories(String type) {
 		lock.acquire();
@@ -868,8 +800,9 @@ public class BookmarkDBManager implements BookmarkManager, ErrorHandler, Manager
 				int typeId = bt.getTypeId();
 				if (bt.hasBookmarks()) {
 					Table table = bookmarkAdapter.getTable(typeId);
-					if (table == null)
+					if (table == null) {
 						continue;
+					}
 					int addrCol = BookmarkDBAdapter.ADDRESS_COL;
 					try {
 						DatabaseTableUtils.updateIndexedAddressField(table, addrCol, addrMap,

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/BookmarkManager.java

@@ -15,17 +15,16 @@
  */
 package ghidra.program.model.listing;
 
+import java.awt.Color;
+import java.util.Iterator;
+
+import javax.swing.ImageIcon;
+
 import ghidra.program.model.address.Address;
 import ghidra.program.model.address.AddressSetView;
 import ghidra.util.exception.CancelledException;
 import ghidra.util.task.TaskMonitor;
 
-import java.awt.Color;
-import java.io.IOException;
-import java.util.Iterator;
-
-import javax.swing.ImageIcon;
-
 /**
  * Interface for managing bookmarks.
  */
@@ -61,12 +60,15 @@ public interface BookmarkManager {
 	 * @param type bookmark type
 	 * @param icon marker icon which may get scaled
 	 * @param color marker color
+	 * @param priority the bookmark priority
 	 * @return bookmark type object
+	 * @throws IllegalArgumentException if any of the arguments are null or if the type is empty 
 	 */
 	BookmarkType defineType(String type, ImageIcon icon, Color color, int priority);
 
 	/**
-	 * Returns list of known bookmark types.
+	 * Returns list of known bookmark types
+	 * @return list of known bookmark types
 	 */
 	BookmarkType[] getBookmarkTypes();
 
@@ -90,6 +92,7 @@ public interface BookmarkManager {
 	 * @param type the name of the bookmark type.
 	 * @param category the category for the bookmark.
 	 * @param comment the comment to associate with the bookmark.
+	 * @return the new bookmark
 	 */
 	Bookmark setBookmark(Address addr, String type, String category, String comment);
 
@@ -127,7 +130,7 @@ public interface BookmarkManager {
 	/**
 	 * Removes all bookmarks over the given address set.
 	 * @param set the set of addresses from which to remove all bookmarks.
-	 * @param monitor a taskmonitor to report the progress.
+	 * @param monitor a task monitor to report the progress.
 	 * @throws CancelledException if the user (via the monitor) cancelled the operation.
 	 */
 	void removeBookmarks(AddressSetView set, TaskMonitor monitor) throws CancelledException;
@@ -136,7 +139,7 @@ public interface BookmarkManager {
 	 * Removes all bookmarks of the given type over the given address set
 	 * @param set the set of addresses from which to remove all bookmarks of the given type.
 	 * @param type the type of bookmarks to remove.
-	 * @param monitor a taskmonitor to report the progress.
+	 * @param monitor a task monitor to report the progress.
 	 * @throws CancelledException if the user (via the monitor) cancelled the operation.
 	 */
 	void removeBookmarks(AddressSetView set, String type, TaskMonitor monitor)
@@ -147,7 +150,7 @@ public interface BookmarkManager {
 	 * @param set the set of addresses from which to remove all bookmarks of the given type and category.
 	 * @param type the type of bookmarks to remove.
 	 * @param category the category of bookmarks to remove.
-	 * @param monitor a taskmonitor to report the progress.
+	 * @param monitor a task monitor to report the progress.
 	 * @throws CancelledException if the user (via the monitor) cancelled the operation.
 	 */
 	void removeBookmarks(AddressSetView set, String type, String category, TaskMonitor monitor)
@@ -183,7 +186,8 @@ public interface BookmarkManager {
 	Iterator<Bookmark> getBookmarksIterator(String type);
 
 	/**
-	 * Returns an iterator over all bookmarks.
+	 * Returns an iterator over all bookmarks
+	 * @return an iterator over all bookmarks
 	 */
 	Iterator<Bookmark> getBookmarksIterator();
 
@@ -202,23 +206,27 @@ public interface BookmarkManager {
 	/**
 	 * Returns the bookmark that has the given id or null if no such bookmark exists.
 	 * @param id the id of the bookmark to be retrieved.
+	 * @return the bookmark
 	 */
 	Bookmark getBookmark(long id);
 
 	/**
-	 * Returns true if program contains one or more bookmarks of the given type.
+	 * Returns true if program contains one or more bookmarks of the given type
 	 * @param type the type of bookmark to check for.
+	 * @return true if program contains one or more bookmarks of the given type
 	 */
 	boolean hasBookmarks(String type);
 
 	/**
-	 * Return the number of bookmarks of the given type.
+	 * Return the number of bookmarks of the given type
-	 * @param type the type of bookmarks to count.
+	 * @param type the type of bookmarks to count
+	 * @return the number of bookmarks of the given type
 	 */
 	int getBookmarkCount(String type);
 
 	/**
-	 * Returns the total number of bookmarks in the program.
+	 * Returns the total number of bookmarks in the program
+	 * @return the total number of bookmarks in the program
 	 */
 	int getBookmarkCount();