From 62cb57d8bbe9d95f8a5f0de3d3af9ae98c6d32e0 Mon Sep 17 00:00:00 2001 From: ghidra1 Date: Thu, 12 Nov 2020 13:48:03 -0500 Subject: [PATCH] GP-260 Corrected composite resolveequivalence issue introduced with GP-260 --- .../program/database/data/CompositeDB.java | 22 ++++--- .../database/data/DataTypeComponentDB.java | 11 ++-- .../program/database/data/StructureDB.java | 47 +++++++++++--- .../ghidra/program/database/data/UnionDB.java | 14 +++- .../program/model/data/DataTypeComponent.java | 4 +- .../model/data/DataTypeComponentImpl.java | 13 ++-- .../program/model/data/StructureDataType.java | 65 ++++++++++++------- .../program/model/data/UnionDataType.java | 9 ++- 8 files changed, 131 insertions(+), 54 deletions(-) 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 c34db6f84f..4bef0a2106 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 @@ -69,20 +69,26 @@ abstract class CompositeDB extends DataTypeDB implements Composite { protected abstract void initialize(); /** - * Get the preferred length for a new component. Constraining length of fixed-length datatype - * may not be sustainable in response to datatype size changes over time. + * Get the preferred length for a new component. For Unions and internally + * aligned structures the preferred component length for a fixed-length dataType + * will be the length of that dataType. Otherwise the length returned will be no + * larger than the specified length. + * * @param dataType new component datatype - * @param length specified length required for Dynamic types such as string - * which must have a positive length specified. + * @param length constrained length or -1 to force use of dataType size. + * Dynamic types such as string must have a positive length + * specified. * @return preferred component length */ protected int getPreferredComponentLength(DataType dataType, int length) { - if (length > 0 && (dataType instanceof Composite) && - ((Composite) dataType).isNotYetDefined()) { - return length; + if ((isInternallyAligned() || (this instanceof Union)) && !(dataType instanceof Dynamic)) { + length = -1; // force use of datatype size } int dtLength = dataType.getLength(); - if (dtLength > 0) { + if (length <= 0) { + length = dtLength; + } + else if (dtLength > 0 && dtLength < length) { length = dtLength; } if (length <= 0) { 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 c6b4c95178..ca8e1027e8 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 @@ -345,17 +345,20 @@ class DataTypeComponentDB implements InternalDataTypeComponent { DataType myParent = getParent(); boolean aligned = (myParent instanceof Composite) ? ((Composite) myParent).isInternallyAligned() : false; - // Components don't need to have matching offset when they are aligned, only matching ordinal. - // NOTE: use getOffset() and getOrdinal() methods since returned values will differ from + // Components don't need to have matching offset when they are aligned + // NOTE: use getOffset() method since returned values will differ from // stored values for flexible array component if ((!aligned && (getOffset() != dtc.getOffset())) || - // Components don't need to have matching length when they are aligned. Is this correct? - (!aligned && (getLength() != dtc.getLength())) || getOrdinal() != dtc.getOrdinal() || !SystemUtilities.isEqual(getFieldName(), dtc.getFieldName()) || !SystemUtilities.isEqual(getComment(), dtc.getComment())) { return false; } + // Component lengths need only be checked for dynamic types + if (getLength() != dtc.getLength() && (myDt instanceof Dynamic)) { + return false; + } + return DataTypeUtilities.isSameOrEquivalentDataType(myDt, otherDt); } 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 75c55882cf..da90c05cec 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 @@ -684,8 +684,9 @@ class StructureDB extends CompositeDB implements Structure { } /** - * Create copy of structure for target dtm (source archive information is discarded). WARNING! - * copying unaligned structures which contain bitfields can produce invalid results when + * Create copy of structure for target dtm (source archive information is discarded). + *

+ * WARNING! copying unaligned structures which contain bitfields can produce invalid results when * switching endianess due to the differences in packing order. * * @param dtm target data type manager @@ -1281,7 +1282,22 @@ class StructureDB extends CompositeDB implements Structure { DataType dt = resolvedDts[i]; // ancestry check already performed by caller - int length = getPreferredComponentLength(dt, dtc.getLength()); + int length = dt.getLength(); + if (length <= 0 || dtc.isBitFieldComponent()) { + length = dtc.getLength(); + } + else { + // do not exceed available space + int maxOffset; + int nextIndex = i + 1; + if (nextIndex < otherComponents.length) { + maxOffset = otherComponents[nextIndex].getOffset(); + } + else { + maxOffset = struct.getLength(); + } + length = Math.min(length, maxOffset - dtc.getOffset()); + } Record rec = componentAdapter.createRecord(dataMgr.getResolvedID(dt), key, length, dtc.getOrdinal(), dtc.getOffset(), dtc.getFieldName(), dtc.getComment()); @@ -1358,6 +1374,9 @@ class StructureDB extends CompositeDB implements Structure { @Override public void dataTypeSizeChanged(DataType dt) { + if (dt instanceof BitFieldDataType) { + return; // unsupported + } lock.acquire(); try { checkDeleted(); @@ -1374,7 +1393,10 @@ class StructureDB extends CompositeDB implements Structure { // assume no impact to bitfields since base types // should not change size int dtcLen = dtc.getLength(); - int length = getPreferredComponentLength(dt, dtcLen); + int length = dt.getLength(); + if (length <= 0) { + length = dtcLen; + } if (length < dtcLen) { dtc.setLength(length, true); shiftOffsets(i + 1, dtcLen - length, 0); @@ -1427,7 +1449,10 @@ class StructureDB extends CompositeDB implements Structure { continue; } int dtcLen = dtc.getLength(); - int length = getPreferredComponentLength(dt, dtcLen); + int length = dt.getLength(); + if (length <= 0) { + length = dtcLen; + } if (dtcLen != length) { if (length < dtcLen) { dtc.setLength(length, true); @@ -1514,14 +1539,18 @@ class StructureDB extends CompositeDB implements Structure { return false; } - int myNumComps = getNumComponents(); - int otherNumComps = struct.getNumComponents(); + int myNumComps = components.size(); + int otherNumComps = struct.getNumDefinedComponents(); if (myNumComps != otherNumComps) { return false; } + DataTypeComponent[] otherDefinedComponents = struct.getDefinedComponents(); + if (otherDefinedComponents.length != myNumComps) { // safety check + return false; + } for (int i = 0; i < myNumComps; i++) { - DataTypeComponent myDtc = getComponent(i); - DataTypeComponent otherDtc = struct.getComponent(i); + DataTypeComponent myDtc = components.get(i); + DataTypeComponent otherDtc = otherDefinedComponents[i]; if (!myDtc.isEquivalent(otherDtc)) { return false; } 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 2b36b98953..1ec4fa3f91 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 @@ -418,14 +418,19 @@ class UnionDB extends CompositeDB implements Union { @Override public void dataTypeSizeChanged(DataType dt) { + if (dt instanceof BitFieldDataType) { + return; // unsupported + } lock.acquire(); try { checkDeleted(); boolean changed = false; for (DataTypeComponentDB dtc : components) { - int length = dtc.getLength(); if (dtc.getDataType() == dt) { - length = getPreferredComponentLength(dt, length); + int length = dt.getLength(); + if (length <= 0) { + length = dtc.getLength(); + } dtc.setLength(length, true); changed = true; } @@ -448,7 +453,10 @@ class UnionDB extends CompositeDB implements Union { dt = adjustBitField(dt); // in case base type changed } int dtcLen = dtc.getLength(); - int length = getPreferredComponentLength(dt, dtcLen); + int length = dt.getLength(); + if (length <= 0) { + length = dtcLen; + } if (length != dtcLen) { dtc.setLength(length, true); changed = true; diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponent.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponent.java index a633336eb5..50bb8a934e 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponent.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponent.java @@ -160,7 +160,9 @@ public interface DataTypeComponent { * Returns true if the given dataTypeComponent is equivalent to this dataTypeComponent. * A dataTypeComponent is "equivalent" if the other component has a data type * that is equivalent to this component's data type. The dataTypeComponents must - * also have the same offset, length, ordinal, field name, and comment. + * also have the same offset, field name, and comment. The length is only checked + * for components which are dyanmic and whose size must be specified when creating + * a component. * @param dtc the dataTypeComponent being tested for equivalence. * @return true if the given dataTypeComponent is equivalent to this dataTypeComponent. */ 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 594ed6a8d6..464511a870 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 @@ -329,17 +329,20 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali DataType myParent = getParent(); boolean aligned = (myParent instanceof Composite) ? ((Composite) myParent).isInternallyAligned() : false; - // Components don't need to have matching offset when they are aligned, only matching ordinal. + // Components don't need to have matching offset when they are aligned + // NOTE: use getOffset() method since returned values will differ from + // stored values for flexible array component if ((!aligned && (getOffset() != dtc.getOffset())) || - // Components don't need to have matching length when they are aligned. Is this correct? - // NOTE: use getOffset() and getOrdinal() methods since returned values will differ from - // stored values for flexible array component - (!aligned && (getLength() != dtc.getLength())) || getOrdinal() != dtc.getOrdinal() || !SystemUtilities.isEqual(getFieldName(), dtc.getFieldName()) || !SystemUtilities.isEqual(getComment(), dtc.getComment())) { return false; } + // Component lengths need only be checked for dynamic types + if (getLength() != dtc.getLength() && (myDt instanceof Dynamic)) { + return false; + } + return DataTypeUtilities.isSameOrEquivalentDataType(myDt, otherDt); } 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 2763080d8b..3ee3002c1a 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 @@ -797,15 +797,18 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur return false; } - int myNumComps = getNumComponents(); - int otherNumComps = struct.getNumComponents(); + int myNumComps = components.size(); + int otherNumComps = struct.getNumDefinedComponents(); if (myNumComps != otherNumComps) { return false; } + DataTypeComponent[] otherDefinedComponents = struct.getDefinedComponents(); + if (otherDefinedComponents.length != myNumComps) { // safety check + return false; + } for (int i = 0; i < myNumComps; i++) { - DataTypeComponent myDtc = getComponent(i); - DataTypeComponent otherDtc = struct.getComponent(i); - + DataTypeComponent myDtc = components.get(i); + DataTypeComponent otherDtc = otherDefinedComponents[i]; if (!myDtc.isEquivalent(otherDtc)) { return false; } @@ -815,6 +818,9 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur @Override public void dataTypeSizeChanged(DataType dt) { + if (dt instanceof BitFieldDataType) { + return; // unsupported + } if (isInternallyAligned()) { adjustInternalAlignment(); return; @@ -823,21 +829,23 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur int n = components.size(); for (int i = 0; i < n; i++) { DataTypeComponentImpl dtc = components.get(i); - int nextIndex = i + 1; if (dtc.getDataType() == dt) { - // assume no impact to bitfields since base types + // assume no impact to bitfields since base types // should not change size - int dtLen = dt.getLength(); int dtcLen = dtc.getLength(); - if (dtLen < dtcLen) { - dtc.setLength(dtLen); - shiftOffsets(nextIndex, dtcLen - dtLen, 0); + int length = dt.getLength(); + if (length <= 0) { + length = dtcLen; + } + if (length < dtcLen) { + dtc.setLength(length); + shiftOffsets(i + 1, dtcLen - length, 0); didChange = true; } - else if (dtLen > dtcLen) { - int consumed = consumeBytesAfter(i, dtLen - dtcLen); + else if (length > dtcLen) { + int consumed = consumeBytesAfter(i, length - dtcLen); if (consumed > 0) { - shiftOffsets(nextIndex, 0 - consumed, 0); + shiftOffsets(i + 1, 0 - consumed, 0); didChange = true; } } @@ -890,8 +898,9 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur } /** - * Create copy of structure for target dtm (source archive information is discarded). WARNING! - * copying unaligned structures which contain bitfields can produce invalid results when + * Create copy of structure for target dtm (source archive information is discarded). + *

+ * WARNING! copying unaligned structures which contain bitfields can produce invalid results when * switching endianess due to the differences in packing order. * * @param dtm target data type manager @@ -991,8 +1000,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur private void doReplaceWithAligned(Structure struct) { // assumes components is clear and that alignment characteristics have been set DataTypeComponent[] otherComponents = struct.getDefinedComponents(); - for (int i = 0; i < otherComponents.length; i++) { - DataTypeComponent dtc = otherComponents[i]; + for (DataTypeComponent dtc : otherComponents) { DataType dt = dtc.getDataType(); int length = (dt instanceof Dynamic) ? dtc.getLength() : -1; add(dt, length, dtc.getFieldName(), dtc.getComment()); @@ -1011,11 +1019,25 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur DataTypeComponent[] otherComponents = struct.getDefinedComponents(); for (int i = 0; i < otherComponents.length; i++) { DataTypeComponent dtc = otherComponents[i]; - DataType dt = dtc.getDataType().clone(dataMgr); checkAncestry(dt); - int length = getPreferredComponentLength(dt, dtc.getLength()); + int length = dt.getLength(); + if (length <= 0 || dtc.isBitFieldComponent()) { + length = dtc.getLength(); + } + else { + // do not exceed available space + int maxOffset; + int nextIndex = i + 1; + if (nextIndex < otherComponents.length) { + maxOffset = otherComponents[nextIndex].getOffset(); + } + else { + maxOffset = struct.getLength(); + } + length = Math.min(length, maxOffset - dtc.getOffset()); + } components.add(new DataTypeComponentImpl(dt, this, length, dtc.getOrdinal(), dtc.getOffset(), dtc.getFieldName(), dtc.getComment())); @@ -1355,8 +1377,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur @Override public void deleteAll() { - for (int i = 0; i < components.size(); i++) { - DataTypeComponent dtc = components.get(i); + for (DataTypeComponentImpl dtc : components) { dtc.getDataType().removeParent(this); } components.clear(); 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 ebca428fb7..298bcbd5c2 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 @@ -372,11 +372,16 @@ public class UnionDataType extends CompositeDataTypeImpl implements Union { @Override public void dataTypeSizeChanged(DataType dt) { + if (dt instanceof BitFieldDataType) { + return; // unsupported + } boolean changed = false; for (DataTypeComponentImpl dtc : components) { - int length = dtc.getLength(); if (dtc.getDataType() == dt) { - length = getPreferredComponentLength(dt, length); + int length = dt.getLength(); + if (length <= 0) { + length = dtc.getLength(); + } dtc.setLength(length); changed = true; }