From 2d5f72c83c2e23b6b17becc5b8cc29c77b9283b2 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Tue, 8 Nov 2022 18:11:40 -0500 Subject: [PATCH 1/3] GP-2810 - Fixed tooltip text for running tool buttons to include the tool's title --- .../ghidra/framework/main/ToolButton.java | 68 ++++++++----------- 1 file changed, 27 insertions(+), 41 deletions(-) diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/ToolButton.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/ToolButton.java index ac9279f688..7561d04a23 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/ToolButton.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/ToolButton.java @@ -41,20 +41,19 @@ import ghidra.util.bean.GGlassPane; import ghidra.util.exception.AssertException; import help.Help; import help.HelpService; +import utility.function.Dummy; /** * Component that is a drop target for a DataTreeTransferable object. * If the object contains a domain file that is supported by a tool of * this tool template, then a tool is launched with the data in it. *

- * This button can be used in one of two ways: to launch new instances of an associated tool + * This button can be used in one of two ways: to launch new instances of an associated tool * template, or to represent a running tool. */ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { - private static final Runnable DUMMY_CALLBACK_RUNNABLE = () -> { - // dummy - }; + private static final Runnable DUMMY_RUNNABLE = Dummy.runnable(); private DropTarget dropTarget; private DropTgtAdapter dropTargetAdapter; @@ -74,6 +73,8 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { /** * Construct a tool button that does not represent a running tool, using * the default tool icon. + * @param plugin the plugin + * @param template the template */ ToolButton(FrontEndPlugin plugin, ToolTemplate template) { this(plugin, null, template, template.getIconURL()); @@ -83,6 +84,9 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { /** * Construct a tool label that represents a running tool, using the * default RUNNING_TOOL icon. + * @param plugin the plugin + * @param tool the running tool + * @param template the template */ ToolButton(FrontEndPlugin plugin, PluginTool tool, ToolTemplate template) { this(plugin, tool, template, tool.getIconURL()); @@ -99,7 +103,6 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { associatedRunningTool = tool; this.template = template; setUpDragDrop(); - setToolTipText(generateToolTipText()); // configure the look and feel of the button setVerticalTextPosition(SwingConstants.BOTTOM); @@ -135,14 +138,11 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { } } - private String generateToolTipText() { + @Override + public String getToolTipText() { if (associatedRunningTool != null) { - if (associatedRunningTool instanceof PluginTool) { - return "" + - HTMLUtilities.escapeHTML(associatedRunningTool.getToolFrame().getTitle()); - } - - return "" + HTMLUtilities.escapeHTML(associatedRunningTool.getName()); + return "" + + HTMLUtilities.escapeHTML(associatedRunningTool.getToolFrame().getTitle()); } return "" + HTMLUtilities.escapeHTML(template.getName()); } @@ -151,9 +151,9 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { doLaunchTool(new DomainFile[] { domainFile }); } -//================================================================================================== +//================================================================================================== // Droppable interface -//================================================================================================== +//================================================================================================== /** * Set drag feedback according to the OK parameter. @@ -211,7 +211,7 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { /** * The given list must contain only valid domain files (i.e., no folders or null items) - * @param nodeList The list of DataTreeNode objects to validate + * @param fileList The list of file objects to validate * @return true if all items in the list are supported */ private boolean containsSupportedDataTypes(List fileList) { @@ -239,7 +239,7 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { * calls this method from its drop() method. * * @param obj Transferable object that is to be dropped. - * @param e has current state of drop operation + * @param event has current state of drop operation * @param f represents the opaque concept of a data format as * would appear on a clipboard, during drag and drop. */ @@ -375,8 +375,8 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { return task.getVersionedObject(); } -//================================================================================================== -// Draggable interface +//================================================================================================== +// Draggable interface //================================================================================================== /** Fix the button state after dragging/dropping, since this is broken in Java */ @@ -402,7 +402,7 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { // Unusual Code Alert! // When dragging, we do not get mouseReleased() events, which we use to launch tools. - // In this case, the drag was cancelled; if we are over ourselves, then simulate + // In this case, the drag was cancelled; if we are over ourselves, then simulate // the Java-eaten mouseReleased() call Container parent = getParent(); if (parent == null) { @@ -457,11 +457,6 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { return plugin.getToolButtonTransferable(); } - /** - * Do the move operation; called when the drag and drop operation - * completes. - * @see ghidra.util.bean.dnd.DragSourceAdapter#dragDropEnd - */ @Override public void move() { resetButtonAfterDrag(this); @@ -484,11 +479,8 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { //================================================================================================== // Package methods -//================================================================================================== +//================================================================================================== - /** - * Set the tool template for this button. - */ void setToolTemplate(ToolTemplate template, Icon icon) { this.template = template; setIcon(icon); @@ -498,16 +490,10 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { return template; } - /** - * Return whether this tool button represents a running tool. - */ boolean isRunningTool() { return associatedRunningTool != null; } - /** - * Close the running tool. - */ void closeTool() { associatedRunningTool.close(); } @@ -548,7 +534,7 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { } private void openFilesAndOpenToolAsNecessary(final DomainFile[] domainFiles) { - openFilesAndOpenToolAsNecessary(domainFiles, DUMMY_CALLBACK_RUNNABLE); + openFilesAndOpenToolAsNecessary(domainFiles, DUMMY_RUNNABLE); } private void openFilesAndOpenToolAsNecessary(final DomainFile[] domainFiles, @@ -565,7 +551,7 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { Component glassPane = toolFrame.getGlassPane(); if (!(glassPane instanceof GGlassPane)) { // We cannot perform the tool launching animation, so just do the old fashion way - Msg.debug(this, "Found root frame without a GhidraGlassPane registered!"); + Msg.debug(this, "Found root frame without a GGlassPane registered!"); // try to recover without animation PluginTool newTool = plugin.getActiveWorkspace().runTool(template); @@ -600,9 +586,9 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { // the final point over which the image will be painted Rectangle endBounds = new Rectangle(new Point(0, 0), frameSize); - // Create our animation code: a zooming effect and an effect to move where the image is - // painted. These effects are independent code-wise, but work together in that the - // mover will set the location and size, and the zoomer will will paint the image with + // Create our animation code: a zooming effect and an effect to move where the image is + // painted. These effects are independent code-wise, but work together in that the + // mover will set the location and size, and the zoomer will will paint the image with // a transparency and a zoom level, which is affected by the movers bounds changing. Image image = ZoomedImagePainter.createIconImage(icon); final ZoomedImagePainter painter = new ZoomedImagePainter(startBounds, image); @@ -690,8 +676,8 @@ class ToolButton extends EmptyBorderButton implements Draggable, Droppable { } //================================================================================================== -// Inner Classes -//================================================================================================== +// Inner Classes +//================================================================================================== private class ToolChangeListener implements DefaultToolChangeListener { private final ToolTemplate toolTemplate; From 277a0f5843a8fcd04e876e02e944a37df9f814c6 Mon Sep 17 00:00:00 2001 From: ghidra1 Date: Wed, 9 Nov 2022 12:12:14 -0500 Subject: [PATCH 2/3] GP-2777 Structure build-up and resolution performance improvements --- .../util/bin/format/pe/cli/blobs/CliBlob.java | 13 +- .../format/pe/cli/streams/CliStreamBlob.java | 6 +- .../program/database/data/CompositeDB.java | 5 +- .../database/data/DataTypeComponentDB.java | 41 +++-- .../program/database/data/StructureDB.java | 148 +++++++++++++----- .../ghidra/program/database/data/UnionDB.java | 2 +- .../model/data/DataTypeComponentImpl.java | 41 +++-- .../program/model/data/StructureDataType.java | 83 +++++++--- .../program/model/data/UnionDataType.java | 6 +- 9 files changed, 255 insertions(+), 90 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliBlob.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliBlob.java index 70fdf198c0..6ab46a5fbe 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliBlob.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliBlob.java @@ -159,7 +159,18 @@ public class CliBlob implements StructConverter { @Override public DataType toDataType() { - Structure struct = new StructureDataType(new CategoryPath(PATH), "Blob_" + getName(), 0); + return toDataType(null); + } + + /** + * Create CLI Blob structure. + * NOTE: This form is provided to reduce resolution time when target datatype manager is known. + * @param dtm datatype manager or null if target datatype manager is unknown. + * @return blob structure + */ + public DataType toDataType(DataTypeManager dtm) { + Structure struct = + new StructureDataType(new CategoryPath(PATH), "Blob_" + getName(), 0, dtm); struct.add(getSizeDataType(), "Size", "coded integer - blob size"); struct.add(getContentsDataType(), getContentsName(), getContentsComment()); return struct; diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/streams/CliStreamBlob.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/streams/CliStreamBlob.java index 87b3ea13f2..05df8151af 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/streams/CliStreamBlob.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/streams/CliStreamBlob.java @@ -4,9 +4,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -122,7 +122,7 @@ public class CliStreamBlob extends CliAbstractStream { // Make sure the old blob has the same size as the new blob DataType oldBlobDataType = oldBlobDataComponent.getDataType(); - DataType newBlobDataType = updatedBlob.toDataType(); + DataType newBlobDataType = updatedBlob.toDataType(program.getDataTypeManager()); if (oldBlobDataType.getLength() != newBlobDataType.getLength()) { Msg.error(this, "Cannot replace existing blob at address " + addr + " with " + diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java index 28b5aee7a8..291dc5120e 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java @@ -671,8 +671,8 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal { } /** - * Copy packing and alignment settings from specified composite without - * repacking or notification. + * Set packing and alignment settings. Record is modified but it is not written to the + * database and no repacking or notification is performed. * @param composite instance whose packing and alignment are to be copied * @throws IOException if database IO error occured */ @@ -681,7 +681,6 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal { composite.getStoredMinimumAlignment()); record.setIntValue(CompositeDBAdapter.COMPOSITE_PACKING_COL, composite.getStoredPackingValue()); - compositeAdapter.updateRecord(record, true); } @Override diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeComponentDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeComponentDB.java index e9ae27d789..67d3dc89f5 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeComponentDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeComponentDB.java @@ -220,15 +220,7 @@ class DataTypeComponentDB implements InternalDataTypeComponent { @Override public void setFieldName(String name) throws DuplicateNameException { if (record != null) { - if (name != null) { - name = name.trim(); - if (name.length() == 0 || name.equals(getDefaultFieldName())) { - name = null; - } - else { - checkDuplicateName(name); - } - } + name = checkFieldName(name); record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, name); updateRecord(true); } @@ -246,6 +238,19 @@ class DataTypeComponentDB implements InternalDataTypeComponent { } } + private String checkFieldName(String name) throws DuplicateNameException { + if (name != null) { + name = name.trim(); + if (name.length() == 0 || name.equals(getDefaultFieldName())) { + name = null; + } + else { + checkDuplicateName(name); + } + } + return name; + } + @Override public int hashCode() { // It is not expected that these objects ever be put in a hash map @@ -392,6 +397,24 @@ class DataTypeComponentDB implements InternalDataTypeComponent { return record; } + /** + * Perform special-case component update that does not result in size or alignment changes. + * @param name new component name + * @param dt new resolved datatype + * @param cmt new comment + */ + void update(String name, DataType dt, String cmt) { + if (record != null) { + // TODO: Need to check field name and throw DuplicateNameException + // name = checkFieldName(name); + record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, name); + record.setLongValue(ComponentDBAdapter.COMPONENT_DT_ID_COL, + dataMgr.getResolvedID(dt)); + record.setString(ComponentDBAdapter.COMPONENT_COMMENT_COL, cmt); + updateRecord(false); + } + } + @Override public void setDataType(DataType newDt) { // intended for internal use only - note exsiting settings should be preserved diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java index 5acef9ca97..6fbbece0c1 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java @@ -179,7 +179,7 @@ class StructureDB extends CompositeDB implements StructureInternal { throw new IllegalArgumentException(e.getMessage(), e); } } - + private DataTypeComponent doAdd(DataType dataType, int length, String name, String comment, boolean validatePackAndNotify) throws DataTypeDependencyException, IllegalArgumentException { @@ -219,7 +219,9 @@ class StructureDB extends CompositeDB implements StructureInternal { structLength += structureGrowth; if (validatePackAndNotify) { - repack(false, false); // may not recognize length change + if (isPackingEnabled()) { + repack(false, false); // may not recognize length change + } record.setIntValue(CompositeDBAdapter.COMPOSITE_NUM_COMPONENTS_COL, numComponents); record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength); @@ -1301,6 +1303,53 @@ class StructureDB extends CompositeDB implements StructureInternal { return null; } + /** + * Perform component replacement(s) + * @param replacedComponents ordered list of components to be replaced + * @param offset offset of component replacement + * @param dataType datatype of component replacement + * @param length length of component replacement + * @param componentName name of component replacement (may be null) + * @param comment comment for component replacement (may be null) + * @return new/updated component (may be null if replacement is not a defined component) + * @throws IOException if an IO error occurs + */ + private DataTypeComponent doComponentReplacement( + LinkedList replacedComponents, int offset, DataType dataType, + int length, String componentName, String comment) + throws IOException { + + // Attempt quick update of a single defined component if possible. + // A quick update requires that component characteristics including length, offset, + // and alignment (if packed) not change. All other situations generally require a + // repack. + DataTypeComponentDB oldComponent = replacedComponents.get(0); + DataType oldDt = oldComponent.getDataType(); + if (replacedComponents.size() == 1 && + oldDt != DEFAULT && + dataType != DEFAULT && + length == oldComponent.getLength() && + offset == oldComponent.getOffset() && + (!isPackingEnabled() || dataType.getAlignment() == oldDt.getAlignment())) { + + oldComponent.update(componentName, dataType, comment); + + setLastChangeTime(System.currentTimeMillis()); + dataMgr.dataTypeChanged(this, false); + return oldComponent; + } + + DataTypeComponent replaceComponent = replaceComponents(replacedComponents, dataType, + offset, length, componentName, comment); + + repack(false, false); + record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength); + compositeAdapter.updateRecord(record, true); + notifySizeChanged(false); + + return replaceComponent; + } + @Override public final DataTypeComponent replace(int ordinal, DataType dataType, int length) throws IllegalArgumentException { @@ -1308,9 +1357,8 @@ class StructureDB extends CompositeDB implements StructureInternal { } @Override - public DataTypeComponent replace(int ordinal, DataType dataType, int length, - String componentName, - String comment) { + public DataTypeComponent replace(int ordinal, DataType dataType, int length, + String componentName, String comment) { lock.acquire(); try { checkDeleted(); @@ -1399,14 +1447,8 @@ class StructureDB extends CompositeDB implements StructureInternal { replacedComponents.add(origDtc); } - DataTypeComponent replaceComponent = - replaceComponents(replacedComponents, dataType, offset, length, componentName, - comment); - - repack(false, false); - record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength); - compositeAdapter.updateRecord(record, true); - notifySizeChanged(false); + DataTypeComponent replaceComponent = doComponentReplacement(replacedComponents, offset, + dataType, length, componentName, comment); return replaceComponent != null ? replaceComponent : getComponent(ordinal); } @@ -1423,8 +1465,8 @@ class StructureDB extends CompositeDB implements StructureInternal { } @Override - public DataTypeComponent replaceAtOffset(int offset, DataType dataType, int length, String name, - String comment) throws IllegalArgumentException { + public DataTypeComponent replaceAtOffset(int offset, DataType dataType, int length, + String componentName, String comment) throws IllegalArgumentException { if (offset < 0) { throw new IllegalArgumentException("Offset cannot be negative."); } @@ -1452,12 +1494,12 @@ class StructureDB extends CompositeDB implements StructureInternal { // defined component found - advance to last one containing offset index = advanceToLastComponentContainingOffset(index, offset); origDtc = components.get(index); - + // case 1: only defined component(s) at offset are zero-length if (origDtc.getLength() == 0) { if (isPackingEnabled()) { // if packed: insert after zero-length component - return insert(index + 1, dataType, length, name, comment); + return insert(index + 1, dataType, length, componentName, comment); } // if non-packed: replace undefined component which immediately follows the // zero-length component @@ -1490,7 +1532,7 @@ class StructureDB extends CompositeDB implements StructureInternal { if (isPackingEnabled()) { // case 4: if replacing padding for packed structure perform insert at correct // ordinal - return insert(index, dataType, length, name, comment); + return insert(index, dataType, length, componentName, comment); } // case 5: replace undefined component at offset - compute undefined component to @@ -1509,14 +1551,9 @@ class StructureDB extends CompositeDB implements StructureInternal { } length = getPreferredComponentLength(dataType, length); - - DataTypeComponent replaceComponent = - replaceComponents(replacedComponents, dataType, offset, length, name, comment); - - repack(false, false); - record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength); - compositeAdapter.updateRecord(record, true); - notifySizeChanged(false); + + DataTypeComponent replaceComponent = doComponentReplacement(replacedComponents, offset, + dataType, length, componentName, comment); return replaceComponent != null ? replaceComponent : getComponentContaining(offset); } @@ -1579,6 +1616,9 @@ class StructureDB extends CompositeDB implements StructureInternal { void doReplaceWith(StructureInternal struct, boolean notify) throws DataTypeDependencyException, IOException { + int oldAlignment = getAlignment(); + int oldLength = structLength; + // pre-resolved component types to catch dependency issues early DataTypeComponent[] otherComponents = struct.getDefinedComponents(); DataType[] resolvedDts = new DataType[otherComponents.length]; @@ -1598,25 +1638,39 @@ class StructureDB extends CompositeDB implements StructureInternal { structAlignment = -1; computedAlignment = -1; - doSetPackingAndAlignment(struct); // updates timestamp + doSetPackingAndAlignment(struct); - if (struct.isPackingEnabled()) { - doReplaceWithPacked(struct, resolvedDts); + if (struct.getDataTypeManager() == dataMgr) { + // assume layout is good and can just be copied without packing + // NOTE: this optimization has been added to StructureDB only + doCopy(struct, otherComponents, resolvedDts); } else { - doReplaceWithNonPacked(struct, resolvedDts); + if (struct.isPackingEnabled()) { + doReplaceWithPacked(struct, resolvedDts); + } + else { + doReplaceWithNonPacked(struct, resolvedDts); + } + repack(false, false); } - repack(false, false); - // must force record update record.setIntValue(CompositeDBAdapter.COMPOSITE_NUM_COMPONENTS_COL, numComponents); record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength); record.setIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL, structAlignment); - compositeAdapter.updateRecord(record, notify); + compositeAdapter.updateRecord(record, true); // updates timestamp if (notify) { - notifySizeChanged(false); + if (structLength != oldLength) { + notifySizeChanged(false); + } + else if (structAlignment != oldAlignment) { + notifyAlignmentChanged(false); + } + else { + dataMgr.dataTypeChanged(this, false); + } } if (pointerPostResolveRequired) { @@ -1682,7 +1736,28 @@ class StructureDB extends CompositeDB implements StructureInternal { new DataTypeComponentDB(dataMgr, componentAdapter, this, rec); components.add(newDtc); } - repack(false, false); + } + + private void doCopy(Structure struct, DataTypeComponent[] definedComponents, + DataType[] resolvedDts) throws IOException { + + // simple replication of struct + structLength = struct.isZeroLength() ? 0 : struct.getLength(); + numComponents = struct.getNumComponents(); + structAlignment = struct.getAlignment(); + + DataTypeComponent[] otherComponents = struct.getDefinedComponents(); + for (int i = 0; i < otherComponents.length; i++) { + DataTypeComponent dtc = otherComponents[i]; + DataType dt = resolvedDts[i]; // ancestry check already performed by caller + DBRecord rec = + componentAdapter.createRecord(dataMgr.getResolvedID(dt), key, dtc.getLength(), + dtc.getOrdinal(), dtc.getOffset(), dtc.getFieldName(), dtc.getComment()); + dt.addParent(this); + DataTypeComponentDB newDtc = + new DataTypeComponentDB(dataMgr, componentAdapter, this, rec); + components.add(newDtc); + } } @Override @@ -1950,6 +2025,9 @@ class StructureDB extends CompositeDB implements StructureInternal { } private void shiftOffsets(int definedComponentIndex, int deltaOrdinal, int deltaOffset) { + if (deltaOffset == 0 && deltaOrdinal == 0) { + return; + } for (int i = definedComponentIndex; i < components.size(); i++) { DataTypeComponentDB dtc = components.get(i); shiftOffset(dtc, deltaOrdinal, deltaOffset); diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/UnionDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/UnionDB.java index 0334ac0ba2..dd8869a237 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/UnionDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/UnionDB.java @@ -344,7 +344,7 @@ class UnionDB extends CompositeDB implements UnionInternal { doAdd(resolvedDts[i], dtc.getLength(), dtc.getFieldName(), dtc.getComment(), false); } - repack(false, false); + repack(false, false); // updates timestamp if (notify) { notifySizeChanged(false); // assume size and/or alignment changed diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponentImpl.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponentImpl.java index afdc2ee086..d979629820 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponentImpl.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponentImpl.java @@ -128,19 +128,7 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali @Override public void setFieldName(String name) throws DuplicateNameException { - if (name != null) { - name = name.trim(); - if (name.length() == 0 || name.equals(getDefaultFieldName())) { - name = null; - } - else { - if (name.equals(this.fieldName)) { - return; - } - checkDuplicateName(name); - } - } - this.fieldName = name; + this.fieldName = checkFieldName(name); } private void checkDuplicateName(String name) throws DuplicateNameException { @@ -155,6 +143,19 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali } } + private String checkFieldName(String name) throws DuplicateNameException { + if (name != null) { + name = name.trim(); + if (name.length() == 0 || name.equals(getDefaultFieldName())) { + name = null; + } + else { + checkDuplicateName(name); + } + } + return name; + } + public static void checkDefaultFieldName(String fieldName) throws DuplicateNameException { if (fieldName.startsWith(DataTypeComponent.DEFAULT_FIELD_NAME_PREFIX)) { String subname = @@ -186,6 +187,20 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali return parent; } + /** + * Perform special-case component update that does not result in size or alignment changes. + * @param name new component name + * @param dt new resolved datatype + * @param cmt new comment + */ + void update(String name, DataType dt, String cmt) { + // TODO: Need to check field name and throw DuplicateNameException + // this.fieldName = = checkFieldName(name); + this.fieldName = name; + this.dataType = dt; + this.comment = cmt; + } + @Override public void update(int ordinal, int offset, int length) { this.ordinal = ordinal; diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StructureDataType.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StructureDataType.java index a708df4e27..106ca77109 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StructureDataType.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StructureDataType.java @@ -44,7 +44,8 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur /** * Construct a new structure with the given name and length. The root category will be used. - * + * NOTE: A constructor form which accepts a {@link DataTypeManager} should be used when possible + * since there may be performance benefits during datatype resolution. * @param name the name of the new structure * @param length the initial size of the structure in bytes. If 0 is specified the structure * will report its length as 1 and {@link #isNotYetDefined()} will return true. @@ -69,7 +70,8 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur /** * Construct a new structure with the given name and length within the specified categry path. - * + * NOTE: A constructor form which accepts a {@link DataTypeManager} should be used when possible + * since there may be performance benefits during datatype resolution. * @param path the category path indicating where this data type is located. * @param name the name of the new structure * @param length the initial size of the structure in bytes. If 0 is specified the structure @@ -417,6 +419,9 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur } private void shiftOffsets(int index, int deltaOrdinal, int deltaOffset) { + if (deltaOffset == 0 && deltaOrdinal == 0) { + return; + } for (int i = index; i < components.size(); i++) { DataTypeComponentImpl dtc = components.get(i); shiftOffset(dtc, deltaOrdinal, deltaOffset); @@ -581,14 +586,9 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur dtc = new DataTypeComponentImpl(DataType.DEFAULT, this, 1, numComponents, structLength); } else { - - int offset = structLength; - int ordinal = numComponents; - int componentLength = getPreferredComponentLength(dataType, length); - - dtc = new DataTypeComponentImpl(dataType, this, componentLength, ordinal, offset, - componentName, comment); + dtc = new DataTypeComponentImpl(dataType, this, componentLength, numComponents, + structLength, componentName, comment); dataType.addParent(this); components.add(dtc); } @@ -602,7 +602,9 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur structLength += structureGrowth; if (packAndNotify) { - repack(false); + if (isPackingEnabled()) { + repack(false); + } notifySizeChanged(); } return dtc; @@ -1216,7 +1218,6 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur components.add(new DataTypeComponentImpl(dt, this, length, dtc.getOrdinal(), dtc.getOffset(), dtc.getFieldName(), dtc.getComment())); } - repack(false); } @Override @@ -1362,6 +1363,46 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur return comps; } + /** + * Perform component replacement(s) + * @param replacedComponents ordered list of components to be replaced + * @param offset offset of component replacement + * @param dataType datatype of component replacement + * @param length length of component replacement + * @param componentName name of component replacement (may be null) + * @param comment comment for component replacement (may be null) + * @return new/updated component (may be null if replacement is not a defined component) + */ + private DataTypeComponent doComponentReplacement( + LinkedList replacedComponents, int offset, DataType dataType, + int length, String componentName, String comment) { + + // Attempt quick update of a single defined component if possible. + // A quick update requires that component characteristics including length, offset, + // and alignment (if packed) not change. All other situations generally require a + // repack. + DataTypeComponentImpl oldComponent = replacedComponents.get(0); + DataType oldDt = oldComponent.getDataType(); + if (replacedComponents.size() == 1 && + oldDt != DEFAULT && + dataType != DEFAULT && + length == oldComponent.getLength() && + offset == oldComponent.getOffset() && + (!isPackingEnabled() || dataType.getAlignment() == oldDt.getAlignment())) { + + oldComponent.update(componentName, dataType, comment); + return oldComponent; + } + + DataTypeComponent replaceComponent = replaceComponents(replacedComponents, dataType, + offset, length, componentName, comment); + + repack(false); + notifySizeChanged(); + + return replaceComponent; + } + @Override public final DataTypeComponent replace(int index, DataType dataType, int length) { return replace(index, dataType, length, null, null); @@ -1453,18 +1494,15 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur replacedComponents.add(origDtc); } - DataTypeComponent replaceComponent = - replaceComponents(replacedComponents, dataType, offset, length, componentName, comment); - - repack(false); - notifySizeChanged(); + DataTypeComponent replaceComponent = doComponentReplacement(replacedComponents, offset, + dataType, length, componentName, comment); return replaceComponent != null ? replaceComponent : getComponent(ordinal); } @Override public DataTypeComponent replaceAtOffset(int offset, DataType dataType, int length, - String name, String comment) throws IllegalArgumentException { + String componentName, String comment) throws IllegalArgumentException { if (offset < 0) { throw new IllegalArgumentException("Offset cannot be negative."); } @@ -1492,7 +1530,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur if (origDtc.getLength() == 0) { if (isPackingEnabled()) { // if packed: insert after zero-length component - return insert(index + 1, dataType, length, name, comment); + return insert(index + 1, dataType, length, componentName, comment); } // if non-packed: replace undefined component which immediately follows the zero-length component replacedComponents.add(new DataTypeComponentImpl(DataType.DEFAULT, this, 1, @@ -1522,7 +1560,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur if (isPackingEnabled()) { // case 4: if replacing padding for packed struction perform insert at correct ordinal - return insert(index, dataType, length, name, comment); + return insert(index, dataType, length, componentName, comment); } // case 5: replace undefined component at offset - compute undefined component to be replaced @@ -1541,11 +1579,8 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur length = getPreferredComponentLength(dataType, length); - DataTypeComponent replaceComponent = - replaceComponents(replacedComponents, dataType, offset, length, name, comment); - - repack(false); - notifySizeChanged(); + DataTypeComponent replaceComponent = doComponentReplacement(replacedComponents, offset, + dataType, length, componentName, comment); return replaceComponent != null ? replaceComponent : getComponentContaining(offset); } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/UnionDataType.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/UnionDataType.java index 7ac6b3aa4f..a4e14ad8a2 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/UnionDataType.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/UnionDataType.java @@ -37,6 +37,8 @@ public class UnionDataType extends CompositeDataTypeImpl implements UnionInterna * Construct a new empty union with the given name within the * specified categry path. An empty union will report its length as 1 and * {@link #isNotYetDefined()} will return true. + * NOTE: A constructor form which accepts a {@link DataTypeManager} should be used when possible + * since there may be performance benefits during datatype resolution. * @param path the category path indicating where this data type is located. * @param name the name of the new union */ @@ -82,7 +84,9 @@ public class UnionDataType extends CompositeDataTypeImpl implements UnionInterna } /** - * Construct a new UnionDataType + * Construct a new UnionDataType. + * NOTE: A constructor form which accepts a {@link DataTypeManager} should be used when possible + * since there may be performance benefits during datatype resolution. * @param name the name of this dataType */ public UnionDataType(String name) { From 8197183de00c663642d9386b9a2b634c0f9a70dd Mon Sep 17 00:00:00 2001 From: emteere <47253321+emteere@users.noreply.github.com> Date: Wed, 9 Nov 2022 13:08:38 -0500 Subject: [PATCH 3/3] GP-2807 Fix potential error creating function/data on existing function/data, minor datatype name fix --- .../format/pe/cli/blobs/CliAbstractSig.java | 11 ++- .../pe/cli/tables/CliTableMethodDef.java | 67 ++++++++++++++----- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliAbstractSig.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliAbstractSig.java index d4f194ea85..9897ffc86a 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliAbstractSig.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/blobs/CliAbstractSig.java @@ -529,10 +529,14 @@ public abstract class CliAbstractSig extends CliBlob implements CliRepresentable private int typeSizeBytes; private int genArgCount; private int countSizeBytes; + private long dataOffset; private List argTypes = new ArrayList<>(); public CliTypeGenericInst(BinaryReader reader, CliElementType typeCode) throws IOException { super(typeCode); + + dataOffset = reader.getPointerIndex(); + firstType = CliElementType.fromInt(reader.readNextByte()); // Should be Class or ValueType long origIndex = reader.getPointerIndex(); encodedType = decodeCompressedUnsignedInt(reader); @@ -600,7 +604,7 @@ public abstract class CliAbstractSig extends CliBlob implements CliRepresentable @Override public DataType getDefinitionDataType() { StructureDataType struct = new StructureDataType(new CategoryPath(PATH), - "GenericInstType" + argTypes.toString(), 0); + "GenericInstType" + "_" + dataOffset, 0); // TODO: the toString() is included in the above line so GenericInst types can contain other GenericInst's, otherwise this is prohibited by StructureDataType struct.add(CliTypeCodeDataType.dataType, "GenericInst", "GenericInst"); struct.add(CliTypeCodeDataType.dataType, "ClassOrValueType", "Class or ValueType"); @@ -696,11 +700,14 @@ public abstract class CliAbstractSig extends CliBlob implements CliRepresentable public class CliTypeSzArray extends CliSigType { private List customMods = new ArrayList<>(); private CliSigType type; + private long dataOffset; public CliTypeSzArray(BinaryReader reader, CliElementType typeCode) throws IOException, InvalidInputException { super(typeCode); + dataOffset = reader.getPointerIndex(); + while (CliCustomMod.isCustomMod(reader)) { customMods.add(new CliCustomMod(reader)); } @@ -735,7 +742,7 @@ public abstract class CliAbstractSig extends CliBlob implements CliRepresentable @Override public DataType getDefinitionDataType() { - StructureDataType struct = new StructureDataType(new CategoryPath(PATH), "SzArray", 0); + StructureDataType struct = new StructureDataType(new CategoryPath(PATH), "SzArray" + "_" + dataOffset, 0); struct.add(CliTypeCodeDataType.dataType, "TypeCode", "SzArray"); for (CliCustomMod mod : customMods) { struct.add(mod.getDefinitionDataType()); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/tables/CliTableMethodDef.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/tables/CliTableMethodDef.java index 391f644a01..16fc02f907 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/tables/CliTableMethodDef.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/cli/tables/CliTableMethodDef.java @@ -433,25 +433,20 @@ public class CliTableMethodDef extends CliAbstractTable { } } + FunctionManager funcMgr = program.getFunctionManager(); + try { - Function newFunc = program.getFunctionManager() - .createFunction(funcName, startAddr, funcAddrSet, SourceType.ANALYSIS); - newFunc.setReturnType(returnType, SourceType.ANALYSIS); - newFunc.updateFunction(null, null, FunctionUpdateType.DYNAMIC_STORAGE_ALL_PARAMS, + // Create the function, if already created, update the existing function + Function func = funcMgr.getFunctionAt(startAddr); + if (func == null) { + func = funcMgr + .createFunction(funcName, startAddr, funcAddrSet, SourceType.ANALYSIS); + } + func.setReturnType(returnType, SourceType.ANALYSIS); + func.updateFunction(null, null, FunctionUpdateType.DYNAMIC_STORAGE_ALL_PARAMS, true, SourceType.ANALYSIS, parameters); - // If Code here is not of the correct type, create a data element to stop disassembly, and comment - PrototypeModel cliCallingConvention = program.getLanguage() - .getDefaultCompilerSpec() - .getCallingConvention(CLITABLEMETHODDEF_CLRCALL_CONVENTION); - - if ((cliCallingConvention == null && !methodRow.isNative()) || - (cliCallingConvention != null && methodRow.isNative())) { - Data data = program.getListing() - .createData(startAddr, new ArrayDataType(BYTE, (int) endAddr.subtract(startAddr) + 1, 1)); - data.setComment(CodeUnit.PRE_COMMENT, - (methodRow.isManaged() ? ".NET CLR Managed Code" : "Native Code")); - } + markToPreventIncorrectProcessorDisassembly(program, methodRow, startAddr, endAddr); } catch (CodeUnitInsertionException e) { // Ignore, something there already @@ -468,8 +463,8 @@ public class CliTableMethodDef extends CliAbstractTable { String err = "Error processing function \" (\" + methodRowIndex + \")" + funcName + "\": Overlapping function (" + startAddr + ", " + endAddr + ": "; - Function existingFuncA = program.getFunctionManager().getFunctionAt(startAddr); - Function existingFuncB = program.getFunctionManager().getFunctionAt(endAddr); + Function existingFuncA = funcMgr.getFunctionContaining(startAddr); + Function existingFuncB = funcMgr.getFunctionContaining(endAddr); if (existingFuncA != null && existingFuncB == null) { err = err + existingFuncA.getName(); @@ -497,6 +492,42 @@ public class CliTableMethodDef extends CliAbstractTable { } } + /** + * + * For .Net code in programs loaded as x86, protect CLI code from disassembly. + * For x86 code in programs loaded as CLI, protect x86 code from disassembly. + * + * @param program program either loaded as x86 or CLI + * @param methodRow CliMethodTable row + * @param startAddr start address of the function + * @param endAddr end address of the function + * @throws CodeUnitInsertionException couldn't create dagta. + */ + private void markToPreventIncorrectProcessorDisassembly(Program program, CliMethodDefRow methodRow, + Address startAddr, Address endAddr) throws CodeUnitInsertionException { + + + PrototypeModel cliCallingConvention = program.getLanguage() + .getDefaultCompilerSpec() + .getCallingConvention(CLITABLEMETHODDEF_CLRCALL_CONVENTION); + + // If Code here is not of the correct type, create a data array of bytes to stop disassembly + if ((cliCallingConvention == null && !methodRow.isNative()) || + (cliCallingConvention != null && methodRow.isNative())) { + Listing listing = program.getListing(); + Data data = listing.getDefinedDataAt(startAddr); + if (data == null) { + int codeLength = (int) endAddr.subtract(startAddr) + 1; + ArrayDataType codeDT = new ArrayDataType(BYTE, codeLength, 1); + data = listing.createData(startAddr, codeDT); + + // comment the type of code that should appear here + data.setComment(CodeUnit.PRE_COMMENT, + (methodRow.isManaged() ? ".NET CLR Managed Code" : "Native Code")); + } + } + } + @Override public StructureDataType getRowDataType() { StructureDataType rowDt = new StructureDataType(new CategoryPath(PATH), "MethodDef Row", 0);