From 66f37184439b63b93fafd64e28ae953feb042e37 Mon Sep 17 00:00:00 2001 From: ghidravore Date: Tue, 3 Nov 2020 19:26:07 -0500 Subject: [PATCH] GP-364 - Call Trees - fixed bug that caused duplicate nodes to have odd spacing in tree; fixed duplicate filtering --- .../app/plugin/core/calltree/CallNode.java | 52 +++++-- .../core/calltree/CallTreeProvider.java | 7 +- .../app/plugin/core/calltree/DeadEndNode.java | 3 +- .../core/calltree/ExternalCallNode.java | 2 +- .../core/calltree/IncomingCallNode.java | 19 ++- .../core/calltree/OutgoingCallNode.java | 22 ++- .../core/calltree/CallTreePluginTest.java | 128 ++++++++++++------ 7 files changed, 161 insertions(+), 72 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallNode.java index 6394cf6f85..8baa644c03 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallNode.java @@ -20,6 +20,8 @@ import java.util.concurrent.atomic.AtomicInteger; import javax.swing.tree.TreePath; +import org.apache.commons.collections4.map.LazyMap; + import docking.widgets.tree.GTreeNode; import docking.widgets.tree.GTreeSlowLoadingNode; import ghidra.program.model.address.*; @@ -44,7 +46,13 @@ public abstract class CallNode extends GTreeSlowLoadingNode { this.filterDepth = filterDepth; } - public abstract Function getContainingFunction(); + /** + * Returns this node's remote function, where remote is the source function for + * an incoming call or a destination function for an outgoing call. May return + * null for nodes that do not have functions. + * @return the function or null + */ + public abstract Function getRemoteFunction(); /** * Returns a location that represents the caller of the callee. @@ -68,7 +76,7 @@ public abstract class CallNode extends GTreeSlowLoadingNode { protected Set getReferencesFrom(Program program, AddressSetView addresses, TaskMonitor monitor) throws CancelledException { - Set set = new HashSet(); + Set set = new HashSet<>(); ReferenceManager referenceManager = program.getReferenceManager(); AddressIterator addressIterator = addresses.getAddresses(true); while (addressIterator.hasNext()) { @@ -93,15 +101,24 @@ public abstract class CallNode extends GTreeSlowLoadingNode { this.allowDuplicates = allowDuplicates; } - protected void addNode(List nodes, GTreeNode node) { + protected void addNode(LazyMap> nodesByFunction, + CallNode node) { + + Function function = node.getRemoteFunction(); + List nodes = nodesByFunction.get(function); + if (nodes.contains(node)) { + return; // never add equal() nodes + } + if (allowDuplicates) { - nodes.add(node); + nodes.add(node); // ok to add multiple nodes for this function with different addresses + } + + if (nodes.isEmpty()) { + nodes.add(node); // no duplicates allow; only add if this is the only node return; } - if (!nodes.contains(node)) { - nodes.add(node); - } } protected class CallNodeComparator implements Comparator { @@ -133,8 +150,8 @@ public abstract class CallNode extends GTreeSlowLoadingNode { Object[] pathComponents = path.getPath(); for (Object pathComponent : pathComponents) { CallNode node = (CallNode) pathComponent; - Function nodeFunction = node.getContainingFunction(); - Function myFunction = getContainingFunction(); + Function nodeFunction = node.getRemoteFunction(); + Function myFunction = getRemoteFunction(); if (node != this && nodeFunction.equals(myFunction)) { return true; } @@ -142,14 +159,12 @@ public abstract class CallNode extends GTreeSlowLoadingNode { return false; } - // overridden since we may have multiple children with the same function name, but in - // different namespaces @Override public boolean equals(Object obj) { if (this == obj) { return true; } - if (obj == null) { + if (!super.equals(obj)) { return false; } if (getClass() != obj.getClass()) { @@ -157,12 +172,21 @@ public abstract class CallNode extends GTreeSlowLoadingNode { } CallNode other = (CallNode) obj; - return getSourceAddress().equals(other.getSourceAddress()); + if (!Objects.equals(getSourceAddress(), other.getSourceAddress())) { + return false; + } + return Objects.equals(getRemoteFunction(), other.getRemoteFunction()); } @Override public int hashCode() { - return getSourceAddress().hashCode(); + final int prime = 31; + int result = super.hashCode(); + Function function = getRemoteFunction(); + result = prime * result + ((function == null) ? 0 : function.hashCode()); + Address sourceAddress = getSourceAddress(); + result = prime * result + ((sourceAddress == null) ? 0 : sourceAddress.hashCode()); + return result; } } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java index f1795839cb..6fa1209f69 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java @@ -283,7 +283,8 @@ public class CallTreeProvider extends ComponentProviderAdapter implements Domain for (TreePath path : selectionPaths) { GTreeNode node = (GTreeNode) path.getLastPathComponent(); - if (node instanceof GTreeNode) { + if (node instanceof OutgoingCallsRootNode || + node instanceof IncomingCallsRootNode) { return false; } } @@ -1095,7 +1096,7 @@ public class CallTreeProvider extends ComponentProviderAdapter implements Domain //TODO do we need to use a PendingRootNode? if (root instanceof CallNode) { CallNode callNode = (CallNode) root; - Function nodeFunction = callNode.getContainingFunction(); + Function nodeFunction = callNode.getRemoteFunction(); if (nodeFunction.equals(function)) { reloadUpdateManager.update(); return true; @@ -1131,7 +1132,7 @@ public class CallTreeProvider extends ComponentProviderAdapter implements Domain // first, if the given node represents the function we have, then we don't need to // go any further - if (function.equals(node.getContainingFunction())) { + if (function.equals(node.getRemoteFunction())) { GTreeNode parent = node.getParent(); parent.removeNode(node); parent.addNode(node.recreate()); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/DeadEndNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/DeadEndNode.java index dd1b2c3794..96e19bb588 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/DeadEndNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/DeadEndNode.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. @@ -54,7 +53,7 @@ public class DeadEndNode extends CallNode { } @Override - public Function getContainingFunction() { + public Function getRemoteFunction() { return null; // no function--dead end } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/ExternalCallNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/ExternalCallNode.java index 90a5eba235..8e9fbc8655 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/ExternalCallNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/ExternalCallNode.java @@ -61,7 +61,7 @@ public class ExternalCallNode extends CallNode { } @Override - public Function getContainingFunction() { + public Function getRemoteFunction() { return function; } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/IncomingCallNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/IncomingCallNode.java index f8b27381ce..48a45c90e0 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/IncomingCallNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/IncomingCallNode.java @@ -17,9 +17,12 @@ package ghidra.app.plugin.core.calltree; import java.util.*; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import javax.swing.Icon; +import org.apache.commons.collections4.map.LazyMap; + import docking.widgets.tree.GTreeNode; import ghidra.app.plugin.core.navigation.locationreferences.ReferenceUtils; import ghidra.program.model.address.Address; @@ -70,7 +73,7 @@ public class IncomingCallNode extends CallNode { } @Override - public Function getContainingFunction() { + public Function getRemoteFunction() { return function; } @@ -86,7 +89,8 @@ public class IncomingCallNode extends CallNode { new FunctionSignatureFieldLocation(program, functionAddress); Set
addresses = ReferenceUtils.getReferenceAddresses(location, monitor); - List nodes = new ArrayList<>(); + LazyMap> nodesByFunction = + LazyMap.lazyMap(new HashMap<>(), k -> new ArrayList<>()); FunctionManager functionManager = program.getFunctionManager(); for (Address fromAddress : addresses) { monitor.checkCanceled(); @@ -97,12 +101,17 @@ public class IncomingCallNode extends CallNode { IncomingCallNode node = new IncomingCallNode(program, callerFunction, fromAddress, filterDuplicates, filterDepth); - addNode(nodes, node); + addNode(nodesByFunction, node); } - Collections.sort(nodes, new CallNodeComparator()); + List children = + nodesByFunction.values() + .stream() + .flatMap(list -> list.stream()) + .collect(Collectors.toList()); + Collections.sort(children, new CallNodeComparator()); - return nodes; + return children; } @Override diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/OutgoingCallNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/OutgoingCallNode.java index 080c3bd2b0..cac2031e5a 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/OutgoingCallNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/OutgoingCallNode.java @@ -17,10 +17,13 @@ package ghidra.app.plugin.core.calltree; import java.util.*; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import javax.swing.Icon; import javax.swing.tree.TreePath; +import org.apache.commons.collections4.map.LazyMap; + import docking.widgets.tree.GTreeNode; import ghidra.program.model.address.Address; import ghidra.program.model.address.AddressSetView; @@ -66,7 +69,7 @@ public abstract class OutgoingCallNode extends CallNode { } @Override - public Function getContainingFunction() { + public Function getRemoteFunction() { return function; } @@ -75,7 +78,8 @@ public abstract class OutgoingCallNode extends CallNode { AddressSetView functionBody = function.getBody(); Address entryPoint = function.getEntryPoint(); Set references = getReferencesFrom(program, functionBody, monitor); - List nodes = new ArrayList(); + LazyMap> nodesByFunction = + LazyMap.lazyMap(new HashMap<>(), k -> new ArrayList<>()); FunctionManager functionManager = program.getFunctionManager(); for (Reference reference : references) { monitor.checkCanceled(); @@ -85,15 +89,21 @@ public abstract class OutgoingCallNode extends CallNode { } Function calledFunction = functionManager.getFunctionAt(toAddress); - createNode(nodes, reference, calledFunction); + createNode(nodesByFunction, reference, calledFunction); } - Collections.sort(nodes, new CallNodeComparator()); + List children = + nodesByFunction.values() + .stream() + .flatMap(list -> list.stream()) + .collect(Collectors.toList()); + Collections.sort(children, new CallNodeComparator()); - return nodes; + return children; } - private void createNode(List nodes, Reference reference, Function calledFunction) { + private void createNode(LazyMap> nodes, Reference reference, + Function calledFunction) { if (calledFunction != null) { if (isExternalCall(calledFunction)) { CallNode node = diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/calltree/CallTreePluginTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/calltree/CallTreePluginTest.java index 8b517d0e23..4702cce0e4 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/calltree/CallTreePluginTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/calltree/CallTreePluginTest.java @@ -26,6 +26,7 @@ import java.util.concurrent.atomic.AtomicReference; import javax.swing.JTree; import javax.swing.tree.TreePath; +import org.apache.commons.collections4.map.LazyMap; import org.junit.*; import docking.ActionContext; @@ -140,7 +141,10 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { function(0x4000, 0x5000); function(0x5000, 0x6000); function(0x5000, 0x6100); - duplicateReference(0x5000, 0x6000); + + // second reference inside of function 0x5000 to 0x6000 + createReference(0x5020, 0x6000); + function(0x6000, 0x7000); function(0x6100, 0x7100); function(0x7000, 0x8000); @@ -150,14 +154,6 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { return builder.getProgram(); } - private void duplicateReference(int from, int to) { - // a bit of space so the function call is not at the entry point - int offset = from + 10; - while (!createReference(offset, to)) { - offset++; - } - } - private Function function(int addr) throws Exception { return ensureFunction(addr); } @@ -598,7 +594,7 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { } @Test - public void testFilterOutgoingDuplicates() { + public void testFilterOutgoingDuplicates_DifferentSource_SameDestination() { // // Test that the filter action will remove duplicate entries from the child nodes of // the outgoing tree @@ -607,6 +603,38 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { setProviderFunction("0x5000"); + ToggleDockingAction filterDuplicatesAction = + (ToggleDockingAction) getAction("Filter Duplicates"); + setToggleActionSelected(filterDuplicatesAction, new ActionContext(), true); + waitForTree(outgoingTree); + + GTreeNode rootNode = getRootNode(outgoingTree); + boolean shouldHaveDuplicates = false; + Map> nameCountMap = createNameCountMap(rootNode); + assertDuplicateChildStatus(nameCountMap, shouldHaveDuplicates); + + performAction(filterDuplicatesAction, true);// deselect + + waitForTree(outgoingTree); + + rootNode = getRootNode(outgoingTree); + nameCountMap = createNameCountMap(rootNode); + shouldHaveDuplicates = true; + assertDuplicateChildStatus(nameCountMap, shouldHaveDuplicates); + } + + @Test + public void testFilterOutgoingDuplicates_SameSource_SameDestination() { + // + // Test that 2 references from the same source address to the same function will not get + // added to the tree, regardless of the duplicate filter state + // + + // add a second reference (this is already defined in setup) + builder.createMemoryCallReference("0x5020", "0x6000"); + + setProviderFunction("0x5000"); + ToggleDockingAction filterDuplicatesAction = (ToggleDockingAction) getAction("Filter Duplicates"); @@ -614,15 +642,41 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { waitForTree(outgoingTree); GTreeNode rootNode = getRootNode(outgoingTree); - List children = rootNode.getChildren(); - assertTrue( - "Outgoing tree does not have callers as expected for function: " + getListingFunction(), - children.size() > 0); + Map> nameCountMap = createNameCountMap(rootNode); - // copy the names of the children into a map so that we can verify that there are - // no duplicates + // 1, not 2 entries (the exact duplicate and the duplicate destination are ignored) + assertEquals(1, nameCountMap.get("Function_6000").size()); + + performAction(filterDuplicatesAction, true);// deselect + waitForTree(outgoingTree); + + rootNode = getRootNode(outgoingTree); + nameCountMap = createNameCountMap(rootNode); + + // 2, not 3 entries (the exact duplicate is ignored) + assertEquals(2, nameCountMap.get("Function_6000").size()); + } + + @Test + public void testFilterOutgoingDuplicates_SameSource_DifferentDestination() { + // + // Test that 2 references from the same source address to the same function will not get + // added to the tree, regardless of the duplicate filter state + // + + // add a second reference from 0x5020 to a function already called (see setup) + builder.createMemoryCallReference("0x5020", "0x6000"); + + setProviderFunction("0x5000"); + + ToggleDockingAction filterDuplicatesAction = + (ToggleDockingAction) getAction("Filter Duplicates"); + setToggleActionSelected(filterDuplicatesAction, new ActionContext(), true); + waitForTree(outgoingTree); + + GTreeNode rootNode = getRootNode(outgoingTree); boolean shouldHaveDuplicates = false; - Map nameCountMap = createNameCountMap(rootNode); + Map> nameCountMap = createNameCountMap(rootNode); assertDuplicateChildStatus(nameCountMap, shouldHaveDuplicates); performAction(filterDuplicatesAction, true);// deselect @@ -663,16 +717,10 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { setToggleActionSelected(filterDuplicatesAction, new ActionContext(), true); waitForTree(incomingTree); + // copy the names of the children into a map so that we can verify no duplicates GTreeNode rootNode = getRootNode(incomingTree); - List children = rootNode.getChildren(); - assertTrue( - "Outgoing tree does not have callers as expected for function: " + getListingFunction(), - children.size() > 0); - - // copy the names of the children into a map so that we can verify that there are - // no duplicates - boolean shouldHaveDuplicates = false; - Map nameCountMap = createNameCountMap(rootNode); + boolean shouldHaveDuplicates = true; + Map> nameCountMap = createNameCountMap(rootNode); assertDuplicateChildStatus(nameCountMap, shouldHaveDuplicates); } @@ -1135,34 +1183,32 @@ public class CallTreePluginTest extends AbstractGhidraHeadedIntegrationTest { return codeBrowserPlugin.getCurrentAddress(); } - private Map createNameCountMap(GTreeNode node) { - Map map = new HashMap<>(); + private Map> createNameCountMap(GTreeNode node) { + Map> map = LazyMap.lazyMap(new HashMap<>(), k -> new ArrayList<>()); List children = node.getChildren(); for (GTreeNode child : children) { - Integer integer = map.get(child); - if (integer == null) { - integer = 0; - } - int asInt = integer; - asInt++; - map.put(child, asInt); + map.get(child.getName()).add(child); } return map; } - private void assertDuplicateChildStatus(Map childCountMap, + private void assertDuplicateChildStatus(Map> childCountMap, boolean shouldHaveDuplicates) { boolean foundDuplicates = false; - Set> entrySet = childCountMap.entrySet(); - for (Entry entry : entrySet) { - int value = entry.getValue(); - if (value != 1) { + Set>> entrySet = childCountMap.entrySet(); + String duplicateName = null; + for (Entry> entry : entrySet) { + List list = entry.getValue(); + if (list.size() != 1) { + duplicateName = entry.getKey(); foundDuplicates = true; + break; } } String errorMessage = - (shouldHaveDuplicates ? "Did not find " : "Found") + " duplicate child entries"; + (shouldHaveDuplicates ? "Did not find " : "Found") + " duplicate child entries for '" + + duplicateName + "'"; assertEquals(errorMessage, shouldHaveDuplicates, foundDuplicates); }