Merge remote-tracking branch 'origin/GP-5475_ghidragon_disallow_spaces_in_fieldNames--SQUASHED'

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/data/EditDataFieldDialog.java

@@ -183,10 +183,11 @@ public class EditDataFieldDialog extends DialogComponentProvider {
 
 	boolean hasNameChange() {
 		String newName = getNewFieldName();
-		if (newName.equals(component.getFieldName())) {
+		String currentName = component.getFieldName();
-			return false;
+		if (currentName == null) {
+			currentName = component.getDefaultFieldName();
 		}
-		if (newName.equals(component.getDefaultFieldName())) {
+		if (newName.equals(currentName)) {
 			return false;
 		}
 		return true;

Ghidra/Features/Base/src/test.slow/java/ghidra/app/merge/datatypes/DataTypeMerge1Test.java

@@ -98,7 +98,8 @@ public class DataTypeMerge1Test extends AbstractDataTypeMergeTest {
 			@Override
 			public void modifyLatest(ProgramDB program) {
 				// change the name
-				Category c = program.getDataTypeManager().getCategory(
+				Category c = program.getDataTypeManager()
+						.getCategory(
 							new CategoryPath("/Category1/Category2/Category5"));
 				try {
 					c.createCategory("AnotherCategory");
@@ -259,7 +260,8 @@ public class DataTypeMerge1Test extends AbstractDataTypeMergeTest {
 			@Override
 			public void modifyLatest(ProgramDB program) {
 				// change the name
-				Category c = program.getDataTypeManager().getCategory(
+				Category c = program.getDataTypeManager()
+						.getCategory(
 							new CategoryPath("/Category1/Category2/Category5"));
 				try {
 					c.createCategory("AnotherCategory");
@@ -312,7 +314,8 @@ public class DataTypeMerge1Test extends AbstractDataTypeMergeTest {
 			@Override
 			public void modifyLatest(ProgramDB program) {
 				// change the name
-				Category c = program.getDataTypeManager().getCategory(
+				Category c = program.getDataTypeManager()
+						.getCategory(
 							new CategoryPath("/Category1/Category2/Category5"));
 				try {
 					c.createCategory("AnotherCategory");
@@ -952,7 +955,7 @@ public class DataTypeMerge1Test extends AbstractDataTypeMergeTest {
 			(Structure) dtm.getDataType(new CategoryPath("/Category1/Category2/Category3"),
 				"IntStruct");
 		DataTypeComponent dtc = s.getComponent(2);
-		assertEquals("My Field Three", dtc.getFieldName());
+		assertEquals("My_Field_Three", dtc.getFieldName());
 		assertEquals("my comments for Field 3", dtc.getComment());
 
 		dtc = s.getComponent(0);

Ghidra/Features/Base/src/test.slow/java/ghidra/app/merge/datatypes/DataTypeMerge3Test.java

@@ -143,7 +143,7 @@ public class DataTypeMerge3Test extends AbstractDataTypeMergeTest {
 		DataType dt = dtcs[3].getDataType();
 		assertTrue(dt.isEquivalent(new FloatDataType()));
 		assertEquals("my comments", dtcs[3].getComment());
-		assertEquals("Float Field", dtcs[3].getFieldName());
+		assertEquals("Float_Field", dtcs[3].getFieldName());
 	}
 
 	@Test
@@ -201,7 +201,7 @@ public class DataTypeMerge3Test extends AbstractDataTypeMergeTest {
 		DataType dt = dtcs[5].getDataType();
 		assertTrue(dt.isEquivalent(new FloatDataType()));
 		assertEquals("my comments", dtcs[5].getComment());
-		assertEquals("Float Field", dtcs[5].getFieldName());
+		assertEquals("Float_Field", dtcs[5].getFieldName());
 	}
 
 	@Test
@@ -429,7 +429,7 @@ public class DataTypeMerge3Test extends AbstractDataTypeMergeTest {
 		assertEquals(3, barComps.length);
 
 		assertEquals(bar, coolUnionComps[5].getDataType());
-		assertEquals("My field name", coolUnionComps[5].getFieldName());
+		assertEquals("My_field_name", coolUnionComps[5].getFieldName());
 		assertEquals("My comments", coolUnionComps[5].getComment());
 
 		assertTrue(barComps[2].getDataType() instanceof BadDataType);
@@ -1168,7 +1168,7 @@ public class DataTypeMerge3Test extends AbstractDataTypeMergeTest {
 		dtcs = union.getComponents();
 		assertEquals(6, dtcs.length);
 		assertEquals("my comments", dtcs[5].getComment());
-		assertEquals("Float Field", dtcs[5].getFieldName());
+		assertEquals("Float_Field", dtcs[5].getFieldName());
 
 	}
 
@@ -1237,7 +1237,7 @@ public class DataTypeMerge3Test extends AbstractDataTypeMergeTest {
 		dtcs = union.getComponents();
 		assertEquals(4, dtcs.length);
 		assertEquals("my comments", dtcs[3].getComment());
-		assertEquals("Float Field", dtcs[3].getFieldName());
+		assertEquals("Float_Field", dtcs[3].getFieldName());
 	}
 
 	@Test

Ghidra/Features/Base/src/test.slow/java/ghidra/app/merge/datatypes/DataTypeMerge5Test.java

@@ -300,7 +300,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(7, dtcs.length);
-		assertEquals("typedef field name TD_MyEnumPointer", dtcs[6].getFieldName());
+		assertEquals("typedef_field_name_TD_MyEnumPointer", dtcs[6].getFieldName());
 		assertEquals("a typedef", dtcs[6].getComment());
 
 		Enum enumm = (Enum) dtm.getDataType(new CategoryPath("/Category1"), "MyEnum");
@@ -420,7 +420,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(7, dtcs.length);
-		assertEquals("typedef field name TD_MyEnumPointer", dtcs[6].getFieldName());
+		assertEquals("typedef_field_name_TD_MyEnumPointer", dtcs[6].getFieldName());
 		assertEquals("a typedef", dtcs[6].getComment());
 
 		Enum enumm = (Enum) dtm.getDataType(new CategoryPath("/MISC"), "FavoriteColors");
@@ -541,7 +541,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(7, dtcs.length);
-		assertEquals("typedef field name TD_MyEnumPointer", dtcs[6].getFieldName());
+		assertEquals("typedef_field_name_TD_MyEnumPointer", dtcs[6].getFieldName());
 		assertEquals("a typedef", dtcs[6].getComment());
 
 		Enum enumm = (Enum) dtm.getDataType(new CategoryPath("/MISC"), "FavoriteColors");
@@ -667,7 +667,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(6, dtcs.length);
-		assertEquals("Float Field", dtcs[5].getFieldName());
+		assertEquals("Float_Field", dtcs[5].getFieldName());
 		assertEquals("my comments", dtcs[5].getComment());
 
 		TypeDef td = (TypeDef) dtm.getDataType(new CategoryPath("/Category1"), "TD_MyEnumPointer");
@@ -773,7 +773,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(7, dtcs.length);
-		assertEquals("typedef field name TD_MyEnumPointer *", dtcs[6].getFieldName());
+		assertEquals("typedef_field_name_TD_MyEnumPointer_*", dtcs[6].getFieldName());
 		assertEquals("a pointer to a typedef", dtcs[6].getComment());
 
 		Enum enumm = (Enum) dtm.getDataType(new CategoryPath("/MISC"), "FavoriteColors");
@@ -901,7 +901,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(7, dtcs.length);
-		assertEquals("typedef field name", dtcs[6].getFieldName());
+		assertEquals("typedef_field_name", dtcs[6].getFieldName());
 		assertEquals("a typedef on a pointer to a typedef", dtcs[6].getComment());
 
 		Enum enumm = (Enum) dtm.getDataType(new CategoryPath("/MISC"), "FavoriteColors");
@@ -1039,7 +1039,7 @@ public class DataTypeMerge5Test extends AbstractDataTypeMergeTest {
 
 		DataTypeComponent[] dtcs = union.getComponents();
 		assertEquals(7, dtcs.length);
-		assertEquals("array of typedef field name", dtcs[6].getFieldName());
+		assertEquals("array_of_typedef_field_name", dtcs[6].getFieldName());
 		assertEquals("an array of typedefs on a pointer to a typedef", dtcs[6].getComment());
 
 		Enum enumm = (Enum) dtm.getDataType(new CategoryPath("/MISC"), "FavoriteColors");

Ghidra/Framework/Generic/src/main/java/ghidra/util/StringUtilities.java

@@ -1207,4 +1207,17 @@ public class StringUtilities {
 	public static String wrapToWidth(String str, int width) {
 		return new LineWrapper(width).append(str).finish();
 	}
+
+	/**
+	 * Removes any whitespace from start or end of string, then replaces any non-printable
+	 * character (< 32) or spaces (32) with an underscore.
+	 * @param s the string to adjust
+	 * @return a new trimmed string with underscores replacing any non-printable characters.
+	 */
+	public static String whitespaceToUnderscores(String s) {
+		if (s == null) {
+			return null;
+		}
+		return s.trim().replaceAll("[\\x00-\\x20]", "_");
+	}
 }

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/ComponentDBAdapterV0.java

@@ -18,6 +18,7 @@ package ghidra.program.database.data;
 import java.io.IOException;
 
 import db.*;
+import ghidra.util.StringUtilities;
 import ghidra.util.exception.VersionException;
 
 /**
@@ -73,13 +74,16 @@ class ComponentDBAdapterV0 extends ComponentDBAdapter {
 	@Override
 	DBRecord createRecord(long dataTypeID, long parentID, int length, int ordinal, int offset,
 			String name, String comment) throws IOException {
+		// Don't allow whitespace in field names. Until we change the API to throw an exception
+		// when a field name has whitespace, just silently replace whitespace with underscores.
+		String fieldName = StringUtilities.whitespaceToUnderscores(name);
 		long key =
 			DataTypeManagerDB.createKey(DataTypeManagerDB.COMPONENT, componentTable.getKey());
 		DBRecord record = ComponentDBAdapter.COMPONENT_SCHEMA.createRecord(key);
 		record.setLongValue(ComponentDBAdapter.COMPONENT_PARENT_ID_COL, parentID);
 		record.setLongValue(ComponentDBAdapter.COMPONENT_OFFSET_COL, offset);
 		record.setLongValue(ComponentDBAdapter.COMPONENT_DT_ID_COL, dataTypeID);
-		record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, name);
+		record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, fieldName);
 		record.setString(ComponentDBAdapter.COMPONENT_COMMENT_COL, comment);
 		record.setIntValue(ComponentDBAdapter.COMPONENT_SIZE_COL, length);
 		record.setIntValue(ComponentDBAdapter.COMPONENT_ORDINAL_COL, ordinal);
@@ -107,5 +111,4 @@ class ComponentDBAdapterV0 extends ComponentDBAdapter {
 		return componentTable.findRecords(new LongField(compositeID),
 			ComponentDBAdapter.COMPONENT_PARENT_ID_COL);
 	}
-
 }

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeComponentDB.java

@@ -227,37 +227,12 @@ class DataTypeComponentDB implements InternalDataTypeComponent {
 	@Override
 	public void setFieldName(String name) throws DuplicateNameException {
 		if (record != null) {
-			name = checkFieldName(name);
+			String fieldName = cleanupFieldName(name);
-			record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, name);
+			record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, fieldName);
 			updateRecord(true);
 		}
 	}
 
-	private void checkDuplicateName(String name) throws DuplicateNameException {
-		DataTypeComponentImpl.checkDefaultFieldName(name);
-		for (DataTypeComponent comp : parent.getDefinedComponents()) {
-			if (comp == this) {
-				continue;
-			}
-			if (name.equals(comp.getFieldName())) {
-				throw new DuplicateNameException("Duplicate field name: " + name);
-			}
-		}
-	}
-
-	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
@@ -431,9 +406,8 @@ class DataTypeComponentDB implements InternalDataTypeComponent {
 			if (StringUtils.isBlank(comment)) {
 				comment = null;
 			}
-			// TODO: Need to check field name and throw DuplicateNameException
+			String fieldName = cleanupFieldName(name);
-			// name = checkFieldName(name);
+			record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, fieldName);
-			record.setString(ComponentDBAdapter.COMPONENT_FIELD_NAME_COL, name);
 			record.setLongValue(ComponentDBAdapter.COMPONENT_DT_ID_COL, dataMgr.getResolvedID(dt));
 			record.setString(ComponentDBAdapter.COMPONENT_COMMENT_COL, comment);
 			updateRecord(false);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeComponentImpl.java

@@ -58,7 +58,7 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali
 		this.ordinal = ordinal;
 		this.offset = offset;
 		this.length = length;
-		this.fieldName = fieldName;
+		this.fieldName = cleanupFieldName(fieldName);
 		setDataType(dataType);
 		setComment(comment);
 	}
@@ -130,32 +130,7 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali
 
 	@Override
 	public void setFieldName(String name) throws DuplicateNameException {
-		this.fieldName = checkFieldName(name);
+		this.fieldName = cleanupFieldName(name);
-	}
-
-	private void checkDuplicateName(String name) throws DuplicateNameException {
-		checkDefaultFieldName(name);
-		if (parent == null) {
-			return; // Bad situation
-		}
-		for (DataTypeComponent comp : parent.getDefinedComponents()) {
-			if (comp != this && name.equals(comp.getFieldName())) {
-				throw new DuplicateNameException("Duplicate field name: " + name);
-			}
-		}
-	}
-
-	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 {
@@ -192,22 +167,20 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali
 	/**
 	 * 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 newDataType new resolved datatype
-	 * @param cmt new comment
+	 * @param newComment new comment
 	 */
-	void update(String name, DataType dt, String cmt) {
+	void update(String name, DataType newDataType, String newComment) {
-		// TODO: Need to check field name and throw DuplicateNameException
+		this.fieldName = cleanupFieldName(name);
-		// this.fieldName =  = checkFieldName(name);
+		this.dataType = newDataType;
-		this.fieldName = name;
+		this.comment = StringUtils.isBlank(newComment) ? null : newComment;
-		this.dataType = dt;
-		this.comment = StringUtils.isBlank(cmt) ? null : cmt;
 	}
 
 	@Override
-	public void update(int ordinal, int offset, int length) {
+	public void update(int newOrdinal, int newOffset, int newLength) {
-		this.ordinal = ordinal;
+		this.ordinal = newOrdinal;
-		this.offset = offset;
+		this.offset = newOffset;
-		this.length = length;
+		this.length = newLength;
 	}
 
 	/**

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/InternalDataTypeComponent.java

@@ -15,6 +15,10 @@
  */
 package ghidra.program.model.data;
 
+import org.apache.commons.lang3.StringUtils;
+
+import ghidra.util.StringUtilities;
+
 public interface InternalDataTypeComponent extends DataTypeComponent {
 
 	/**
@@ -51,4 +55,19 @@ public interface InternalDataTypeComponent extends DataTypeComponent {
 		return buffer.toString();
 	}
 
+	/**
+	 * Internal method for cleaning up field names. 
+	 * @param name the new field name
+	 * @return the name with bad chars removed and also set back to null in the event
+	 * the new name is the default name.
+	 */
+	public default String cleanupFieldName(String name) {
+		// For now, silently convert whitespace to underscores
+		String fieldName = StringUtilities.whitespaceToUnderscores(name);
+		if (StringUtils.isBlank(fieldName) || fieldName.equals(getDefaultFieldName())) {
+			fieldName = null;
+		}
+		return fieldName;
+	}
+
 }

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/data/StructureDBTest.java

@@ -26,6 +26,7 @@ import com.google.common.collect.Sets;
 import generic.test.AbstractGenericTest;
 import ghidra.program.model.data.*;
 import ghidra.util.InvalidNameException;
+import ghidra.util.exception.DuplicateNameException;
 import ghidra.util.task.TaskMonitor;
 import ghidra.util.task.TaskMonitorAdapter;
 
@@ -1476,8 +1477,8 @@ public class StructureDBTest extends AbstractGenericTest {
 		assertEquals(2, struct.getNumDefinedComponents());
 
 		len /= 2;
-		struct.replaceAtOffset(len-2, WordDataType.dataType, -1, "x", null); // will be preserved below
+		struct.replaceAtOffset(len - 2, WordDataType.dataType, -1, "x", null); // will be preserved below
-		struct.replaceAtOffset(len+2, WordDataType.dataType, -1, "y", null); // will be cleared below
+		struct.replaceAtOffset(len + 2, WordDataType.dataType, -1, "y", null); // will be cleared below
 		struct.setLength(len);
 		assertEquals(len, struct.getLength());
 		assertEquals(len - 2, struct.getNumComponents());
@@ -2616,4 +2617,23 @@ public class StructureDBTest extends AbstractGenericTest {
 		}
 	}
 
+	@Test
+	public void testFieldNameWhitespaceConvertedToUnderscores() throws DuplicateNameException {
+		StructureDataType newStruct = new StructureDataType("Test", 0);
+		DataTypeComponent component = newStruct.add(new ByteDataType(), " name with spaces", null);
+		assertEquals("name_with_spaces", component.getFieldName());
+
+		struct = (StructureDB) dataMgr.resolve(newStruct, null);
+		component = struct.getComponent(0);
+		component.setFieldName("name in db with spaces");
+		assertEquals("name_in_db_with_spaces", component.getFieldName());
+
+		component = struct.add(new ByteDataType(), "another test", null);
+		assertEquals("another_test", component.getFieldName());
+
+		struct.insert(0, new ByteDataType(), 1, "insert test", "");
+		component = struct.getComponent(0);
+		assertEquals("insert_test", component.getFieldName());
+	}
+
 }