From d3d298373c399c1f17a20639e51064969278d82d Mon Sep 17 00:00:00 2001 From: ghidra1 Date: Wed, 13 Nov 2024 16:04:08 -0500 Subject: [PATCH] GP-5037 Improved PDB composite reconstruction. Adding support for alignment padding injection. --- .../pdb/BitFieldGroupCompositeMember.java | 7 +- .../format/pdb/DefaultCompositeMember.java | 263 +++++++++++++----- .../bin/format/pdb/CompositeMemberTest.java | 140 +++++++++- 3 files changed, 336 insertions(+), 74 deletions(-) diff --git a/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/BitFieldGroupCompositeMember.java b/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/BitFieldGroupCompositeMember.java index b96d626cef..608a6aebae 100644 --- a/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/BitFieldGroupCompositeMember.java +++ b/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/BitFieldGroupCompositeMember.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. @@ -84,7 +84,7 @@ public class BitFieldGroupCompositeMember extends CompositeMember { if (list.isEmpty()) { return 0; } - return list.get(0).getLength(); + return list.get(0).getLength(); // length is base-datatype-length } @Override @@ -136,6 +136,7 @@ public class BitFieldGroupCompositeMember extends CompositeMember { if (!member.isSingleBitFieldMember()) { throw new IllegalArgumentException("expected single bit-field member"); } + // Check verifies that bitfield base-datatype-length is the same as the group if (!list.isEmpty() && (member.getOffset() != getOffset() || member.getLength() != getLength())) { throw new IllegalArgumentException( diff --git a/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/DefaultCompositeMember.java b/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/DefaultCompositeMember.java index b055b98459..4950646256 100644 --- a/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/DefaultCompositeMember.java +++ b/Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/DefaultCompositeMember.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. @@ -16,6 +16,7 @@ package ghidra.app.util.bin.format.pdb; import java.util.*; +import java.util.Map.Entry; import java.util.function.Consumer; import ghidra.program.model.data.*; @@ -39,6 +40,8 @@ public class DefaultCompositeMember extends CompositeMember { private static int MAX_CONSTRUCTION_DEPTH = 20; + private static final String PADDING_COMPONENT_NAME = "_padding_"; + private DataTypeManager dataTypeManager; private Consumer errorConsumer; @@ -58,8 +61,10 @@ public class DefaultCompositeMember extends CompositeMember { private BitFieldGroupCompositeMember bitFieldGroup; // Structure container data - private Map structureMemberOffsetMap; + private TreeMap structureMemberOffsetMap; private RangeMap structureMemberRangeMap; + private int largestPrimitiveSize; + private boolean hasPadding = false; // Union container data private List unionMemberList; @@ -135,6 +140,8 @@ public class DefaultCompositeMember extends CompositeMember { dataTypeManager = member.dataTypeManager; structureMemberOffsetMap = member.structureMemberOffsetMap; structureMemberRangeMap = member.structureMemberRangeMap; + // allow padding size to use pointer-size and smaller by default + largestPrimitiveSize = member.dataTypeManager.getDataOrganization().getPointerSize(); unionMemberList = member.unionMemberList; } @@ -148,7 +155,7 @@ public class DefaultCompositeMember extends CompositeMember { */ private DefaultCompositeMember(int componentOffset, DataType baseDataType, int bitSize, int bitOffsetWithinBaseType) throws InvalidDataTypeException { - memberName = "padding"; + memberName = PADDING_COMPONENT_NAME; memberDataType = new PdbBitField(baseDataType, bitSize, bitOffsetWithinBaseType); memberIsZeroLengthArray = false; memberOffset = componentOffset; @@ -314,6 +321,8 @@ public class DefaultCompositeMember extends CompositeMember { */ private void alignComposite(int preferredSize) { + Composite composite = (Composite) memberDataType; + // don't attempt to align empty composite - don't complain if (isStructureContainer()) { if (structureMemberOffsetMap.isEmpty()) { @@ -324,7 +333,6 @@ public class DefaultCompositeMember extends CompositeMember { return; } - Composite composite = (Composite) memberDataType; Composite copy = (Composite) composite.copy(dataTypeManager); int pack = 0; @@ -333,20 +341,34 @@ public class DefaultCompositeMember extends CompositeMember { boolean alignOK = isGoodAlignment(copy, preferredSize); if (alignOK) { composite.setToDefaultPacking(); + if (hasPadding) { + removeUnnecessaryPadding(composite); + } } else { - copy.setToMachineAligned(); - alignOK = isGoodAlignment(copy, preferredSize); - if (alignOK) { - composite.setToDefaultPacking(); - composite.setToMachineAligned(); - } - else { - pack = 1; - copy.setExplicitPackingValue(pack); + if (preferredSize > 0 && copy.getLength() != preferredSize) { + copy.setToMachineAligned(); // will only impact structure length alignOK = isGoodAlignment(copy, preferredSize); if (alignOK) { - composite.setExplicitPackingValue(pack); + composite.setToDefaultPacking(); + composite.setToMachineAligned(); + if (hasPadding) { + removeUnnecessaryPadding(composite); + } + } + else { + copy.setToDefaultAligned(); // restore default alignment + } + } + if (!alignOK) { + removeAllPadding(composite); // includes bit-field padding + if (!hasPadding) { + pack = 1; + copy.setExplicitPackingValue(pack); + alignOK = isGoodAlignment(copy, preferredSize); + if (alignOK) { + composite.setExplicitPackingValue(pack); + } } } } @@ -357,6 +379,64 @@ public class DefaultCompositeMember extends CompositeMember { } } + private void removeUnnecessaryPadding(Composite packedComposite) { + if (!packedComposite.isPackingEnabled()) { + throw new IllegalArgumentException("composite must have packing enabled"); + } + if (!(packedComposite instanceof Structure struct)) { + return; + } + int preferredLength = packedComposite.getLength(); + DataTypeComponent[] definedComponents = struct.getDefinedComponents(); + int lastIndex = definedComponents.length - 1; + for (int i = 0; i < definedComponents.length; i++) { + DataTypeComponent dtc = definedComponents[i]; + if (!isPaddingComponent(dtc, true)) { + continue; // leave bitfield padding intact + } + int nextComponentOffset = -1; + if (i < lastIndex) { + nextComponentOffset = definedComponents[i + 1].getOffset(); + } + int ordinal = dtc.getOrdinal(); + + // experiment with padding removal and restore if removal impacts structure + struct.delete(ordinal); + + if (struct.getLength() != preferredLength || (nextComponentOffset > 0 && + nextComponentOffset != definedComponents[i + 1].getOffset())) { + // restore padding component + struct.insert(ordinal, dtc.getDataType(), -1, PADDING_COMPONENT_NAME, null); + } + } + } + + private void removeAllPadding(Composite composite) { + if (!(composite instanceof Structure struct)) { + return; + } + boolean doDelete = composite.isPackingEnabled(); + DataTypeComponent[] definedComponents = struct.getDefinedComponents(); + for (int i = definedComponents.length - 1; i >= 0; i--) { + DataTypeComponent dtc = definedComponents[i]; + if (isPaddingComponent(dtc, false)) { + if (doDelete) { + struct.delete(dtc.getOrdinal()); + } + else { + struct.clearComponent(dtc.getOrdinal()); + } + } + } + } + + private boolean isPaddingComponent(DataTypeComponent dtc, boolean skipBitFields) { + if (skipBitFields && dtc.isBitFieldComponent()) { + return false; + } + return PADDING_COMPONENT_NAME.equals(dtc.getFieldName()); + } + private boolean isGoodAlignment(Composite testComposite, int preferredSize) { boolean alignOK = true; if (preferredSize > 0 && testComposite.getNumComponents() != 0) { @@ -410,6 +490,7 @@ public class DefaultCompositeMember extends CompositeMember { @Override int getLength() { if (memberDataType instanceof BitFieldDataType) { + // FIXME: This assumption interferes with pack(1) case for bitfields BitFieldDataType bitfield = (BitFieldDataType) memberDataType; return bitfield.getBaseTypeSize(); } @@ -424,6 +505,9 @@ public class DefaultCompositeMember extends CompositeMember { memberType = MemberType.STRUCTURE; structureMemberOffsetMap = new TreeMap<>(); structureMemberRangeMap = new RangeMap(-1); + // allow padding size to use pointer-size and smaller by default + largestPrimitiveSize = memberDataType.getDataOrganization().getPointerSize(); + hasPadding = false; unionMemberList = null; } else { @@ -610,7 +694,6 @@ public class DefaultCompositeMember extends CompositeMember { } DefaultCompositeMember memberCopy = new DefaultCompositeMember(this); - memberCopy.memberOffset = 0; CategoryPath tempCategoryPath = parent.getDataType().getCategoryPath(); String tempName = allocateTemporaryContainerName("struct"); @@ -618,57 +701,18 @@ public class DefaultCompositeMember extends CompositeMember { Structure nestedStructure = new StructureDataType(tempCategoryPath, tempName, 0, dataTypeManager); - String oldName = memberName; - DataType oldDataType = memberDataType; - - DefaultCompositeMember deferredBitFieldMember = null; - if (oldDataType instanceof PdbBitField) { - PdbBitField bitfieldDt = (PdbBitField) oldDataType; - try { - int bitOffset = bitfieldDt.getBitOffsetWithinBase(); - DefaultCompositeMember padding = getPaddingBitField(null, memberCopy); - if (padding != null) { - deferredBitFieldMember = memberCopy; - memberCopy = padding; - bitfieldDt = (PdbBitField) memberCopy.memberDataType; - bitOffset = bitfieldDt.getBitOffsetWithinBase(); - } - else if (bitOffset < 0) { - // TODO: assumes little-endian, add support for big-endian - bitOffset = 0; - } - insertMinimalStructureBitfield(nestedStructure, 0, memberCopy.memberName, - bitfieldDt, memberCopy.getMemberComment()); - } - catch (InvalidDataTypeException e) { - Msg.error(this, "PDB failed to add bitfield: " + e.getMessage()); - return false; - } - } - else { - nestedStructure.insertAtOffset(0, oldDataType, oldDataType.getLength(), oldName, - memberCopy.getMemberComment()); - } - memberName = tempName; + memberOffset = 0; memberDataType = nestedStructure; memberIsZeroLengthArray = false; memberDataTypeName = null; // signifies a container initializeContainer(); - structureMemberRangeMap.paintRange(0, memberCopy.getLength() - 1, 0); - structureMemberOffsetMap.put(0, memberCopy); - - memberCopy.setParent(this); - if (parent != null) { - parent.memberChanged(oldName, this); + parent.memberChanged(memberCopy.memberName, this); } - if (deferredBitFieldMember != null) { - return addStructureMember(deferredBitFieldMember); - } - return true; + return addStructureMember(memberCopy); } private String getMemberComment() { @@ -688,6 +732,85 @@ public class DefaultCompositeMember extends CompositeMember { return buf.toString(); } + private int getMinimumPackedStructureLength() { + if (!isStructureContainer()) { + throw new IllegalStateException(); + } + + Entry lastEntry = structureMemberOffsetMap.lastEntry(); + if (lastEntry == null) { + return 0; + } + + int lastOffset = lastEntry.getKey(); + CompositeMember lastMember = lastEntry.getValue(); + return lastOffset + lastMember.getLength(); + } + + /** + * Insert minimal padding into structure prior to the addition of a component such that packing + * will allow component to be placed at intended offset. + * @param nextComponentOffset + * @param dt + */ + private void insertMinimalStructurePadding(int nextComponentOffset, DataType dt) { + + if (!isStructureContainer()) { + throw new IllegalStateException(); + } + + int structLen = getMinimumPackedStructureLength(); + if (nextComponentOffset <= structLen) { + return; + } + + if (dt instanceof AbstractIntegerDataType) { + largestPrimitiveSize = Math.max(largestPrimitiveSize, dt.getLength()); + } + + Structure struct = (Structure) memberDataType; + + int fillSpace = nextComponentOffset - structLen; + while (fillSpace > 0) { + + int alignedOffset = DataOrganizationImpl.getAlignedOffset(dt.getAlignment(), structLen); + if (alignedOffset == nextComponentOffset) { + return; + } + + DataType paddingDt = getPaddingDataType(nextComponentOffset, structLen); + if (paddingDt == null) { + return; + } + + int paddingOffset = + DataOrganizationImpl.getAlignedOffset(paddingDt.getAlignment(), structLen); + struct.insertAtOffset(paddingOffset, paddingDt, -1, PADDING_COMPONENT_NAME, null); + hasPadding = true; + + structLen = struct.getLength(); + fillSpace = nextComponentOffset - structLen; + } + } + + private DataType getPaddingDataType(int nextComponentOffset, int structLen) { + + if (largestPrimitiveSize <= 1) { + return new CharDataType(dataTypeManager); + } + + for (int paddingSize = largestPrimitiveSize; paddingSize > 1; --paddingSize) { + DataType paddingDt = + AbstractIntegerDataType.getSignedDataType(paddingSize, dataTypeManager); + int alignedOffset = + DataOrganizationImpl.getAlignedOffset(paddingDt.getAlignment(), structLen); + if ((alignedOffset + paddingSize) <= nextComponentOffset) { + return paddingDt; + } + } + return new CharDataType(dataTypeManager); + } + /** * Insert a structure bitfield without creating additional undefined padding * components (i.e., keep to minimal storage size). @@ -751,9 +874,6 @@ public class DefaultCompositeMember extends CompositeMember { Composite composite = (Composite) memberDataType; DataTypeComponent component = composite.getComponent(composite.getNumComponents() - 1); -// if (component.getOffset() != newMember.getOffset()) { -// return false; // unexpected -// } DataType dataType = component.getDataType(); if (!(dataType instanceof BitFieldDataType) && !(dataType == DataType.DEFAULT)) { @@ -843,10 +963,13 @@ public class DefaultCompositeMember extends CompositeMember { // TODO: assumes little-endian, add support for big-endian bitOffset = 0; } + insertMinimalStructurePadding(member.memberOffset, + bitfieldDt.getBaseDataType()); insertMinimalStructureBitfield((Structure) memberDataType, member.memberOffset, member.getName(), bitfieldDt, member.getMemberComment()); } else { + insertMinimalStructurePadding(member.memberOffset, member.memberDataType); ((Structure) memberDataType).insertAtOffset(member.memberOffset, member.memberDataType, member.getLength(), member.memberName, member.getMemberComment()); @@ -955,13 +1078,29 @@ public class DefaultCompositeMember extends CompositeMember { return true; } + int unionMemberCount = unionMemberList.size(); + CompositeMember lastUnionMember = + unionMemberCount == 0 ? null : unionMemberList.get(unionMemberCount - 1); + // NOTE: It is assumed that offset will always be ascending and not reach back to union // members before the last one - CompositeMember lastUnionMember = unionMemberList.get(unionMemberList.size() - 1); + if (lastUnionMember == null) { + member.parent = this; + if (!member.transformIntoStructureContainer()) { + return false; + } + ((Union) memberDataType).add(member.memberDataType, member.memberName, null); + unionMemberList.add(member); + if (parent != null) { + parent.sizeChanged(this); + } + return true; + } if (lastUnionMember.isStructureContainer() && member.memberOffset >= lastUnionMember.getOffset()) { + DefaultCompositeMember struct = (DefaultCompositeMember) lastUnionMember; if (struct.isRelatedBitField(member.memberOffset - lastUnionMember.getOffset(), member)) { diff --git a/Ghidra/Features/PDB/src/test/java/ghidra/app/util/bin/format/pdb/CompositeMemberTest.java b/Ghidra/Features/PDB/src/test/java/ghidra/app/util/bin/format/pdb/CompositeMemberTest.java index c98dec1f5b..8ca817fab5 100644 --- a/Ghidra/Features/PDB/src/test/java/ghidra/app/util/bin/format/pdb/CompositeMemberTest.java +++ b/Ghidra/Features/PDB/src/test/java/ghidra/app/util/bin/format/pdb/CompositeMemberTest.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. @@ -103,6 +103,62 @@ public class CompositeMemberTest extends AbstractGhidraHeadlessIntegrationTest Msg.info(this, message + " (Test: " + getName() + ")"); } + @Test + public void testUnionWithStructureOfBitFieldsWithPadding() throws Exception { + + /** + + + + + + + union TestUnion { + struct { + int :32; + int :11; + int field1:1; + int field2:4; + }; + short fieldSHORT; + }; + + */ + UnionDataType union = new UnionDataType(CategoryPath.ROOT, "testUnion", dataMgr); + + //@formatter:off + List members = + CollectionUtils.asList( + new MyPdbMember("field1:0x1:0xb", "int", 4), + new MyPdbMember("field2:0x4:0xc", "int", 4), + new MyPdbMember("fieldSHORT", "short", 0) + ); + //@formatter:on + + assertTrue(DefaultCompositeMember.applyDataTypeMembers(union, false, 8, members, this, + TaskMonitor.DUMMY)); + + //@formatter:off + CompositeTestUtils.assertExpectedComposite(this, + "/testUnion\n" + + "pack()\n" + + "Union testUnion {\n" + + " 0 testUnion_s_0 8 _s_0 \"\"\n" + + " 0 short 2 fieldSHORT \"\"\n" + + "}\n" + + "Length: 8 Alignment: 4\n" + + "/testUnion/testUnion_s_0\n" + + "pack()\n" + + "Structure testUnion_s_0 {\n" + + " 0 int 4 _padding_ \"\"\n" + + " 4 int:11(0) 2 _padding_ \"\"\n" + + " 5 int:1(3) 1 field1 \"\"\n" + + " 5 int:4(4) 1 field2 \"\"\n" + + "}\n" + + "Length: 8 Alignment: 4", union, true); + //@formatter:on + } + @Test public void testSimpleStructureWithBitFields() throws Exception { @@ -137,6 +193,72 @@ public class CompositeMemberTest extends AbstractGhidraHeadlessIntegrationTest //@formatter:on } + @Test + public void testSimplePack1StructureWithBitFields() throws Exception { + + StructureDataType struct = new StructureDataType("struct", 0, dataMgr); + + //@formatter:off + List members = + CollectionUtils.asList( + new MyPdbMember("a", "uchar", 0), + new MyPdbMember("b:0x4:0x0", "uchar", 1), + new MyPdbMember("c:0x4:0x4", "uchar", 1), + new MyPdbMember("d:0x4:0x0", "uchar", 2), + new MyPdbMember("e:0x4:0x0", "uint", 3), + new MyPdbMember("f", "ushort", 7)); + //@formatter:on + + assertTrue(DefaultCompositeMember.applyDataTypeMembers(struct, false, 9, members, this, + TaskMonitor.DUMMY)); + + //@formatter:off + CompositeTestUtils.assertExpectedComposite(this, + "/struct\n" + + "pack(1)\n" + + "Structure struct {\n" + + " 0 uchar 1 a \"\"\n" + + " 1 uchar:4(0) 1 b \"\"\n" + + " 1 uchar:4(4) 1 c \"\"\n" + + " 2 uchar:4(0) 1 d \"\"\n" + + " 3 uint:4(0) 1 e \"\"\n" + + " 7 ushort 2 f \"\"\n" + + "}\n" + + "Length: 9 Alignment: 1", struct, true); + //@formatter:on + } + + @Test + public void testSimpleNoPackStructureWithBitFields() throws Exception { + + StructureDataType struct = new StructureDataType("struct", 0, dataMgr); + + //@formatter:off + List members = + CollectionUtils.asList( + new MyPdbMember("a", "uchar", 0), + new MyPdbMember("b:0x4:0x0", "uchar", 1), + new MyPdbMember("e:0x4:0x0", "uint", 3), + new MyPdbMember("f", "ushort", 7)); + //@formatter:on + + assertTrue(DefaultCompositeMember.applyDataTypeMembers(struct, false, 9, members, this, + TaskMonitor.DUMMY)); + + //@formatter:off + CompositeTestUtils.assertExpectedComposite(this, + "/struct\n" + + "pack(disabled)\n" + + "Structure struct {\n" + + " 0 uchar 1 a \"\"\n" + + " 1 uchar:4(0) 1 b \"\"\n" + + " 3 uint:4(0) 1 e \"\"\n" + + " 7 ushort 2 f \"\"\n" + + "}\n" + + "Length: 9 Alignment: 1", struct, true); + //@formatter:on + } + @Test public void testSimpleStructureWithFillerBitFields() throws Exception { @@ -162,10 +284,10 @@ public class CompositeMemberTest extends AbstractGhidraHeadlessIntegrationTest "pack()\n" + "Structure struct {\n" + " 0 uchar 1 a \"\"\n" + - " 1 uchar:2(0) 1 padding \"\"\n" + + " 1 uchar:2(0) 1 _padding_ \"\"\n" + " 1 uchar:2(2) 1 b \"\"\n" + " 1 uchar:1(4) 1 c1 \"\"\n" + - " 1 uchar:1(5) 1 padding \"\"\n" + + " 1 uchar:1(5) 1 _padding_ \"\"\n" + " 1 uchar:1(6) 1 c2 \"\"\n" + " 2 uchar:7(0) 1 d \"\"\n" + " 4 uint:4(0) 1 e \"\"\n" + @@ -238,20 +360,20 @@ public class CompositeMemberTest extends AbstractGhidraHeadlessIntegrationTest "pack()\n" + "Structure struct_u_0_s_0 {\n" + " 0 char:1(0) 1 a0 \"\"\n" + - " 0 char:1(1) 1 padding \"\"\n" + + " 0 char:1(1) 1 _padding_ \"\"\n" + " 0 char:1(2) 1 a2 \"\"\n" + - " 0 char:1(3) 1 padding \"\"\n" + + " 0 char:1(3) 1 _padding_ \"\"\n" + " 0 char:1(4) 1 a4 \"\"\n" + "}\n" + "Length: 1 Alignment: 1\n" + "/struct/struct_u_0/struct_u_0_s_1\n" + "pack()\n" + "Structure struct_u_0_s_1 {\n" + - " 0 char:1(0) 1 padding \"\"\n" + + " 0 char:1(0) 1 _padding_ \"\"\n" + " 0 char:1(1) 1 a1 \"\"\n" + - " 0 char:1(2) 1 padding \"\"\n" + + " 0 char:1(2) 1 _padding_ \"\"\n" + " 0 char:1(3) 1 a3 \"\"\n" + - " 0 char:1(4) 1 padding \"\"\n" + + " 0 char:1(4) 1 _padding_ \"\"\n" + " 0 char:1(5) 1 a5 \"\"\n" + "}\n" + "Length: 1 Alignment: 1", struct, true);