From 4fbbe989be4dcd20602c49dd414d4d8653521671 Mon Sep 17 00:00:00 2001 From: dev747368 <48332326+dev747368@users.noreply.github.com> Date: Fri, 20 Dec 2019 11:46:25 -0500 Subject: [PATCH] GT-3414, issue #1259 - fix GUI lockup when defined strings table loads A large binary with lots of strings can cause the "Defined Strings" table to fight with the Listing view over database locks. This change eases some of the issues by making the defined strings table be nicer by yielding to the swing thread before starting its query loop (which allows a Listing view which is also doing its full rendering at the same time to finish before the loop starts) and making the DefinedDataIterator a bit smarter about what elements it needs to recurse into. The Listing can still be quite a bit laggy when the Defined Strings table is loading. Also fixed a logic error in StringDataInstance that caused it to return a empty string for arrays of character elements. --- .../TranslateStringsScript.java | 4 +- .../core/strings/ViewStringsTableModel.java | 12 +- .../model/data/StringDataInstance.java | 19 +++ .../program/model/listing/DataIterator.java | 45 +++-- .../program/util/DefinedDataIterator.java | 156 +++++++++++++----- .../demangler/DemangledAddressTableTest.java | 3 +- 6 files changed, 172 insertions(+), 67 deletions(-) diff --git a/Ghidra/Features/Base/ghidra_scripts/TranslateStringsScript.java b/Ghidra/Features/Base/ghidra_scripts/TranslateStringsScript.java index a63ce964d3..7d49f7ea86 100644 --- a/Ghidra/Features/Base/ghidra_scripts/TranslateStringsScript.java +++ b/Ghidra/Features/Base/ghidra_scripts/TranslateStringsScript.java @@ -21,6 +21,7 @@ import ghidra.program.model.data.StringDataInstance; import ghidra.program.model.data.TranslationSettingsDefinition; import ghidra.program.model.listing.Data; import ghidra.program.util.DefinedDataIterator; +import util.CollectionUtils; public class TranslateStringsScript extends GhidraScript { @@ -39,7 +40,8 @@ public class TranslateStringsScript extends GhidraScript { int count = 0; monitor.initialize(currentProgram.getListing().getNumDefinedData()); monitor.setMessage("Translating strings"); - for (Data data : DefinedDataIterator.definedStrings(currentProgram, currentSelection)) { + for (Data data : CollectionUtils.asIterable( + DefinedDataIterator.definedStrings(currentProgram, currentSelection))) { if (monitor.isCancelled()) { break; } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/strings/ViewStringsTableModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/strings/ViewStringsTableModel.java index 5a97f4a334..8bcab1c68c 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/strings/ViewStringsTableModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/strings/ViewStringsTableModel.java @@ -30,6 +30,7 @@ import ghidra.program.model.data.*; import ghidra.program.model.listing.*; import ghidra.program.util.*; import ghidra.util.StringUtilities; +import ghidra.util.Swing; import ghidra.util.datastruct.Accumulator; import ghidra.util.exception.CancelledException; import ghidra.util.table.AddressBasedTableModel; @@ -38,6 +39,7 @@ import ghidra.util.table.column.GColumnRenderer; import ghidra.util.table.field.AbstractProgramLocationTableColumn; import ghidra.util.table.field.AddressBasedLocation; import ghidra.util.task.TaskMonitor; +import util.CollectionUtils; /** * Table model for the "Defined Strings" table. @@ -111,9 +113,10 @@ class ViewStringsTableModel extends AddressBasedTableModel { Listing listing = localProgram.getListing(); monitor.setCancelEnabled(true); - monitor.initialize((int) listing.getNumDefinedData()); - - for (Data stringInstance : DefinedDataIterator.definedStrings(localProgram)) { + monitor.initialize(listing.getNumDefinedData()); + Swing.allowSwingToProcessEvents(); + for (Data stringInstance : CollectionUtils.asIterable( + DefinedDataIterator.definedStrings(localProgram))) { accumulator.add(createIndexedStringInstanceLocation(localProgram, stringInstance)); monitor.checkCanceled(); monitor.incrementProgress(1); @@ -139,7 +142,8 @@ class ViewStringsTableModel extends AddressBasedTableModel { } public void addDataInstance(Program localProgram, Data data, TaskMonitor monitor) { - for (Data stringInstance : DefinedDataIterator.definedStrings(data)) { + for (Data stringInstance : CollectionUtils.asIterable( + DefinedDataIterator.definedStrings(data))) { addObject(createIndexedStringInstanceLocation(localProgram, stringInstance)); } } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StringDataInstance.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StringDataInstance.java index 95c0c61b22..9e4120b14d 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StringDataInstance.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StringDataInstance.java @@ -65,6 +65,25 @@ public class StringDataInstance { return false; } + /** + * Returns true if the specified {@link DataType} is (or could be) a + * string. + *

+ * Arrays of char-like elements (see {@link ArrayStringable}) are treated + * as string data types. The actual data instance needs to be inspected + * to determine if the array is an actual string. + *

+ * @param dt DataType to test + * @return boolean true if data type is or could be a string + */ + public static boolean isStringDataType(DataType dt) { + if (dt instanceof TypeDef) { + dt = ((TypeDef) dt).getBaseDataType(); + } + return dt instanceof AbstractStringDataType || (dt instanceof Array && + ArrayStringable.getArrayStringable(((Array) dt).getDataType()) != null); + } + /** * Returns true if the {@link Data} instance is one of the many 'char' data types. * diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/DataIterator.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/DataIterator.java index 5bf0713c05..7ce00fda13 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/DataIterator.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/DataIterator.java @@ -15,8 +15,8 @@ */ package ghidra.program.model.listing; +import java.util.Arrays; import java.util.Iterator; -import java.util.NoSuchElementException; import util.CollectionUtils; @@ -25,8 +25,12 @@ import util.CollectionUtils; * * @see CollectionUtils#asIterable */ -public interface DataIterator extends Iterator, Iterable { - public static final DataIterator EMPTY = createEmptyIterator(); +public interface DataIterator extends Iterator { + public static final DataIterator EMPTY = Of(/*nothing*/); + + public static DataIterator Of(Data... dataInstances) { + return new IteratorWrapper(Arrays.asList(dataInstances).iterator()); + } @Override public boolean hasNext(); @@ -34,21 +38,26 @@ public interface DataIterator extends Iterator, Iterable { @Override public Data next(); - @Override - default Iterator iterator() { - return this; - } + // -------------------------------------------------------------------------------- + // Helper static stuff + // -------------------------------------------------------------------------------- + + static class IteratorWrapper implements DataIterator { + private Iterator it; + + IteratorWrapper(Iterator it) { + this.it = it; + } + + @Override + public boolean hasNext() { + return it.hasNext(); + } + + @Override + public Data next() { + return it.next(); + } - // -------------------------------------------------------------------------------- - // Helper static methods - // -------------------------------------------------------------------------------- - public static DataIterator createEmptyIterator() { - return new DataIterator() { - //@formatter:off - @Override public Data next() { throw new NoSuchElementException(); } - @Override public void remove() { throw new IllegalStateException(); } - @Override public boolean hasNext() { return false; } - //@formatter:on - }; } } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/DefinedDataIterator.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/DefinedDataIterator.java index b037892156..31f8c96712 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/DefinedDataIterator.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/DefinedDataIterator.java @@ -15,8 +15,7 @@ */ package ghidra.program.util; -import java.util.LinkedList; -import java.util.Queue; +import java.util.*; import java.util.function.Predicate; import ghidra.program.model.address.AddressSetView; @@ -42,8 +41,7 @@ public class DefinedDataIterator implements DataIterator { */ public static DefinedDataIterator byDataType(Program program, Predicate dataTypePredicate) { - return new DefinedDataIterator(program, null, - data -> dataTypePredicate.test(data.getBaseDataType())); + return new DefinedDataIterator(program, null, dataTypePredicate, null); } /** @@ -53,7 +51,9 @@ public class DefinedDataIterator implements DataIterator { * @return new iterator */ public static DefinedDataIterator definedStrings(Program program) { - return new DefinedDataIterator(program, null, data -> StringDataInstance.isString(data)); + return new DefinedDataIterator(program, null, + dataType -> StringDataInstance.isStringDataType(dataType), + data -> StringDataInstance.isString(data)); } /** @@ -64,7 +64,9 @@ public class DefinedDataIterator implements DataIterator { * @return new iterator */ public static DefinedDataIterator definedStrings(Program program, AddressSetView addrs) { - return new DefinedDataIterator(program, addrs, data -> StringDataInstance.isString(data)); + return new DefinedDataIterator(program, addrs, + dataType -> StringDataInstance.isStringDataType(dataType), + data -> StringDataInstance.isString(data)); } /** @@ -76,73 +78,141 @@ public class DefinedDataIterator implements DataIterator { */ public static DefinedDataIterator definedStrings(Data singleDataInstance) { return new DefinedDataIterator(singleDataInstance, + dataType -> StringDataInstance.isStringDataType(dataType), data -> StringDataInstance.isString(data)); } - private Queue resultsQueue = new LinkedList<>(); + private Predicate dataTypePredicate; private Predicate dataInstancePredicate; - private DataIterator definedDataIterator; + private Deque itStack = new ArrayDeque<>(); + private Data currentDataResult; private DefinedDataIterator(Program program, AddressSetView addrs, - Predicate dataInstancePredicate) { + Predicate dataTypePredicate, Predicate dataInstancePredicate) { + this.dataTypePredicate = dataTypePredicate; this.dataInstancePredicate = dataInstancePredicate; - this.definedDataIterator = program.getListing().getDefinedData( - (addrs == null) ? program.getMemory().getAllInitializedAddressSet() : addrs, true); + + itStack.addLast(program.getListing().getDefinedData( + (addrs == null) ? program.getMemory().getAllInitializedAddressSet() : addrs, true)); } - private DefinedDataIterator(Data singleDataInstance, Predicate dataInstancePredicate) { + private DefinedDataIterator(Data singleDataInstance, Predicate dataTypePredicate, + Predicate dataInstancePredicate) { + this.dataTypePredicate = dataTypePredicate; this.dataInstancePredicate = dataInstancePredicate; - this.definedDataIterator = DataIterator.EMPTY; - processDataInstance(singleDataInstance); + + itStack.addLast(DataIterator.Of(singleDataInstance)); } @Override public boolean hasNext() { - if (resultsQueue.isEmpty()) { + if (currentDataResult == null) { findNext(); } - return !resultsQueue.isEmpty(); + return currentDataResult != null; } @Override public Data next() { - if (hasNext()) { - return resultsQueue.remove(); + if (currentDataResult == null) { + throw new NoSuchElementException(); } - return null; + Data result = currentDataResult; + currentDataResult = null; + return result; + } + + private DataIterator currentIt() { + DataIterator it = null; + while ((it = itStack.peekLast()) != null && !it.hasNext()) { + itStack.removeLast(); + } + return it; } private void findNext() { - while (definedDataIterator.hasNext() && resultsQueue.isEmpty()) { - Data nextData = definedDataIterator.next(); - processDataInstance(nextData); - } - } - - private void processDataInstance(Data data) { - if (dataInstancePredicate.test(data)) { - resultsQueue.add(data); - return; - } - DataType dt = data.getBaseDataType(); - if (dt instanceof Composite || isIterableArray(dt)) { - for (int compNum = 0, compCount = - data.getNumComponents(); compNum < compCount; compNum++) { - Data componentData = data.getComponent(compNum); - processDataInstance(componentData); + DataIterator it = null; + while ((it = currentIt()) != null) { + Data data = it.next(); + DataType dt = data.getBaseDataType(); + if (matchesDataTypePredicate(dt) && matchesDataInstancePredicate(data)) { + currentDataResult = data; + return; + } + if (dataTypePredicate != null && isContainerDT(dt) && + recursiveMatchesDataTypePredicate(dt)) { + itStack.addLast(new DataComponentIterator(data)); } } } - private boolean isIterableArray(DataType dataType) { - if (dataType instanceof Array) { - DataType elementDT = ((Array) dataType).getDataType(); - if (elementDT instanceof TypeDef) { - elementDT = ((TypeDef) elementDT).getBaseDataType(); + private boolean isContainerDT(DataType dt) { + return dt instanceof Array || dt instanceof Composite; + } + + private boolean recursiveMatchesDataTypePredicate(DataType dt) { + if (matchesDataTypePredicate(dt)) { + return true; + } + if (dt instanceof Array) { + Array arrayDT = (Array) dt; + DataType elementDT = arrayDT.getDataType(); + return recursiveMatchesDataTypePredicate(elementDT); + } + else if (dt instanceof Structure) { + Structure comp = (Structure) dt; + for (DataTypeComponent dtc : comp.getDefinedComponents()) { + if (recursiveMatchesDataTypePredicate(dtc.getDataType())) { + return true; + } } - return (elementDT instanceof Array) || (elementDT instanceof Composite) || - (elementDT instanceof AbstractStringDataType); + return false; + } + else if (dt instanceof Composite) { + Composite comp = (Composite) dt; + for (DataTypeComponent dtc : comp.getComponents()) { + if (recursiveMatchesDataTypePredicate(dtc.getDataType())) { + return true; + } + } + return false; + } + else if (dt instanceof TypeDef) { + TypeDef tdDT = (TypeDef) dt; + return recursiveMatchesDataTypePredicate(tdDT.getBaseDataType()); } return false; } + + private boolean matchesDataTypePredicate(DataType dt) { + return dataTypePredicate == null || dataTypePredicate.test(dt); + } + + private boolean matchesDataInstancePredicate(Data data) { + return dataInstancePredicate == null || dataInstancePredicate.test(data); + } + + private static class DataComponentIterator implements DataIterator { + private Data data; + private int currentIndex; + private int elementCount; + + public DataComponentIterator(Data data) { + this.data = data; + this.elementCount = data.getNumComponents(); + } + + @Override + public boolean hasNext() { + return currentIndex < elementCount; + } + + @Override + public Data next() { + Data result = data.getComponent(currentIndex); + currentIndex++; + return result; + } + + } } diff --git a/Ghidra/Test/IntegrationTest/src/test.slow/java/ghidra/app/util/demangler/DemangledAddressTableTest.java b/Ghidra/Test/IntegrationTest/src/test.slow/java/ghidra/app/util/demangler/DemangledAddressTableTest.java index 6889b77e14..794ae412c4 100644 --- a/Ghidra/Test/IntegrationTest/src/test.slow/java/ghidra/app/util/demangler/DemangledAddressTableTest.java +++ b/Ghidra/Test/IntegrationTest/src/test.slow/java/ghidra/app/util/demangler/DemangledAddressTableTest.java @@ -28,6 +28,7 @@ import ghidra.program.model.symbol.*; import ghidra.test.AbstractGhidraHeadlessIntegrationTest; import ghidra.test.ToyProgramBuilder; import ghidra.util.task.TaskMonitor; +import util.CollectionUtils; public class DemangledAddressTableTest extends AbstractGhidraHeadlessIntegrationTest { @@ -227,7 +228,7 @@ public class DemangledAddressTableTest extends AbstractGhidraHeadlessIntegration private void assertPointersAt(int totalNonPointerData, int totalInstructions, String... addrs) { Listing listing = program.getListing(); int index = 0; - for (Data d : listing.getDefinedData(true)) { + for (Data d : CollectionUtils.asIterable(listing.getDefinedData(true))) { if (d.isPointer()) { assertTrue("too many pointers found, expected only " + addrs.length, index < addrs.length);