From 002119277f2cf08b63e6ea1c73606a3f7b924cf4 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Thu, 14 Oct 2021 16:24:39 -0400 Subject: [PATCH] GP-1398 - Fixed the Function Editor's Storage Address Editor dialog to ensure that the Cancel button will not allow data type changes to be passed through to the primary editor. Closes #3490 --- .../function/editor/FunctionEditorModel.java | 17 +- .../function/editor/FunctionVariableData.java | 2 +- .../function/editor/ParameterTableModel.java | 27 +-- .../editor/StorageAddressEditorDialog.java | 164 +++++++++--------- .../java/docking/DialogComponentProvider.java | 17 +- 5 files changed, 120 insertions(+), 107 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionEditorModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionEditorModel.java index 4bb56cd1cd..e8506dd903 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionEditorModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionEditorModel.java @@ -368,7 +368,7 @@ public class FunctionEditorModel { private boolean isValidParamType(ParamInfo param) { DataType dataType = param.getDataType(); - if (dataType.isEquivalent(DataType.VOID)) { + if (dataType.isEquivalent(VoidDataType.dataType)) { statusText = "\"void\" is not allowed as a parameter datatype."; return false; } @@ -440,8 +440,8 @@ public class FunctionEditorModel { return returnInfo.getFormalDataType(); } - public void setFormalReturnType(DataType formalReturnType) { - setParameterFormalDataType(returnInfo, formalReturnType); + public boolean setFormalReturnType(DataType formalReturnType) { + return setParameterFormalDataType(returnInfo, formalReturnType); } public String getStatusText() { @@ -775,18 +775,20 @@ public class FunctionEditorModel { notifyDataChanged(); } - public void setParameterFormalDataType(ParamInfo param, DataType formalDataType) { + public boolean setParameterFormalDataType(ParamInfo param, DataType formalDataType) { boolean isReturn = (param.getOrdinal() == Parameter.RETURN_ORIDINAL); try { formalDataType = VariableUtilities.checkDataType(formalDataType, isReturn, 0, program); } catch (InvalidInputException e) { Msg.showError(this, null, "Invalid Data Type", e.getMessage()); - return; + return false; } + if (formalDataType.equals(param.getFormalDataType())) { - return; + return true; } + param.setFormalDataType(formalDataType.clone(program.getDataTypeManager())); if (allowCustomStorage) { if (isReturn && (formalDataType instanceof VoidDataType)) { @@ -808,6 +810,7 @@ public class FunctionEditorModel { updateParameterAndReturnStorage(); } notifyDataChanged(); + return true; } private void adjustStorageSize(ParamInfo param, VariableStorage curStorage, int newSize) { @@ -1180,7 +1183,7 @@ public class FunctionEditorModel { /** * Sets the change state of the model. Normally, the model sets the modelChanged variable to true * every time something is changed. This provides a way to for applications to make some initial changes - * but make the dialog think that nothing has changed. + * but make the dialog think that nothing has changed. * @param b the new changeState for this model */ public void setModelChanged(boolean b) { diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionVariableData.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionVariableData.java index 80e849a1a7..900ba67e98 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionVariableData.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionVariableData.java @@ -25,7 +25,7 @@ interface FunctionVariableData { public void setName(String name); - public void setFormalDataType(DataType dataType); + public boolean setFormalDataType(DataType dataType); public VariableStorage getStorage(); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterTableModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterTableModel.java index 52807d91db..76e03be492 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterTableModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterTableModel.java @@ -109,6 +109,14 @@ class ParameterTableModel extends AbstractGTableModel { fireTableDataChanged(); } + public void setAllowStorageEditing(boolean canCustomizeStorage) { + this.canCustomizeStorage = canCustomizeStorage; + } + +//================================================================================================== +// Inner Classes +//================================================================================================== + private abstract class ParamCol { private String name; private boolean isEditable; @@ -229,11 +237,7 @@ class ParameterTableModel extends AbstractGTableModel { } } - public void setAllowStorageEditing(boolean canCustomizeStorage) { - this.canCustomizeStorage = canCustomizeStorage; - } - - class ParameterRowData implements FunctionVariableData { + private class ParameterRowData implements FunctionVariableData { private ParamInfo param; ParameterRowData(ParamInfo paramInfo) { @@ -265,8 +269,8 @@ class ParameterTableModel extends AbstractGTableModel { } @Override - public void setFormalDataType(DataType dataType) { - functionModel.setParameterFormalDataType(param, dataType); + public boolean setFormalDataType(DataType dataType) { + return functionModel.setParameterFormalDataType(param, dataType); } @Override @@ -280,7 +284,7 @@ class ParameterTableModel extends AbstractGTableModel { } } - class ReturnRowData implements FunctionVariableData { + private class ReturnRowData implements FunctionVariableData { private DataType formalDataType; private VariableStorage storage; @@ -310,8 +314,8 @@ class ParameterTableModel extends AbstractGTableModel { } @Override - public void setFormalDataType(DataType dataType) { - functionModel.setFormalReturnType(dataType); + public boolean setFormalDataType(DataType dataType) { + return functionModel.setFormalReturnType(dataType); } @Override @@ -321,8 +325,7 @@ class ParameterTableModel extends AbstractGTableModel { @Override public void setName(String name) { - // TODO Auto-generated method stub - + // no name for return type } } } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressEditorDialog.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressEditorDialog.java index 82a13bc990..bc49991101 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressEditorDialog.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressEditorDialog.java @@ -19,6 +19,7 @@ import java.awt.*; import java.awt.event.*; import java.util.Arrays; import java.util.List; +import java.util.Objects; import javax.swing.*; import javax.swing.event.*; @@ -41,9 +42,7 @@ import ghidra.util.layout.VerticalLayout; public class StorageAddressEditorDialog extends DialogComponentProvider implements ModelChangeListener { - private FunctionVariableData variableData; - private StorageAddressModel model; - private VarnodeTableModel varnodeTableModel; + private ParameterDataTypeCellEditor dataTypeEditor; private GTable varnodeTable; private ListSelectionListener selectionListener; @@ -52,23 +51,34 @@ public class StorageAddressEditorDialog extends DialogComponentProvider private JButton removeButton; private JButton upButton; private JButton downButton; - private int size; private JLabel currentSizeLabel; + + private FunctionVariableData variableData; + private StorageAddressModel model; + private VarnodeTableModel varnodeTableModel; + private int size; private boolean cancelled = true; + private boolean adjustingDataType = false; + + private DataType previousDataType; + private DataType currentDataType; /** * Constructor - * @param program - * @param service - * @param storage - * @param variableData + * @param program the program + * @param service the data type manager service + * @param storage the variable storage + * @param variableData the variable data */ public StorageAddressEditorDialog(Program program, DataTypeManagerService service, VariableStorage storage, FunctionVariableData variableData) { super("Storage Address Editor"); this.variableData = variableData; model = new StorageAddressModel(program, storage, this); - setDataType(variableData.getFormalDataType()); + + currentDataType = variableData.getFormalDataType(); + previousDataType = currentDataType; + setDataType(currentDataType); setHelpLocation(new HelpLocation("FunctionPlugin", "Edit_Parameter_Storage")); addWorkPanel(buildMainPanel(service)); addOKButton(); @@ -78,50 +88,14 @@ public class StorageAddressEditorDialog extends DialogComponentProvider /** * Read-only use constructor for Help screenshot - * @param program - * @param service + * @param program the program + * @param service the data type manager service * @param var function parameter to be displayed in editor dialog * @param ordinal parameter ordinal (-1 for return) */ public StorageAddressEditorDialog(Program program, DataTypeManagerService service, final Variable var, final int ordinal) { - this(program, service, var.getVariableStorage(), new FunctionVariableData() { - - @Override - public void setStorage(VariableStorage storage) { - // unsupported - } - - @Override - public void setName(String name) { - // unsupported - } - - @Override - public void setFormalDataType(DataType dataType) { - // unsupported - } - - @Override - public VariableStorage getStorage() { - return var.getVariableStorage(); - } - - @Override - public String getName() { - return var.getName(); - } - - @Override - public Integer getIndex() { - return ordinal; - } - - @Override - public DataType getFormalDataType() { - return var.getDataType(); - } - }); + this(program, service, var.getVariableStorage(), new ReadOnlyVariableData(ordinal, var)); } @Override @@ -131,6 +105,15 @@ public class StorageAddressEditorDialog extends DialogComponentProvider return; } } + + if (!Objects.equals(previousDataType, currentDataType)) { + boolean isValid = variableData.setFormalDataType(currentDataType); + if (!isValid) { + setStatusText("Invalid data type"); + return; + } + } + cancelled = false; close(); } @@ -147,13 +130,12 @@ public class StorageAddressEditorDialog extends DialogComponentProvider } private void setDataType(DataType dt) { - DataType dataType = variableData.getFormalDataType(); - size = dataType.getLength(); - boolean unconstrained = - (dataType instanceof AbstractFloatDataType) || Undefined.isUndefined(dataType); + currentDataType = dt; + size = dt.getLength(); + boolean unconstrained = (dt instanceof AbstractFloatDataType) || Undefined.isUndefined(dt); model.setRequiredSize(size, unconstrained); if (sizeLabel != null) { - sizeLabel.setText("" + size); + sizeLabel.setText(Integer.toString(size)); dataChanged(); } } @@ -171,7 +153,6 @@ public class StorageAddressEditorDialog extends DialogComponentProvider @Override public void editingStopped(ChangeEvent e) { DataType dt = (DataType) dataTypeEditor.getCellEditorValue(); - variableData.setFormalDataType(dt); setDataType(dt); } @@ -213,7 +194,7 @@ public class StorageAddressEditorDialog extends DialogComponentProvider panel.add(dataTypeEditComponent); panel.add(new GLabel("Datatype Size: ")); - sizeLabel = new GDLabel("" + size); + sizeLabel = new GDLabel(Integer.toString(size)); panel.add(sizeLabel); panel.add(new GLabel("Allocated Size:")); currentSizeLabel = new GDLabel(""); @@ -312,7 +293,6 @@ public class StorageAddressEditorDialog extends DialogComponentProvider private void updateTableSelection() { int[] selectedRows = model.getSelectedVarnodeRows(); - if (!Arrays.equals(selectedRows, varnodeTable.getSelectedRows())) { varnodeTable.clearSelection(); for (int i : selectedRows) { @@ -342,14 +322,11 @@ public class StorageAddressEditorDialog extends DialogComponentProvider } private void updateCurrentSize() { - currentSizeLabel.setText("" + model.getCurrentSize()); + currentSizeLabel.setText(Integer.toString(model.getCurrentSize())); } - private boolean adjustingDataType = false; - private void updateDataType() { - // If storage size has changed with an undefined datatype, - // alter the size of the undefined type + if (adjustingDataType) { return; } @@ -358,7 +335,7 @@ public class StorageAddressEditorDialog extends DialogComponentProvider int currentSize = model.getCurrentSize(); if (currentSize > 0 && Undefined.isUndefined(variableData.getFormalDataType())) { DataType adjustedUndefinedtype = Undefined.getUndefinedDataType(currentSize); - variableData.setFormalDataType(adjustedUndefinedtype); + currentDataType = adjustedUndefinedtype; dataTypeEditor.getEditor().setCellEditorValue(adjustedUndefinedtype); setDataType(adjustedUndefinedtype); } @@ -368,22 +345,6 @@ public class StorageAddressEditorDialog extends DialogComponentProvider } } -// public static void main(String[] args) throws Exception { -//// DockingWindowsLookAndFeelUtils.setLookAndFeel("Metal"); -// ProgramBuilder builder = new ProgramBuilder(); -// builder.addMemory("1000", 1000); -// Function fun = builder.addFunction("foo", "1000", 20, new VoidDataType()); -// -// Program program = builder.getProgram(); -// AddressSpace stackSpace = program.getAddressFactory().getStackSpace(); -// Address address = stackSpace.getAddress(4); -// VariableStorage storage = new VariableStorage(program, address, 4); -// -// DockingWindowManager dwm = new DockingWindowManager("Test", null, null); -// dwm.showDialog(new StorageAddressEditorDialog(program, storage, 8)); -// System.exit(0); -// } - @Override public void tableRowsChanged() { TableCellEditor cellEditor = varnodeTable.getCellEditor(); @@ -397,4 +358,51 @@ public class StorageAddressEditorDialog extends DialogComponentProvider public boolean wasCancelled() { return cancelled; } + + private static class ReadOnlyVariableData implements FunctionVariableData { + + private int ordinal; + private Variable variable; + + private ReadOnlyVariableData(int ordinal, Variable variable) { + this.ordinal = ordinal; + this.variable = variable; + } + + @Override + public void setStorage(VariableStorage storage) { + // unsupported + } + + @Override + public boolean setFormalDataType(DataType dataType) { + return false; + } + + @Override + public void setName(String name) { + // unsupported + } + + @Override + public VariableStorage getStorage() { + return variable.getVariableStorage(); + } + + @Override + public String getName() { + return variable.getName(); + } + + @Override + public Integer getIndex() { + return ordinal; + } + + @Override + public DataType getFormalDataType() { + return variable.getDataType(); + } + } + } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/DialogComponentProvider.java b/Ghidra/Framework/Docking/src/main/java/docking/DialogComponentProvider.java index c02e24d0d6..ab82acafc1 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/DialogComponentProvider.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/DialogComponentProvider.java @@ -100,7 +100,7 @@ public class DialogComponentProvider private Dimension defaultSize; /** - * Constructor for a GhidraDialogComponent that be modal and will include a status line and + * Constructor for a GhidraDialogComponent that will be modal and will include a status line and * a button panel. Its title will be the same as its name. * @param title the dialog title. */ @@ -109,8 +109,7 @@ public class DialogComponentProvider } /** - * Constructor for a GhidraDialogComponent that will include a status line and - * a button panel. + * Constructor for a GhidraDialogComponent that will include a status line and a button panel. * @param title the title for this dialog. * @param modal true if this dialog should be modal. */ @@ -369,7 +368,7 @@ public class DialogComponentProvider * To change this behavior, call {@link #setDefaultButton(JButton)} with the desired * default button. * - * @param button the button + * @param button the button */ protected void addButton(JButton button) { if (defaultButton == null && buttonPanel.getComponentCount() == 0) { @@ -681,7 +680,7 @@ public class DialogComponentProvider isAlerting = true; - // Note: manually call validate() so the 'statusLabel' updates its bounds after + // Note: manually call validate() so the 'statusLabel' updates its bounds after // the text has been setStatusText() (validation is buffered which means the // normal Swing mechanism may not have yet happened). mainPanel.validate(); @@ -762,7 +761,7 @@ public class DialogComponentProvider private void showProgressBar(String localTitle, boolean hasProgress, boolean canCancel) { if (!isVisible()) { - // It doesn't make any sense to show the task monitor when the dialog is not + // It doesn't make any sense to show the task monitor when the dialog is not // visible, so show the dialog DockingWindowManager.showDialog(getParent(), this); } @@ -1088,7 +1087,7 @@ public class DialogComponentProvider return dialog; } - private Component getParent() { + protected Component getParent() { if (dialog == null) { return null; } @@ -1210,7 +1209,7 @@ public class DialogComponentProvider /** * Add an action to this dialog. Only actions with icons are added to the toolbar. - * Note, if you add an action to this dialog, do not also add the action to + * Note, if you add an action to this dialog, do not also add the action to * the tool, as this dialog will do that for you. * @param action the action */ @@ -1223,7 +1222,7 @@ public class DialogComponentProvider private void addKeyBindingAction(DockingActionIf action) { - // add the action to the tool in order get key event management (key bindings + // add the action to the tool in order get key event management (key bindings // options and key event processing) DockingWindowManager dwm = DockingWindowManager.getActiveInstance(); if (dwm == null) {