diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterDataTypeCellEditor.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterDataTypeCellEditor.java index a39cb66ddc..e7603d7a6f 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterDataTypeCellEditor.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/ParameterDataTypeCellEditor.java @@ -159,7 +159,7 @@ class ParameterDataTypeCellEditor extends AbstractCellEditor fireEditingCanceled(); // user picked the same datatype } else { - dt = dataType; + dt = dataType.clone(dtm); fireEditingStopped(); } } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressModel.java index 36e7f46ea2..24ddd2bed0 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/StorageAddressModel.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. @@ -161,40 +161,7 @@ class StorageAddressModel { private void validate() { statusText = ""; - isValid = hasValidVarnodes() && hasCorrectAllocatedSize() && isProperMix() && hasNoDups(); - } - - private boolean hasNoDups() { - AddressSet addressSet = new AddressSet(); - for (int i = 0; i < varnodes.size(); i++) { - VarnodeInfo varnode = varnodes.get(i); - AddressRange range; - try { - range = new AddressRangeImpl(varnode.getAddress(), varnode.getSize()); - } - catch (AddressOverflowException e) { - // should already have been checked - throw new AssertException("Unexpected exception", e); - } - if (addressSet.intersects(range.getMinAddress(), range.getMaxAddress())) { - statusText = "Row " + i + ": Overlapping storage address used."; - return false; - } - addressSet.add(range); - } - return true; - } - - private boolean isProperMix() { - // all except last varnode must be a register - for (int i = 0; i < varnodes.size() - 1; i++) { - VarnodeInfo varnode = varnodes.get(i); - if (varnode.getType() != VarnodeType.Register) { - statusText = "Only the last entry may be of type " + varnode.getType(); - return false; - } - } - return true; + isValid = hasValidVarnodes() && hasCorrectAllocatedSize(); } private boolean hasCorrectAllocatedSize() { @@ -217,11 +184,22 @@ class StorageAddressModel { private boolean hasValidVarnodes() { for (int i = 0; i < varnodes.size(); i++) { VarnodeInfo varnode = varnodes.get(i); - if (!(isValidSize(varnode, i) && isValidAddress(varnode, i))) { + if (varnode.getAddress() == null) { + statusText = "Row " + i + ": Storage not specified"; + return false; + } + if (!(isValidSize(varnode, i) || !isValidAddress(varnode, i))) { return false; } } - return true; + try { + buildStorage(); + return true; + } + catch (InvalidInputException e) { + statusText = e.getMessage(); + return false; + } } private boolean isValidSize(VarnodeInfo varnode, int row) { @@ -349,10 +327,7 @@ class StorageAddressModel { return program; } - VariableStorage getStorage() { - if (!isValid) { - return null; - } + private VariableStorage buildStorage() throws InvalidInputException { if (varnodes.size() == 0) { if (requiredSize == 0) { return VariableStorage.VOID_STORAGE; @@ -361,10 +336,22 @@ class StorageAddressModel { } List varnodeList = new ArrayList<>(varnodes.size()); for (VarnodeInfo varnodeInfo : varnodes) { - varnodeList.add(new Varnode(varnodeInfo.getAddress(), varnodeInfo.getSize())); + Address addr = varnodeInfo.getAddress(); + if (addr == null) { + // this method should not be invoked with this condition + return VariableStorage.UNASSIGNED_STORAGE; + } + varnodeList.add(new Varnode(addr, varnodeInfo.getSize())); + } + return new VariableStorage(program, varnodeList); + } + + VariableStorage getStorage() { + if (!isValid) { + return null; } try { - return new VariableStorage(program, varnodeList); + return buildStorage(); } catch (InvalidInputException e) { // validation checks should prevent this. diff --git a/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelBETest.java b/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelBETest.java index ccb0f0f052..987398cfec 100644 --- a/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelBETest.java +++ b/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelBETest.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. @@ -40,7 +40,7 @@ public class StorageEditorModelBETest extends StorageEditorModelTest { varnode = model.getVarnodes().get(1); model.setVarnode(varnode, program.getRegister(testRegName).getAddress().add(6), 2); assertTrue(!model.isValid()); - assertEquals("Row 1: Overlapping storage address used.", model.getStatusText()); + assertEquals("One or more conflicting storage varnodes", model.getStatusText()); } @Test @@ -70,4 +70,34 @@ public class StorageEditorModelBETest extends StorageEditorModelTest { assertEquals(register.getAddress().getOffset(), model.getVarnodes().get(0).getAddress().getOffset()); } + + @Override + @Test + public void testCompoundStorage() { + + // Big-endian test case + + Register register = model.getProgram().getRegister("g1"); + assertNotNull(register); + + VarnodeInfo varnode = model.getVarnodes().get(0); + assertEquals(VarnodeType.Stack, varnode.getType()); + + model.addVarnode(); + + varnode = model.getVarnodes().get(1); + + model.setVarnodeType(varnode, VarnodeType.Register); + model.setVarnode(varnode, register); + + assertTrue(!model.isValid()); + assertEquals("Compound storage must use registers except for last BE varnode", + model.getStatusText()); + + // select last row (i.e., register) and move it up to make valid with stack being last + model.setSelectedVarnodeRows(new int[] { 1 }); + model.moveSelectedVarnodeUp(); + + assertTrue(model.isValid()); + } } diff --git a/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelTest.java b/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelTest.java index 3e3776c7f1..32c4d2925a 100644 --- a/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelTest.java +++ b/Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/function/editor/StorageEditorModelTest.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. @@ -70,20 +70,19 @@ public class StorageEditorModelTest extends AbstractGuiTest { Address address = stackSpace.getAddress(4); VariableStorage storage = new VariableStorage(program, address, currentStorage); - model = new StorageAddressModel(program, storage, - new ModelChangeListener() { + model = new StorageAddressModel(program, storage, new ModelChangeListener() { - @Override - public void dataChanged() { - dataChangeCalled = true; - } + @Override + public void dataChanged() { + dataChangeCalled = true; + } - @Override - public void tableRowsChanged() { - // nothing here - } + @Override + public void tableRowsChanged() { + // nothing here + } - }); + }); model.setRequiredSize(requiredStorage, unconstrained); } @@ -297,7 +296,37 @@ public class StorageEditorModelTest extends AbstractGuiTest { varnode = model.getVarnodes().get(1); model.setVarnode(varnode, program.getRegister(testRegName).getAddress(), 2); assertTrue(!model.isValid()); - assertEquals("Row 1: Overlapping storage address used.", model.getStatusText()); + assertEquals("One or more conflicting storage varnodes", model.getStatusText()); + } + + @Test + public void testCompoundStorage() { + + // Little-endian test case + + VarnodeInfo varnode = model.getVarnodes().get(0); + assertEquals(VarnodeType.Stack, varnode.getType()); + + model.addVarnode(); + + // select new row and move it up + model.setSelectedVarnodeRows(new int[] { 1 }); + model.moveSelectedVarnodeUp(); + + varnode = model.getVarnodes().get(0); + + model.setVarnodeType(varnode, VarnodeType.Register); + model.setVarnode(varnode, program.getRegister(testRegName).getAddress(), 4); + + assertTrue(!model.isValid()); + assertEquals("Compound storage must use registers except for first LE varnode", + model.getStatusText()); + + // select last row (i.e., stack) and move it as first to make valid + model.setSelectedVarnodeRows(new int[] { 1 }); + model.moveSelectedVarnodeUp(); + + assertTrue(model.isValid()); } } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableStorage.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableStorage.java index ce15603c0b..ed45116e23 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableStorage.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/VariableStorage.java @@ -181,7 +181,7 @@ public class VariableStorage implements Comparable { return size; } - private void checkVarnodes() throws InvalidInputException { + private void checkVarnodes() throws IllegalArgumentException, InvalidInputException { if (varnodes.length == 0) { throw new IllegalArgumentException("A minimum of one varnode must be specified"); } @@ -191,10 +191,11 @@ public class VariableStorage implements Comparable { for (int i = 0; i < varnodes.length; i++) { Varnode varnode = varnodes[i]; if (varnode == null) { - throw new InvalidInputException("Null varnode not permitted"); + throw new IllegalArgumentException("Null varnode not permitted"); } if (varnode.getSize() <= 0) { - throw new InvalidInputException("Unsupported varnode size: " + varnode.getSize()); + throw new IllegalArgumentException( + "Unsupported varnode size: " + varnode.getSize()); } boolean isRegister = false; @@ -246,13 +247,13 @@ public class VariableStorage implements Comparable { if (programArch.getLanguage().isBigEndian()) { if (i < (varnodes.length - 1) && !isRegister) { throw new InvalidInputException( - "Compound storage must use registers except for least significant varnode"); + "Compound storage must use registers except for last BE varnode"); } } else { if (i > 0 && !isRegister) { throw new InvalidInputException( - "Compound storage must use registers except for most significant varnode"); + "Compound storage must use registers except for first LE varnode"); } } size += varnode.getSize(); @@ -260,7 +261,7 @@ public class VariableStorage implements Comparable { for (int i = 0; i < varnodes.length; i++) { for (int j = i + 1; j < varnodes.length; j++) { if (varnodes[i].intersects(varnodes[j])) { - throw new InvalidInputException("One or more conflicting varnodes"); + throw new InvalidInputException("One or more conflicting storage varnodes"); } } }