GP-812 corrected improper exposure of DatabaseObject methods

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/DatabaseObject.java

@@ -26,7 +26,7 @@ import ghidra.util.Lock;
  * been deleted. Instantiating an object will cause it to be added immediately to the associated
  * cache.
  */
-abstract public class DatabaseObject {
+public abstract class DatabaseObject {
 
 	protected long key;
 	private volatile boolean deleted;
@@ -41,7 +41,7 @@ abstract public class DatabaseObject {
 	 * @param key database key to uniquely identify this object
 	 */
 	@SuppressWarnings("unchecked")
-	public DatabaseObject(@SuppressWarnings("rawtypes") DBObjectCache cache, long key) {
+	protected DatabaseObject(@SuppressWarnings("rawtypes") DBObjectCache cache, long key) {
 		this.key = key;
 		this.cache = cache;
 		if (cache != null) {
@@ -64,17 +64,6 @@ abstract public class DatabaseObject {
 		deleted = true;
 	}
 
-	/**
-	 * Returns true if this object has been deleted. Note: once an object has been deleted, it will
-	 * never be "refreshed". For example, if an object is ever deleted and is resurrected via an
-	 * "undo", you will have get a fresh instance of the object.
-	 * 
-	 * @return true if this object has been deleted.
-	 */
-	public boolean isDeleted() {
-		return deleted;
-	}
-
 	/**
 	 *
 	 * Invalidate this object. This does not necessarily mean that this object can never be used
@@ -101,21 +90,28 @@ abstract public class DatabaseObject {
 	}
 
 	/**
-	 * Returns true if object is currently invalid. Calling checkIsValid may successfully refresh
-	 * object making it valid.
+	 * Returns true if object is currently invalid and must be validated prior to further use. 
+	 * An invalid object may result from a cache invalidation which corresponds to wide-spread 
+	 * record changes.  A common situation where this can occur is an undo/redo operation
+	 * against the underlying database.  The methods {@link #checkIsValid()}, {@link #checkDeleted()},
+	 * {@link #validate(Lock)} and {@link #isDeleted(Lock)} are methods which will force
+	 * a re-validation if required.
 	 * 
-	 * @see #checkIsValid()
+	 * @return true if this object is invalid and must be re-validated, else false if object state
+	 * is currently valid which may include a deleted state.
 	 */
-	public boolean isInvalid() {
-		return invalidateCount != getCurrentValidationCount();
+	protected boolean isInvalid() {
+		return !deleted && invalidateCount != getCurrentValidationCount();
 	}
 
 	/**
 	 * Checks if this object has been deleted, in which case any use of the object is not allowed.
+	 * This method should be invoked before any modifications to the object are performed to 
+	 * ensure it still exists and is in a valid state.
 	 * 
 	 * @throws ConcurrentModificationException if the object has been deleted from the database.
 	 */
-	public void checkDeleted() {
+	protected void checkDeleted() {
 		if (!checkIsValid()) {
 			throw new ConcurrentModificationException("Object has been deleted.");
 		}
@@ -125,9 +121,9 @@ abstract public class DatabaseObject {
 	 * Check whether this object is still valid. If the object is invalid, the object will attempt
 	 * to refresh itself. If the refresh fails, the object will be marked as deleted.
 	 * 
-	 * @return true if the object is valid.
+	 * @return true if the object is valid, else false if deleted
 	 */
-	public boolean checkIsValid() {
+	protected boolean checkIsValid() {
 		return checkIsValid(null);
 	}
 
@@ -140,10 +136,7 @@ abstract public class DatabaseObject {
 	 * @param record optional record which may be used to refresh invalid object
 	 * @return true if the object is valid.
 	 */
-	public boolean checkIsValid(DBRecord record) {
-		if (deleted) {
-			return false;
-		}
+	protected boolean checkIsValid(DBRecord record) {
 		if (isInvalid()) {
 			setValid();// prevent checkIsValid recursion during refresh
 			if (!refresh(record)) {
@@ -162,10 +155,10 @@ abstract public class DatabaseObject {
 	 * invalid, then the lock will be used to refresh as needed.
 	 * 
 	 * @param lock the lock that will be used if the object needs to be refreshed.
-	 * @return true if object is valid, else false
+	 * @return true if object is valid, else false if deleted
 	 */
-	public boolean validate(Lock lock) {
-		if (!deleted && !isInvalid()) {
+	protected boolean validate(Lock lock) {
+		if (!isInvalid()) {
 			return true;
 		}
 		lock.acquire();
@@ -177,6 +170,18 @@ abstract public class DatabaseObject {
 		}
 	}
 
+	/**
+	 * Returns true if this object has been deleted. Note: once an object has been deleted, it will
+	 * never be "refreshed". For example, if an object is ever deleted and is resurrected via an
+	 * "undo", you will have get a fresh instance of the object.
+	 * 
+	 * @param lock object cache lock object
+	 * @return true if this object has been deleted.
+	 */
+	public boolean isDeleted(Lock lock) {
+		return deleted || !validate(lock);
+	}
+
 	/**
 	 * Tells the object to refresh its state from the database.
 	 * 

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CategoryDB.java

@@ -19,8 +19,8 @@ import java.io.IOException;
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 
-import db.Field;
 import db.DBRecord;
+import db.Field;
 import ghidra.program.database.DBObjectCache;
 import ghidra.program.database.DatabaseObject;
 import ghidra.program.model.data.*;
@@ -92,8 +92,7 @@ class CategoryDB extends DatabaseObject implements Category {
 		}
 		mgr.lock.acquire();
 		try {
-			checkIsValid();
-			if (isRoot() || isDeleted()) {
+			if (!checkIsValid() || isRoot()) {
 				categoryPath = CategoryPath.ROOT;
 			}
 			if (categoryPath == null) {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeDB.java

@@ -216,7 +216,7 @@ abstract class DataTypeDB extends DatabaseObject implements DataType, ChangeList
 	 */
 	@Override
 	public boolean isDeleted() {
-		return !checkIsValid();
+		return isDeleted(lock);
 	}
 
 	/**

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionDB.java

@@ -95,6 +95,11 @@ public class FunctionDB extends DatabaseObject implements Function {
 		frame = new FunctionStackFrame(this);
 	}
 
+	@Override
+	public boolean isDeleted() {
+		return isDeleted(manager.lock);
+	}
+
 	public void setValidationEnabled(boolean state) {
 		validateEnabled = state;
 	}
@@ -105,6 +110,12 @@ public class FunctionDB extends DatabaseObject implements Function {
 		entryPoint = functionSymbol.getAddress();
 	}
 
+	@Override
+	protected void checkDeleted() {
+		// expose method to function package
+		super.checkDeleted();
+	}
+
 	@Override
 	public boolean isThunk() {
 		manager.lock.acquire();
@@ -853,8 +864,7 @@ public class FunctionDB extends DatabaseObject implements Function {
 
 	private void renumberParameterOrdinals() {
 		int ordinal = autoParams != null ? autoParams.size() : 0;
-		for (int i = 0; i < params.size(); i++) {
-			ParameterDB param = params.get(i);
+		for (ParameterDB param : params) {
 			param.setOrdinal(ordinal++);
 		}
 	}
@@ -1096,21 +1106,18 @@ public class FunctionDB extends DatabaseObject implements Function {
 			loadVariables();
 			ArrayList<Variable> list = new ArrayList<>();
 			if (autoParams != null) {
-				for (int i = 0; i < autoParams.size(); i++) {
-					Parameter p = autoParams.get(i);
+				for (AutoParameterImpl p : autoParams) {
 					if (filter == null || filter.matches(p)) {
 						list.add(p);
 					}
 				}
 			}
-			for (int i = 0; i < params.size(); i++) {
-				Parameter p = params.get(i);
+			for (ParameterDB p : params) {
 				if (filter == null || filter.matches(p)) {
 					list.add(p);
 				}
 			}
-			for (int i = 0; i < locals.size(); i++) {
-				Variable var = locals.get(i);
+			for (VariableDB var : locals) {
 				if (filter == null || filter.matches(var)) {
 					list.add(var);
 				}
@@ -1140,15 +1147,13 @@ public class FunctionDB extends DatabaseObject implements Function {
 			loadVariables();
 			ArrayList<Parameter> list = new ArrayList<>();
 			if (autoParams != null) {
-				for (int i = 0; i < autoParams.size(); i++) {
-					Parameter p = autoParams.get(i);
+				for (AutoParameterImpl p : autoParams) {
 					if (filter == null || filter.matches(p)) {
 						list.add(p);
 					}
 				}
 			}
-			for (int i = 0; i < params.size(); i++) {
-				Parameter p = params.get(i);
+			for (ParameterDB p : params) {
 				if (filter == null || filter.matches(p)) {
 					list.add(p);
 				}
@@ -1176,8 +1181,7 @@ public class FunctionDB extends DatabaseObject implements Function {
 			}
 			loadVariables();
 			ArrayList<Variable> list = new ArrayList<>();
-			for (int i = 0; i < locals.size(); i++) {
-				Variable var = locals.get(i);
+			for (VariableDB var : locals) {
 				if (filter == null || filter.matches(var)) {
 					list.add(var);
 				}
@@ -1657,8 +1661,7 @@ public class FunctionDB extends DatabaseObject implements Function {
 					}
 					if (ordinal != params.size()) {
 						// shift params to make room for inserted param
-						for (int i = 0; i < params.size(); i++) {
-							ParameterDB param = params.get(i);
+						for (ParameterDB param : params) {
 							int paramOrdinal = param.getOrdinal();
 							if (paramOrdinal >= ordinal) {
 								param.setOrdinal(paramOrdinal + 1);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionManagerDB.java

@@ -505,7 +505,7 @@ public class FunctionManagerDB implements FunctionManager {
 		try {
 			FunctionDB func = cache.get(key);
 			if (func != null) {
-				func.checkIsValid();
+				func.checkDeleted();
 				func.createClassStructIfNeeded();
 				func.updateParametersAndReturn();
 			}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionStackFrame.java

@@ -1,6 +1,5 @@
 /* ###
  * IP: GHIDRA
- * REVIEWED: YES
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +15,8 @@
  */
 package ghidra.program.database.function;
 
+import java.util.*;
+
 import ghidra.program.model.data.DataType;
 import ghidra.program.model.listing.*;
 import ghidra.program.model.pcode.Varnode;
@@ -23,8 +24,6 @@ import ghidra.program.model.symbol.SourceType;
 import ghidra.util.exception.DuplicateNameException;
 import ghidra.util.exception.InvalidInputException;
 
-import java.util.*;
-
 class FunctionStackFrame implements StackFrame {
 
 	// NOTE: Program stack frame may contain compound variables which have a stack component
@@ -46,17 +45,17 @@ class FunctionStackFrame implements StackFrame {
 	}
 
 	boolean checkIsValid() {
-		if (function.checkIsValid()) {
-			if (invalid) {
-				stackGrowsNegative =
-					function.getFunctionManager().getProgram().getCompilerSpec().stackGrowsNegative();
-				variables = function.getVariables(VariableFilter.COMPOUND_STACK_VARIABLE_FILTER);
-				Arrays.sort(variables, StackVariableComparator.get());
-				invalid = false;
-			}
-			return true;
+		if (function.isDeleted()) {
+			return false;
 		}
-		return false;
+		if (invalid) {
+			stackGrowsNegative =
+				function.getFunctionManager().getProgram().getCompilerSpec().stackGrowsNegative();
+			variables = function.getVariables(VariableFilter.COMPOUND_STACK_VARIABLE_FILTER);
+			Arrays.sort(variables, StackVariableComparator.get());
+			invalid = false;
+		}
+		return true;
 	}
 
 	void checkDeleted() {
@@ -101,9 +100,9 @@ class FunctionStackFrame implements StackFrame {
 					}
 				}
 				else {
-					for (int i = 0; i < params.length; i++) {
-						if (offset >= params[i].getLastStorageVarnode().getOffset()) {
-							ordinal = params[i].getOrdinal();
+					for (Parameter param : params) {
+						if (offset >= param.getLastStorageVarnode().getOffset()) {
+							ordinal = param.getOrdinal();
 						}
 					}
 				}
@@ -159,9 +158,9 @@ class FunctionStackFrame implements StackFrame {
 		try {
 			checkIsValid();
 			ArrayList<Variable> list = new ArrayList<Variable>();
-			for (int i = 0; i < variables.length; i++) {
-				if (!(variables[i] instanceof Parameter)) {
-					list.add(variables[i]);
+			for (Variable variable : variables) {
+				if (!(variable instanceof Parameter)) {
+					list.add(variable);
 				}
 			}
 			Variable[] vars = new Variable[list.size()];
@@ -182,9 +181,9 @@ class FunctionStackFrame implements StackFrame {
 		try {
 			checkIsValid();
 			ArrayList<Parameter> list = new ArrayList<Parameter>();
-			for (int i = 0; i < variables.length; i++) {
-				if (variables[i] instanceof Parameter) {
-					list.add((Parameter) variables[i]);
+			for (Variable variable : variables) {
+				if (variable instanceof Parameter) {
+					list.add((Parameter) variable);
 				}
 			}
 			Parameter[] vars = new Parameter[list.size()];
@@ -329,8 +328,8 @@ class FunctionStackFrame implements StackFrame {
 		try {
 			checkIsValid();
 			int cnt = 0;
-			for (int i = 0; i < variables.length; i++) {
-				if (variables[i] instanceof Parameter) {
+			for (Variable variable : variables) {
+				if (variable instanceof Parameter) {
 					++cnt;
 				}
 			}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/references/ReferenceDBManager.java

@@ -25,7 +25,6 @@ import db.*;
 import db.util.ErrorHandler;
 import ghidra.program.database.*;
 import ghidra.program.database.external.ExternalManagerDB;
-import ghidra.program.database.function.FunctionDB;
 import ghidra.program.database.map.AddressMap;
 import ghidra.program.database.symbol.*;
 import ghidra.program.model.address.*;
@@ -638,13 +637,12 @@ public class ReferenceDBManager implements ReferenceManager, ManagerDB, ErrorHan
 		lock.acquire();
 		try {
 			Function function = var.getFunction();
-			if (!(function instanceof FunctionDB) || function.getProgram() != program ||
-				!((FunctionDB) function).checkIsValid()) {
+			if (function.getProgram() != program || function.isDeleted()) {
 				return NO_REFS;
 			}
 
 			SymbolDB varSymbol = (SymbolDB) var.getSymbol();
-			if (varSymbol != null && !varSymbol.checkIsValid()) {
+			if (varSymbol != null && varSymbol.isDeleted()) {
 				return NO_REFS;
 			}
 

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolDB.java

@@ -82,6 +82,17 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 		lock = symbolMgr.getLock();
 	}
 
+	@Override
+	public boolean isDeleted() {
+		return isDeleted(lock);
+	}
+
+	@Override
+	protected void checkDeleted() {
+		// expose method to symbol package
+		super.checkDeleted();
+	}
+
 	@Override
 	public String toString() {
 		return getName();

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/address/GlobalSymbol.java

@@ -34,8 +34,8 @@ public class GlobalSymbol implements Symbol {
 	}
 
 	@Override
-	public boolean checkIsValid() {
-		return true;
+	public boolean isDeleted() {
+		return false;
 	}
 
 	@Override

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

@@ -711,4 +711,11 @@ public interface Function extends Namespace {
 	 * symbol will be removed. 
 	 */
 	public void promoteLocalUserLabelsToGlobal();
+
+	/**
+	 * Determine if this function object has been deleted.  NOTE: the function could be
+	 * deleted at anytime due to asynchronous activity.  
+	 * @return true if function has been deleted, false if not.
+	 */
+	public boolean isDeleted();
 }

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

@@ -109,6 +109,7 @@ public interface Variable extends Comparable<Variable> {
 	/**
 	 * Returns the function that contains this Variable.  May be null if the variable is not in
 	 * a function.
+	 * @return containing function or null
 	 */
 
 	public Function getFunction();
@@ -190,7 +191,7 @@ public interface Variable extends Comparable<Variable> {
 	/** 
 	 * @return true if this is a simple variable consisting of a single register varnode
 	 * which will be returned by either the {@link #getFirstStorageVarnode()} or 
-	 * {@link getLastStorageVarnode()} methods.  The register can be obtained using the 
+	 * {@link #getLastStorageVarnode()} methods.  The register can be obtained using the 
 	 * {@link #getRegister()} method.
 	 */
 	public boolean isRegisterVariable();
@@ -262,11 +263,13 @@ public interface Variable extends Comparable<Variable> {
 	/**
 	 * @return the symbol associated with this variable or null if no symbol 
 	 * associated.  Certain dynamic variables such as auto-parameters do not
-	 * have a symbol. 
+	 * have a symbol and will return null. 
 	 */
 	public Symbol getSymbol();
 
 	/**
+	 * Determine is another variable is equivalent to this variable.
+	 * @param variable other variable
 	 * @return true if the specified variable is equivalent to this variable
 	 */
 	public boolean isEquivalent(Variable variable);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/symbol/Symbol.java

@@ -40,12 +40,6 @@ public interface Symbol {
 	 */
 	public String getName();
 
-	/**
-	 * Check whether this symbol is still valid (i.e., deleted).
-	 * @return true if valid or false if deleted.
-	 */
-	public boolean checkIsValid();
-
 	/**
 	 * Gets the full path name for this symbol as an ordered array of strings ending
 	 * with the symbol name. The global symbol will return an empty array.
@@ -67,12 +61,14 @@ public interface Symbol {
 	public String getName(boolean includeNamespace);
 
 	/**
-	 * @return the namespace that contains this symbol
+	 * Return the parent namespace for this symbol.
+	 * @return the namespace that contains this symbol.
 	 */
 	public Namespace getParentNamespace();
 
 	/**
 	 * Returns namespace symbol of the namespace containing this symbol
+	 * @return parent namespace symbol
 	 */
 	public Symbol getParentSymbol();
 
@@ -84,14 +80,16 @@ public interface Symbol {
 	public boolean isDescendant(Namespace namespace);
 
 	/**
-	 * Returns whether the given parent is valid for this Symbol.
-	 * @param parent
+	 * Determines if the given parent is valid for this Symbol.  Specified namespace 
+	 * must belong to the same symbol table as this symbol.
+	 * @param parent prospective parent namespace for this symbol
 	 * @return true if parent is valid
 	 */
 	public boolean isValidParent(Namespace parent);
 
 	/**
-	 * Returns the symbol type
+	 * Returns this symbol's type
+	 * @return symbol type
 	 */
 	public SymbolType getSymbolType();
 
@@ -273,4 +271,11 @@ public interface Symbol {
 	 * @return the source of this symbol
 	 */
 	public SourceType getSource();
+
+	/**
+	 * Determine if this symbol object has been deleted.  NOTE: the symbol could be
+	 * deleted at anytime due to asynchronous activity.  
+	 * @return true if symbol has been deleted, false if not.
+	 */
+	public boolean isDeleted();
 }

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/model/FunctionTestDouble.java

@@ -47,6 +47,11 @@ public class FunctionTestDouble implements Function {
 		functionSignature = signature;
 	}
 
+	@Override
+	public boolean isDeleted() {
+		return false;
+	}
+
 	@Override
 	public String toString() {
 		return name;