Skip to content

Commit

Permalink
GP-4814 Corrected severe bugs in StructureDB and UnionDB delete(Set
Browse files Browse the repository at this point in the history 
ordinals) method
  • Loading branch information
@ghidra1
ghidra1 committed Aug 2, 2024
1 parent 34ba255 commit ba0f16f
Show file tree
Hide file tree
Showing 5 changed files with 138 additions and 66 deletions.
9 changes: 7 additions & 2 deletions ...ra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -345,6 +345,11 @@ protected void doSetNameRecord(String name) throws IOException {
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
Expand Down
84 changes: 56 additions & 28 deletions ...ra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -544,20 +544,29 @@ public void delete(int ordinal) {
}

/**
* 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);
long compKey = dtc.getKey();
componentAdapter.removeRecord(compKey);
dataMgr.getSettingsAdapter().removeAllSettingsRecords(compKey);
doDelete(dtc);
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>
Expand Down Expand Up @@ -587,38 +596,53 @@ private void doDeleteWithComponentShift(int index, boolean disableOffsetShift) {

@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()) {
return;
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");
}

boolean bitFieldRemoved = false;

TreeSet<Integer> treeSet = null;
if (!isPackingEnabled()) {
// treeSet only used to track undefined filler removal
treeSet = new TreeSet<>(ordinals);
}
Integer nextOrdinal = firstOrdinal;

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
Expand All @@ -627,8 +651,10 @@ public void delete(Set<Integer> ordinals) {
else {
offsetAdjustment -= dtc.getLength();
}
doDelete(dtc); // delete defined component record
--ordinalAdjustment;
lastDefinedOrdinal = ordinal;
nextOrdinal = sortedOrdinals.higher(ordinal);
}
else {
if (ordinalAdjustment != 0) {
Expand All @@ -638,10 +664,10 @@ public void delete(Set<Integer> ordinals) {
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;
Expand All @@ -653,7 +679,7 @@ public void delete(Set<Integer> ordinals) {
components = newComponents;
updateComposite(numComponents + ordinalAdjustment, -1, -1, true);

if (isPackingEnabled()) {
if (isPacked) {
if (!repack(false, true)) {
dataMgr.dataTypeChanged(this, false);
}
Expand All @@ -667,6 +693,9 @@ public void delete(Set<Integer> ordinals) {
notifySizeChanged(false);
}
}
catch (IOException e) {
dataMgr.dbError(e);
}
finally {
lock.release();
}
Expand Down Expand Up @@ -1684,9 +1713,9 @@ private void doReplaceWithPacked(Structure struct, DataType[] resolvedDts) {

private void doReplaceWithNonPacked(Structure struct, DataType[] resolvedDts)
throws IOException {

// caller responsible for record updates

// assumes components is clear and that alignment characteristics have been set.
if (struct.isNotYetDefined()) {
return;
Expand Down Expand Up @@ -2486,9 +2515,8 @@ private boolean updateComposite(int currentNumComponents, int currentLength,
record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength);
compositeChanged = true;
}
if (currentAlignment >= 0 &&
(currentAlignment != structAlignment || currentAlignment != record
.getIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL))) {
if (currentAlignment >= 0 && (currentAlignment != structAlignment ||
currentAlignment != record.getIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL))) {
structAlignment = currentAlignment;
record.setIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL, structAlignment);
compositeChanged = true;
Expand Down
56 changes: 37 additions & 19 deletions Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/UnionDB.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -159,16 +159,6 @@ private DataTypeComponentDB createComponent(long dtID, int length, int ordinal,
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 {
Expand Down Expand Up @@ -234,24 +224,33 @@ public void delete(int ordinal) {

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();
}
}

@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();
Expand All @@ -266,7 +265,9 @@ public void delete(Set<Integer> ordinals) {
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 {
Expand All @@ -285,10 +286,24 @@ public void delete(Set<Integer> ordinals) {
}
}
else {
unionLength = newLength;
notifySizeChanged(false);
boolean sizeChanged = (unionLength != newLength);
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();
}
Expand Down Expand Up @@ -332,7 +347,7 @@ void doReplaceWith(UnionInternal union, boolean notify)

for (DataTypeComponentDB dtc : components) {
dtc.getDataType().removeParent(this);
removeComponent(dtc.getKey());
removeComponentRecord(dtc.getKey());
}
components.clear();
unionAlignment = -1;
Expand Down Expand Up @@ -708,7 +723,7 @@ public void dataTypeDeleted(DataType dt) {
if (removeBitFieldComponent || dtc.getDataType() == dt) {
dt.removeParent(this);
components.remove(i);
removeComponent(dtc.getKey());
removeComponentRecord(dtc.getKey());
shiftOrdinals(i, -1);
changed = true;
}
Expand All @@ -717,6 +732,9 @@ public void dataTypeDeleted(DataType dt) {
dataMgr.dataTypeChanged(this, false);
}
}
catch (IOException e) {
dataMgr.dbError(e);
}
finally {
lock.release();
}
Expand Down Expand Up @@ -853,7 +871,7 @@ else if (dtc.getDataType() == oldDt) {
if (remove) {
oldDt.removeParent(this);
components.remove(i);
removeComponent(dtc.getKey());
removeComponentRecord(dtc.getKey());
shiftOrdinals(i, -1);
changed = true;
}
Expand Down
Loading

0 comments on commit ba0f16f

Please sign in to comment.