From 51b1b51d896ddf420c37db1e76f9a4115a27b550 Mon Sep 17 00:00:00 2001 From: caheckman <48068198+caheckman@users.noreply.github.com> Date: Thu, 9 Feb 2023 16:36:23 -0500 Subject: [PATCH] GP-1123 Check for name collision when creating placeholder structure --- .../symbol/DBTraceFunctionSymbol.java | 33 +++++++++---------- .../actions/FillOutStructureCmd.java | 3 ++ .../program/database/function/FunctionDB.java | 12 ++++--- .../model/listing/VariableUtilities.java | 30 +++++++++++++++-- 4 files changed, 54 insertions(+), 24 deletions(-) diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/symbol/DBTraceFunctionSymbol.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/symbol/DBTraceFunctionSymbol.java index dc9af1ae96..f43e53a648 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/symbol/DBTraceFunctionSymbol.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/symbol/DBTraceFunctionSymbol.java @@ -253,8 +253,8 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol DBTraceBookmarkType errType = manager.trace.getBookmarkManager().getOrDefineBookmarkType(BookmarkType.ERROR); manager.trace.getBookmarkManager() - .addBookmark(getLifespan(), entryPoint, errType, - "Bad Variables Removed", "Removed " + badns.size() + " bad variables"); + .addBookmark(getLifespan(), entryPoint, errType, "Bad Variables Removed", + "Removed " + badns.size() + " bad variables"); for (AbstractDBTraceVariableSymbol s : badns) { s.delete(); } @@ -366,9 +366,9 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol } this.callFixup = newCallFixup; update(FIXUP_COLUMN); - manager.trace.setChanged( - new TraceChangeRecord<>(TraceFunctionChangeType.CHANGED_CALL_FIXUP, getSpace(), - this, oldCallFixup, newCallFixup)); + manager.trace + .setChanged(new TraceChangeRecord<>(TraceFunctionChangeType.CHANGED_CALL_FIXUP, + getSpace(), this, oldCallFixup, newCallFixup)); } } @@ -498,8 +498,8 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol } StringBuilder sb = new StringBuilder(); DBTraceParameterSymbol retVar = getReturn(); - sb.append((formalSignature ? retVar.getFormalDataType() - : retVar.getDataType()).getDisplayName()); + sb.append((formalSignature ? retVar.getFormalDataType() : retVar.getDataType()) + .getDisplayName()); sb.append(' '); if (includeCallingConvention && hasExplicitCallingConvention()) { String cc = getCallingConventionName(); @@ -1626,9 +1626,9 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol else { andFlags(NO_RETURN_CLEAR); } - manager.trace.setChanged( - new TraceChangeRecord<>(TraceFunctionChangeType.CHANGED_NORETURN, getSpace(), this, - !hasNoReturn, hasNoReturn)); + manager.trace + .setChanged(new TraceChangeRecord<>(TraceFunctionChangeType.CHANGED_NORETURN, + getSpace(), this, !hasNoReturn, hasNoReturn)); } } @@ -1833,7 +1833,9 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol // NOTE: Check for existence first, to avoid resolving unnecessarily. // TODO: If ever struct-class are strongly related, fix that here, too. classStruct = VariableUtilities.findOrCreateClassStruct((GhidraClass) parentNS, dtm); - dtm.resolve(classStruct, null); + if (classStruct != null) { + dtm.resolve(classStruct, null); + } } } @@ -1858,8 +1860,7 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol } private List
getFunctionThunkAddresses(long functionKey, boolean recursive) { - Collection thunkSymbols = - manager.functionsByThunked.get(getKey()); + Collection thunkSymbols = manager.functionsByThunked.get(getKey()); if (thunkSymbols == null || thunkSymbols.isEmpty()) { return null; } @@ -1916,8 +1917,7 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol try (LockHold hold = LockHold.lock(manager.lock.readLock())) { Set result = new HashSet<>(); for (DBTraceReference ref : manager.trace.getReferenceManager() - .getReferencesToRange( - lifespan, new AddressRangeImpl(entryPoint, entryPoint))) { + .getReferencesToRange(lifespan, new AddressRangeImpl(entryPoint, entryPoint))) { if (monitor.isCancelled()) { break; } @@ -1942,8 +1942,7 @@ public class DBTraceFunctionSymbol extends DBTraceNamespaceSymbol Set result = new HashSet<>(); for (AddressRange rng : getBody()) { for (DBTraceReference ref : manager.trace.getReferenceManager() - .getReferencesFromRange( - lifespan, rng)) { + .getReferencesFromRange(lifespan, rng)) { if (monitor.isCancelled()) { return result; } diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/FillOutStructureCmd.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/FillOutStructureCmd.java index 63a724ffee..f233e4bc98 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/FillOutStructureCmd.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/FillOutStructureCmd.java @@ -516,6 +516,9 @@ public class FillOutStructureCmd extends BackgroundCommand { return null; } Structure structDT = VariableUtilities.findOrCreateClassStruct(f); + if (structDT == null) { + return null; + } // FIXME: How should an existing packed structure be handled? Growing and offset-based placement does not apply int len = structDT.isZeroLength() ? 0 : structDT.getLength(); if (len < size) { diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionDB.java index 29083ec529..fa60100546 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/function/FunctionDB.java @@ -209,7 +209,7 @@ public class FunctionDB extends DatabaseObject implements Function { @Override public ExternalLocation getExternalLocation() { if (isExternal()) { - ExternalManagerDB extMgr = (ExternalManagerDB) program.getExternalManager(); + ExternalManagerDB extMgr = program.getExternalManager(); return extMgr.getExternalLocation(getSymbol()); } return null; @@ -993,7 +993,7 @@ public class FunctionDB extends DatabaseObject implements Function { v.setStorageAndDataType(storage, var.getDataType()); } else { - SymbolManager symbolMgr = (SymbolManager) program.getSymbolTable(); + SymbolManager symbolMgr = program.getSymbolTable(); VariableSymbolDB s = symbolMgr.createVariableSymbol(name, this, SymbolType.LOCAL_VAR, firstUseOffset, storage, source); s.setStorageAndDataType(storage, var.getDataType()); @@ -1460,7 +1460,7 @@ public class FunctionDB extends DatabaseObject implements Function { } // Append new parameters if needed - SymbolManager symbolMgr = (SymbolManager) program.getSymbolTable(); + SymbolManager symbolMgr = program.getSymbolTable(); for (int i = newParamIndex; i < newParams.size(); i++) { Variable newParam = newParams.get(i); DataType dt = (newParam instanceof Parameter && !useCustomStorage) @@ -1683,7 +1683,7 @@ public class FunctionDB extends DatabaseObject implements Function { } } } - SymbolManager symbolMgr = (SymbolManager) program.getSymbolTable(); + SymbolManager symbolMgr = program.getSymbolTable(); VariableSymbolDB s = symbolMgr.createVariableSymbol(name, this, SymbolType.PARAMETER, ordinal, storage, paramSource); s.setStorageAndDataType(storage, var.getDataType()); @@ -2671,7 +2671,9 @@ public class FunctionDB extends DatabaseObject implements Function { // relationship between a class namespace and its structure is needed // so its name and category can track properly. classStruct = VariableUtilities.findOrCreateClassStruct(this); - dataTypeManager.resolve(classStruct, null); + if (classStruct != null) { + dataTypeManager.resolve(classStruct, null); + } } } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableUtilities.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableUtilities.java index 781317ce39..066e7871fd 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableUtilities.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableUtilities.java @@ -750,17 +750,37 @@ public class VariableUtilities { * Create an empty placeholder class structure whose category is derived from * the function's class namespace. NOTE: The structure will not be added to the data * type manager. + * The structure is created in the program's preferred category, or in the root category + * if a preferred category has not been set. If a colliding data-type matching the class name + * and category already exists, null is returned. * @param classNamespace class namespace * @param dataTypeManager data type manager's whose data organization should be applied. - * @return new class structure + * @return new class structure (or null if there is a collision) */ private static Structure createPlaceholderClassStruct(GhidraClass classNamespace, DataTypeManager dataTypeManager) { Namespace classParentNamespace = classNamespace.getParentNamespace(); + CategoryPath prefRoot = null; + if (dataTypeManager instanceof ProgramBasedDataTypeManager) { + prefRoot = ((ProgramBasedDataTypeManager) dataTypeManager).getProgram() + .getPreferredRootNamespaceCategoryPath(); + } + if (prefRoot == null) { + prefRoot = CategoryPath.ROOT; + } CategoryPath category = - DataTypeUtilities.getDataTypeCategoryPath(CategoryPath.ROOT, classParentNamespace); + DataTypeUtilities.getDataTypeCategoryPath(prefRoot, classParentNamespace); + DataType existingDT = dataTypeManager.getDataType(category, classNamespace.getName()); + if (existingDT != null) { + // If a data-type already exists in the parent, try to create in the child + category = DataTypeUtilities.getDataTypeCategoryPath(prefRoot, classNamespace); + existingDT = dataTypeManager.getDataType(category, classNamespace.getName()); + if (existingDT != null) { // If this also already exists + return null; // Don't create a placeholder + } + } StructureDataType structDT = new StructureDataType(category, classNamespace.getName(), 0, dataTypeManager); structDT.setDescription("PlaceHolder Class Structure"); @@ -782,6 +802,9 @@ public class VariableUtilities { * and may refer to a non-existing category path which corresponds to the class-parent's * namespace. * + * If an unrelated data-type already exists matching the class name and category, + * null is returned. + * * @param classNamespace class namespace * @param dataTypeManager data type manager which should be searched and whose * data organization should be used. @@ -811,6 +834,9 @@ public class VariableUtilities { * and may refer to a non-existing category path which corresponds to the class-parent's * namespace. * + * If the function is not part of a class, or if an unrelated data-type already exists with + * the class's name and category, null is returned. + * * @param function function's whose class namespace is the basis for the structure * @return new or existing structure whose name matches the function's class namespace or * null if function not contained within a class namespace.