GP-4814 Corrected severe bugs in StructureDB and UnionDB delete(Set

ordinals) method

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

@@ -345,6 +345,11 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 		compositeAdapter.updateRecord(record, true);
 	}
 
+	protected void removeComponentRecord(long compKey) throws IOException {
+		componentAdapter.removeRecord(compKey);
+		dataMgr.getSettingsAdapter().removeAllSettingsRecords(compKey);
+	}
+
 	/**
 	 * This method throws an exception if the indicated data type is not a valid
 	 * data type for a component of this composite data type.  If the DEFAULT 

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

@@ -544,20 +544,29 @@ class StructureDB extends CompositeDB implements StructureInternal {
 	}
 
 	/**
-	 * Removes a defined component at the specified index without any alteration to other components.
+	 * Removes a defined component at the specified index from the components list without any 
+	 * alteration to other components and removes parent association for component datatype.
 	 * @param index defined component index
 	 * @return the defined component which was removed.
 	 * @throws IOException if an IO error occurs
 	 */
 	private DataTypeComponentDB doDelete(int index) throws IOException {
 		DataTypeComponentDB dtc = components.remove(index);
-		dtc.getDataType().removeParent(this);
+		doDelete(dtc);
-		long compKey = dtc.getKey();
-		componentAdapter.removeRecord(compKey);
-		dataMgr.getSettingsAdapter().removeAllSettingsRecords(compKey);
 		return dtc;
 	}
 
+	/**
+	 * Removes a defined component without any alteration to other components or the components 
+	 * list and removes parent association for component datatype.
+	 * @param dtc datatype component
+	 * @throws IOException if an IO error occurs
+	 */
+	private void doDelete(DataTypeComponentDB dtc) throws IOException {
+		dtc.getDataType().removeParent(this);
+		removeComponentRecord(dtc.getKey());
+	}
+
 	/**
 	 * Removes a defined component at the specified index.
 	 * <p>
@@ -587,38 +596,53 @@ class StructureDB extends CompositeDB implements StructureInternal {
 
 	@Override
 	public void delete(Set<Integer> ordinals) {
+
+		if (ordinals.isEmpty()) {
+			return;
+		}
+
+		if (ordinals.size() == 1) {
+			ordinals.forEach(ordinal -> delete(ordinal));
+			return;
+		}
+
 		lock.acquire();
 		try {
 			checkDeleted();
 
-			if (ordinals.isEmpty()) {
+			TreeSet<Integer> sortedOrdinals = new TreeSet<>(ordinals);
-				return;
+			int firstOrdinal = sortedOrdinals.first();
+			int lastOrdinal = sortedOrdinals.last();
+			if (firstOrdinal < 0 || lastOrdinal >= numComponents) {
+				throw new IndexOutOfBoundsException(ordinals.size() + " ordinals specified");
 			}
 
-			boolean bitFieldRemoved = false;
+			Integer nextOrdinal = firstOrdinal;
 
-			TreeSet<Integer> treeSet = null;
-			if (!isPackingEnabled()) {
-				// treeSet only used to track undefined filler removal
-				treeSet = new TreeSet<>(ordinals);
-			}
-
-			List<DataTypeComponentDB> newComponents = new ArrayList<>();
 			int ordinalAdjustment = 0;
 			int offsetAdjustment = 0;
 			int lastDefinedOrdinal = -1;
+
+			boolean isPacked = isPackingEnabled();
+
+			boolean bitFieldRemoved = false;
+
+			List<DataTypeComponentDB> newComponents = new ArrayList<>(components.size());
+
 			for (DataTypeComponentDB dtc : components) {
 				int ordinal = dtc.getOrdinal();
-				if (treeSet != null && lastDefinedOrdinal < (ordinal - 1)) {
+				if (!isPacked && nextOrdinal != null && nextOrdinal < ordinal) {
 					// Identify removed filler since last defined component
-					Set<Integer> removedFillerSet = treeSet.subSet(lastDefinedOrdinal + 1, ordinal);
+					SortedSet<Integer> removedFillerSet =
+						sortedOrdinals.subSet(lastDefinedOrdinal + 1, ordinal);
 					if (!removedFillerSet.isEmpty()) {
 						int undefinedRemoveCount = removedFillerSet.size();
 						ordinalAdjustment -= undefinedRemoveCount;
 						offsetAdjustment -= undefinedRemoveCount;
+						nextOrdinal = sortedOrdinals.higher(removedFillerSet.last());
 					}
 				}
-				if (ordinals.contains(ordinal)) {
+				if (nextOrdinal != null && nextOrdinal == ordinal) {
 					// defined component removed
 					if (dtc.isBitFieldComponent()) {
 						// defer reconciling bitfield space to repack
@@ -627,8 +651,10 @@ class StructureDB extends CompositeDB implements StructureInternal {
 					else {
 						offsetAdjustment -= dtc.getLength();
 					}
+					doDelete(dtc); // delete defined component record
 					--ordinalAdjustment;
 					lastDefinedOrdinal = ordinal;
+					nextOrdinal = sortedOrdinals.higher(ordinal);
 				}
 				else {
 					if (ordinalAdjustment != 0) {
@@ -638,10 +664,10 @@ class StructureDB extends CompositeDB implements StructureInternal {
 					lastDefinedOrdinal = ordinal;
 				}
 			}
-			if (treeSet != null) {
+			if (!isPacked) {
 				// Identify removed filler after last defined component
 				Set<Integer> removedFillerSet =
-					treeSet.subSet(lastDefinedOrdinal + 1, numComponents);
+					sortedOrdinals.subSet(lastDefinedOrdinal + 1, numComponents);
 				if (!removedFillerSet.isEmpty()) {
 					int undefinedRemoveCount = removedFillerSet.size();
 					ordinalAdjustment -= undefinedRemoveCount;
@@ -653,7 +679,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 			components = newComponents;
 			updateComposite(numComponents + ordinalAdjustment, -1, -1, true);
 
-			if (isPackingEnabled()) {
+			if (isPacked) {
 				if (!repack(false, true)) {
 					dataMgr.dataTypeChanged(this, false);
 				}
@@ -667,6 +693,9 @@ class StructureDB extends CompositeDB implements StructureInternal {
 				notifySizeChanged(false);
 			}
 		}
+		catch (IOException e) {
+			dataMgr.dbError(e);
+		}
 		finally {
 			lock.release();
 		}
@@ -2486,9 +2515,8 @@ class StructureDB extends CompositeDB implements StructureInternal {
 			record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength);
 			compositeChanged = true;
 		}
-		if (currentAlignment >= 0 &&
+		if (currentAlignment >= 0 && (currentAlignment != structAlignment ||
-			(currentAlignment != structAlignment || currentAlignment != record
+			currentAlignment != record.getIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL))) {
-					.getIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL))) {
 			structAlignment = currentAlignment;
 			record.setIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL, structAlignment);
 			compositeChanged = true;

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

@@ -159,16 +159,6 @@ class UnionDB extends CompositeDB implements UnionInternal {
 		return null;
 	}
 
-	private void removeComponent(long compKey) {
-		try {
-			componentAdapter.removeRecord(compKey);
-			dataMgr.getSettingsAdapter().removeAllSettingsRecords(compKey);
-		}
-		catch (IOException e) {
-			dataMgr.dbError(e);
-		}
-	}
-
 	@Override
 	public DataTypeComponent insert(int ordinal, DataType dataType, int length, String name,
 			String comment) throws IllegalArgumentException {
@@ -234,13 +224,16 @@ class UnionDB extends CompositeDB implements UnionInternal {
 
 			DataTypeComponentDB dtc = components.remove(ordinal);
 			dtc.getDataType().removeParent(this);
-			removeComponent(dtc.getKey());
+			removeComponentRecord(dtc.getKey());
 			shiftOrdinals(ordinal, -1);
 
 			if (!repack(false, true)) {
 				dataMgr.dataTypeChanged(this, false);
 			}
 		}
+		catch (IOException e) {
+			dataMgr.dbError(e);
+		}
 		finally {
 			lock.release();
 		}
@@ -248,10 +241,16 @@ class UnionDB extends CompositeDB implements UnionInternal {
 
 	@Override
 	public void delete(Set<Integer> ordinals) {
+
 		if (ordinals.isEmpty()) {
 			return;
 		}
 
+		if (ordinals.size() == 1) {
+			ordinals.forEach(ordinal -> delete(ordinal));
+			return;
+		}
+
 		lock.acquire();
 		try {
 			checkDeleted();
@@ -266,7 +265,9 @@ class UnionDB extends CompositeDB implements UnionInternal {
 			for (DataTypeComponentDB dtc : components) {
 				int ordinal = dtc.getOrdinal();
 				if (ordinals.contains(ordinal)) {
-					// component removed
+					// component removed - delete record
+					dtc.getDataType().removeParent(this);
+					removeComponentRecord(dtc.getKey());
 					--ordinalAdjustment;
 				}
 				else {
@@ -285,10 +286,24 @@ class UnionDB extends CompositeDB implements UnionInternal {
 				}
 			}
 			else {
-				unionLength = newLength;
+				boolean sizeChanged = (unionLength != newLength);
-				notifySizeChanged(false);
+				if (sizeChanged) {
+					unionLength = newLength;
+					record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, unionLength);
+				}
+				compositeAdapter.updateRecord(record, true);
+
+				if (sizeChanged) {
+					notifySizeChanged(false);
+				}
+				else {
+					dataMgr.dataTypeChanged(this, false);
+				}
 			}
 		}
+		catch (IOException e) {
+			dataMgr.dbError(e);
+		}
 		finally {
 			lock.release();
 		}
@@ -332,7 +347,7 @@ class UnionDB extends CompositeDB implements UnionInternal {
 
 		for (DataTypeComponentDB dtc : components) {
 			dtc.getDataType().removeParent(this);
-			removeComponent(dtc.getKey());
+			removeComponentRecord(dtc.getKey());
 		}
 		components.clear();
 		unionAlignment = -1;
@@ -708,7 +723,7 @@ class UnionDB extends CompositeDB implements UnionInternal {
 				if (removeBitFieldComponent || dtc.getDataType() == dt) {
 					dt.removeParent(this);
 					components.remove(i);
-					removeComponent(dtc.getKey());
+					removeComponentRecord(dtc.getKey());
 					shiftOrdinals(i, -1);
 					changed = true;
 				}
@@ -717,6 +732,9 @@ class UnionDB extends CompositeDB implements UnionInternal {
 				dataMgr.dataTypeChanged(this, false);
 			}
 		}
+		catch (IOException e) {
+			dataMgr.dbError(e);
+		}
 		finally {
 			lock.release();
 		}
@@ -853,7 +871,7 @@ class UnionDB extends CompositeDB implements UnionInternal {
 				if (remove) {
 					oldDt.removeParent(this);
 					components.remove(i);
-					removeComponent(dtc.getKey());
+					removeComponentRecord(dtc.getKey());
 					shiftOrdinals(i, -1);
 					changed = true;
 				}

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

@@ -348,30 +348,44 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 			return;
 		}
 
-		boolean bitFieldRemoved = false;
+		if (ordinals.size() == 1) {
-
+			ordinals.forEach(ordinal -> delete(ordinal));
-		TreeSet<Integer> treeSet = null;
+			return;
-		if (!isPackingEnabled()) {
-			// treeSet only used to track undefined filler removal
-			treeSet = new TreeSet<>(ordinals);
 		}
 
-		List<DataTypeComponentImpl> newComponents = new ArrayList<>();
+		TreeSet<Integer> sortedOrdinals = new TreeSet<>(ordinals);
+		int firstOrdinal = sortedOrdinals.first();
+		int lastOrdinal = sortedOrdinals.last();
+		if (firstOrdinal < 0 || lastOrdinal >= numComponents) {
+			throw new IndexOutOfBoundsException(ordinals.size() + " ordinals specified");
+		}
+
+		Integer nextOrdinal = firstOrdinal;
+
 		int ordinalAdjustment = 0;
 		int offsetAdjustment = 0;
 		int lastDefinedOrdinal = -1;
+
+		boolean isPacked = isPackingEnabled();
+
+		boolean bitFieldRemoved = false;
+
+		List<DataTypeComponentImpl> newComponents = new ArrayList<>(components.size());
+
 		for (DataTypeComponentImpl dtc : components) {
 			int ordinal = dtc.getOrdinal();
-			if (treeSet != null && lastDefinedOrdinal < (ordinal - 1)) {
+			if (!isPacked && nextOrdinal != null && nextOrdinal < ordinal) {
 				// Identify removed filler since last defined component
-				Set<Integer> removedFillerSet = treeSet.subSet(lastDefinedOrdinal + 1, ordinal);
+				SortedSet<Integer> removedFillerSet =
+					sortedOrdinals.subSet(lastDefinedOrdinal + 1, ordinal);
 				if (!removedFillerSet.isEmpty()) {
 					int undefinedRemoveCount = removedFillerSet.size();
 					ordinalAdjustment -= undefinedRemoveCount;
 					offsetAdjustment -= undefinedRemoveCount;
+					nextOrdinal = sortedOrdinals.higher(removedFillerSet.last());
 				}
 			}
-			if (ordinals.contains(ordinal)) {
+			if (nextOrdinal != null && nextOrdinal == ordinal) {
 				// defined component removed
 				if (dtc.isBitFieldComponent()) {
 					// defer reconciling bitfield space to repack
@@ -382,6 +396,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 				}
 				--ordinalAdjustment;
 				lastDefinedOrdinal = ordinal;
+				nextOrdinal = sortedOrdinals.higher(ordinal);
 			}
 			else {
 
@@ -392,9 +407,10 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 				lastDefinedOrdinal = ordinal;
 			}
 		}
-		if (treeSet != null) {
+		if (!isPacked) {
 			// Identify removed filler after last defined component
-			Set<Integer> removedFillerSet = treeSet.subSet(lastDefinedOrdinal + 1, numComponents);
+			Set<Integer> removedFillerSet =
+				sortedOrdinals.subSet(lastDefinedOrdinal + 1, numComponents);
 			if (!removedFillerSet.isEmpty()) {
 				int undefinedRemoveCount = removedFillerSet.size();
 				ordinalAdjustment -= undefinedRemoveCount;
@@ -405,7 +421,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 		components = newComponents;
 		numComponents += ordinalAdjustment;
 
-		if (isPackingEnabled()) {
+		if (isPacked) {
 			repack(true);
 		}
 		else {

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

@@ -286,6 +286,11 @@ public class UnionDataType extends CompositeDataTypeImpl implements UnionInterna
 			return;
 		}
 
+		if (ordinals.size() == 1) {
+			ordinals.forEach(ordinal -> delete(ordinal));
+			return;
+		}
+
 		int oldAlignment = getAlignment();
 
 		List<DataTypeComponentImpl> newComponents = new ArrayList<>();
@@ -312,7 +317,7 @@ public class UnionDataType extends CompositeDataTypeImpl implements UnionInterna
 				notifyAlignmentChanged();
 			}
 		}
-		else {
+		else if (unionLength != newLength) {
 			unionLength = newLength;
 			notifySizeChanged();
 		}