From cbd270cec2f5a4b9836eec07417c103e2e08d2a0 Mon Sep 17 00:00:00 2001 From: ghidra1 Date: Fri, 30 Aug 2019 15:18:33 -0400 Subject: [PATCH] GT-3126 corrected FileBytes issue with undo/redo. Also corrected FileBytes bug which could result in empty DBBuffer. --- .../DB/src/main/java/db/ChainedBuffer.java | 9 +- .../program/database/mem/FileBytes.java | 91 +++++++++++-------- .../database/mem/FileBytesAdapter.java | 27 +++--- .../database/mem/FileBytesAdapterNoTable.java | 4 +- .../database/mem/FileBytesAdapterV0.java | 36 +++++--- .../program/database/mem/MemoryMapDB.java | 4 +- .../program/database/mem/FileBytesTest.java | 57 ++++++++++++ .../program/database/mem/MemBlockDBTest.java | 2 +- 8 files changed, 157 insertions(+), 73 deletions(-) diff --git a/Ghidra/Framework/DB/src/main/java/db/ChainedBuffer.java b/Ghidra/Framework/DB/src/main/java/db/ChainedBuffer.java index 141be01229..9e69467d86 100644 --- a/Ghidra/Framework/DB/src/main/java/db/ChainedBuffer.java +++ b/Ghidra/Framework/DB/src/main/java/db/ChainedBuffer.java @@ -115,7 +115,7 @@ public class ChainedBuffer implements Buffer { * Construct a new chained buffer with optional obfuscation and uninitialized data source. * This method may only be invoked while a database transaction * is in progress. - * @param size buffer size + * @param size buffer size (0 < size <= 0x7fffffff) * @param enableObfuscation true to enable xor-ing of stored data to facilitate data obfuscation. * @param uninitializedDataSource optional data source for uninitialized data. This should be a * read-only buffer which will always be used when re-instantiating the same stored ChainedBuffer. @@ -131,6 +131,9 @@ public class ChainedBuffer implements Buffer { this.size = size; this.useXORMask = enableObfuscation; + if (size == 0) { + throw new IllegalArgumentException("Zero length buffer not permitted"); + } if (size < 0) { throw new IllegalArgumentException( "Maximum bufer size is " + Integer.MAX_VALUE + "; given size of " + size); @@ -165,7 +168,7 @@ public class ChainedBuffer implements Buffer { * Construct a new chained buffer with optional obfuscation. * This method may only be invoked while a database transaction * is in progress. - * @param size buffer size + * @param size buffer size (0 < size <= 0x7fffffff) * @param enableObfuscation true to enable xor-ing of stored data to facilitate data obfuscation. * @param bufferMgr database buffer manager * @throws IOException @@ -178,7 +181,7 @@ public class ChainedBuffer implements Buffer { /** * Construct a new chained buffer. * This method may only be invoked while a database transaction is in progress. - * @param size buffer size + * @param size buffer size (0 < size <= 0x7fffffff) * @param bufferMgr database buffer manager * @throws IOException */ diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytes.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytes.java index 8b4777043c..d3941761c5 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytes.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytes.java @@ -18,6 +18,8 @@ package ghidra.program.database.mem; import java.io.IOException; import java.util.ConcurrentModificationException; +import org.apache.commons.lang3.StringUtils; + import db.*; /** @@ -26,22 +28,35 @@ import db.*; */ public class FileBytes { - private final DBBuffer[] originalBuffers; - private final DBBuffer[] layeredBuffers; - private final String filename; + final FileBytesAdapter adapter; + private final long id; + private final String filename; private final long fileOffset; private final long size; - private boolean invalid = false; - private MemoryMapDB memMap; - public FileBytes(FileBytesAdapter adapter, MemoryMapDB memMap, Record record) - throws IOException { - this.memMap = memMap; + private DBBuffer[] originalBuffers; + private DBBuffer[] layeredBuffers; + private boolean invalid = false; + + public FileBytes(FileBytesAdapter adapter, Record record) throws IOException { + this.adapter = adapter; + this.id = record.getKey(); this.filename = record.getString(FileBytesAdapter.FILENAME_COL); this.fileOffset = record.getLongValue(FileBytesAdapter.OFFSET_COL); this.size = record.getLongValue(FileBytesAdapter.SIZE_COL); - this.id = record.getKey(); + refresh(record); + } + + synchronized boolean refresh(Record record) throws IOException { + + String f = record.getString(FileBytesAdapter.FILENAME_COL); + long offset = record.getLongValue(FileBytesAdapter.OFFSET_COL); + long sz = record.getLongValue(FileBytesAdapter.SIZE_COL); + if (offset != fileOffset || sz != size || !StringUtils.equals(f, filename)) { + return false; + } + BinaryField field = (BinaryField) record.getFieldValue(FileBytesAdapter.BUF_IDS_COL); int[] bufferIds = new BinaryCodedField(field).getIntArray(); @@ -56,7 +71,11 @@ public class FileBytes { for (int i = 0; i < bufferIds.length; i++) { layeredBuffers[i] = adapter.getBuffer(bufferIds[i], originalBuffers[i]); } + return true; + } + long getId() { + return id; } /** @@ -93,7 +112,7 @@ public class FileBytes { * @throws IOException if there is a problem reading the database. * @throws IndexOutOfBoundsException if the given offset is invalid. */ - public byte getModifiedByte(long offset) throws IOException { + public synchronized byte getModifiedByte(long offset) throws IOException { return getByte(layeredBuffers, offset); } @@ -104,7 +123,7 @@ public class FileBytes { * @throws IOException if there is a problem reading the database. * @throws IndexOutOfBoundsException if the given offset is invalid. */ - public byte getOriginalByte(long offset) throws IOException { + public synchronized byte getOriginalByte(long offset) throws IOException { return getByte(originalBuffers, offset); } @@ -117,7 +136,7 @@ public class FileBytes { * @return the number of bytes actually populated. * @throws IOException if there is an error reading from the database */ - public int getModifiedBytes(long offset, byte[] b) throws IOException { + public synchronized int getModifiedBytes(long offset, byte[] b) throws IOException { return getBytes(layeredBuffers, offset, b, 0, b.length); } @@ -130,7 +149,7 @@ public class FileBytes { * @return the number of bytes actually populated. * @throws IOException if there is an error reading from the database */ - public int getOriginalBytes(long offset, byte[] b) throws IOException { + public synchronized int getOriginalBytes(long offset, byte[] b) throws IOException { return getBytes(originalBuffers, offset, b, 0, b.length); } @@ -148,7 +167,8 @@ public class FileBytes { * @throws IndexOutOfBoundsException if the destination offset and length would exceed the * size of the buffer b. */ - public int getModifiedBytes(long offset, byte[] b, int off, int length) throws IOException { + public synchronized int getModifiedBytes(long offset, byte[] b, int off, int length) + throws IOException { return getBytes(layeredBuffers, offset, b, off, length); } @@ -166,24 +186,21 @@ public class FileBytes { * @throws IndexOutOfBoundsException if the destination offset and length would exceed the * size of the buffer b. */ - public int getOriginalBytes(long offset, byte[] b, int off, int length) throws IOException { + public synchronized int getOriginalBytes(long offset, byte[] b, int off, int length) + throws IOException { return getBytes(originalBuffers, offset, b, off, length); } - void checkValid() { + synchronized void checkValid() { if (invalid) { throw new ConcurrentModificationException(); } } - void invalidate() { + synchronized void invalidate() { invalid = true; } - long getId() { - return id; - } - /** * Changes the byte at the given offset to the given value. Note, the * original byte can still be accessed via {@link #getOriginalByte(long)} @@ -193,13 +210,14 @@ public class FileBytes { * @param b the new byte value; * @throws IOException if the write to the database fails. */ - void putByte(long offset, byte b) throws IOException { + synchronized void putByte(long offset, byte b) throws IOException { + + checkValid(); + if (offset < 0 || offset >= size) { throw new IndexOutOfBoundsException(); } - checkValid(); - // The max buffer size will be the size of the first buffer. (If more than // one buffer exists, then the first buffer will be the true max size. If only one buffer, // then its actual size can be used as the max size and it won't matter.) @@ -220,7 +238,7 @@ public class FileBytes { * @return the number of bytes written * @throws IOException if the write to the database fails. */ - int putBytes(long offset, byte[] b) throws IOException { + synchronized int putBytes(long offset, byte[] b) throws IOException { return putBytes(offset, b, 0, b.length); } @@ -236,7 +254,10 @@ public class FileBytes { * @return the number of bytes written * @throws IOException if the write to the database fails. */ - int putBytes(long offset, byte[] b, int off, int length) throws IOException { + synchronized int putBytes(long offset, byte[] b, int off, int length) throws IOException { + + checkValid(); + if (b == null) { throw new NullPointerException(); } @@ -247,8 +268,6 @@ public class FileBytes { return 0; } - checkValid(); - // adjust size if asking length is more than we have length = (int) Math.min(length, size - offset); if (length == 0) { @@ -276,12 +295,13 @@ public class FileBytes { } private byte getByte(DBBuffer[] buffers, long offset) throws IOException { + + checkValid(); + if (offset < 0 || offset >= size) { throw new IndexOutOfBoundsException(); } - checkValid(); - // The max buffer size will be the size of the first buffer. (If more than // one buffer exists, then the first buffer will be the true max size. If only one buffer, // then its actual size can be used as the max size and it won't matter.) @@ -295,6 +315,8 @@ public class FileBytes { private int getBytes(DBBuffer[] buffers, long offset, byte[] b, int off, int length) throws IOException { + checkValid(); + if (off < 0 || length < 0 || length > b.length - off) { throw new IndexOutOfBoundsException(); } @@ -302,8 +324,6 @@ public class FileBytes { return 0; } - checkValid(); - // adjust size if asking length is more than we have length = (int) Math.min(length, size - offset); if (length == 0) { @@ -332,7 +352,7 @@ public class FileBytes { @Override public String toString() { - return filename; + return getFilename(); } @Override @@ -349,7 +369,7 @@ public class FileBytes { if (getClass() != obj.getClass()) return false; FileBytes other = (FileBytes) obj; - if (memMap != other.memMap) + if (adapter != other.adapter) return false; if (id != other.id) return false; @@ -358,7 +378,4 @@ public class FileBytes { return true; } - MemoryMapDB getMemMap() { - return memMap; - } } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapter.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapter.java index ecb260735b..5cb722caea 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapter.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapter.java @@ -27,7 +27,7 @@ import ghidra.util.task.TaskMonitor; * Database Adapter for storing and retrieving original file bytes. */ abstract class FileBytesAdapter { - private static final int MAX_BUF_SIZE = 1_000_000_000; + static final int MAX_BUF_SIZE = 1_000_000_000; public static final int FILENAME_COL = FileBytesAdapterV0.V0_FILENAME_COL; public static final int OFFSET_COL = FileBytesAdapterV0.V0_OFFSET_COL; @@ -40,40 +40,39 @@ abstract class FileBytesAdapter { protected MemoryMapDB memMap; - FileBytesAdapter(DBHandle handle, MemoryMapDB memMap) { + FileBytesAdapter(DBHandle handle) { this.handle = handle; - this.memMap = memMap; } - static FileBytesAdapter getAdapter(DBHandle handle, int openMode, MemoryMapDB memMap, - TaskMonitor monitor) throws VersionException, IOException { + static FileBytesAdapter getAdapter(DBHandle handle, int openMode, TaskMonitor monitor) + throws VersionException, IOException { if (openMode == DBConstants.CREATE) { - return new FileBytesAdapterV0(handle, memMap, true); + return new FileBytesAdapterV0(handle, true); } try { - return new FileBytesAdapterV0(handle, memMap, false); + return new FileBytesAdapterV0(handle, false); } catch (VersionException e) { if (!e.isUpgradable() || openMode == DBConstants.UPDATE) { throw e; } - FileBytesAdapter adapter = findReadOnlyAdapter(handle, memMap); + FileBytesAdapter adapter = findReadOnlyAdapter(handle); if (openMode == DBConstants.UPGRADE) { - adapter = upgrade(handle, memMap, adapter, monitor); + adapter = upgrade(handle, adapter, monitor); } return adapter; } } - private static FileBytesAdapter findReadOnlyAdapter(DBHandle handle, MemoryMapDB memMap) { - return new FileBytesAdapterNoTable(handle, memMap); + private static FileBytesAdapter findReadOnlyAdapter(DBHandle handle) { + return new FileBytesAdapterNoTable(handle); } - private static FileBytesAdapter upgrade(DBHandle handle, MemoryMapDB memMap, - FileBytesAdapter oldAdapter, TaskMonitor monitor) throws VersionException, IOException { - return new FileBytesAdapterV0(handle, memMap, true); + private static FileBytesAdapter upgrade(DBHandle handle, FileBytesAdapter oldAdapter, + TaskMonitor monitor) throws VersionException, IOException { + return new FileBytesAdapterV0(handle, true); } abstract FileBytes createFileBytes(String filename, long offset, long size, InputStream is) diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterNoTable.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterNoTable.java index d111e784ab..d82f7159c3 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterNoTable.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterNoTable.java @@ -27,8 +27,8 @@ import db.DBHandle; */ class FileBytesAdapterNoTable extends FileBytesAdapter { - public FileBytesAdapterNoTable(DBHandle handle, MemoryMapDB memMap) { - super(handle, memMap); + public FileBytesAdapterNoTable(DBHandle handle) { + super(handle); } @Override diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterV0.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterV0.java index dce9b3f888..b737a4e9b3 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterV0.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/FileBytesAdapterV0.java @@ -45,9 +45,8 @@ class FileBytesAdapterV0 extends FileBytesAdapter { private Table table; private List fileBytesList = new ArrayList<>(); - FileBytesAdapterV0(DBHandle handle, MemoryMapDB memMap, boolean create) - throws VersionException, IOException { - super(handle, memMap); + FileBytesAdapterV0(DBHandle handle, boolean create) throws VersionException, IOException { + super(handle); if (create) { table = handle.createTable(TABLE_NAME, SCHEMA); @@ -66,7 +65,7 @@ class FileBytesAdapterV0 extends FileBytesAdapter { RecordIterator iterator = table.iterator(); while (iterator.hasNext()) { Record record = iterator.next(); - fileBytesList.add(new FileBytes(this, memMap, record)); + fileBytesList.add(new FileBytes(this, record)); } } @@ -85,7 +84,7 @@ class FileBytesAdapterV0 extends FileBytesAdapter { record.setField(V0_BUF_IDS_COL, new BinaryCodedField(bufIds)); record.setField(V0_LAYERED_BUF_IDS_COL, new BinaryCodedField(layeredBufIds)); table.putRecord(record); - FileBytes fileBytes = new FileBytes(this, memMap, record); + FileBytes fileBytes = new FileBytes(this, record); fileBytesList.add(fileBytes); return fileBytes; } @@ -108,8 +107,15 @@ class FileBytesAdapterV0 extends FileBytesAdapter { while (iterator.hasNext()) { Record record = iterator.next(); FileBytes fileBytes = map.remove(record.getKey()); + if (fileBytes != null) { + if (!fileBytes.refresh(record)) { + // FileBytes attributes changed + fileBytes.invalidate(); + fileBytes = null; + } + } if (fileBytes == null) { - fileBytes = new FileBytes(this, memMap, record); + fileBytes = new FileBytes(this, record); } newList.add(fileBytes); } @@ -148,16 +154,18 @@ class FileBytesAdapterV0 extends FileBytesAdapter { private DBBuffer[] createBuffers(long size, InputStream is) throws IOException { int maxBufSize = getMaxBufferSize(); - int bufCount = (int) (size / maxBufSize); - int sizeLastBuf = (int) (size % maxBufSize); - if (sizeLastBuf > 0) { - bufCount++; - } + int bufCount = (int) (size + maxBufSize - 1) / maxBufSize; + DBBuffer[] buffers = new DBBuffer[bufCount]; - for (int i = 0; i < bufCount - 1; i++) { - buffers[i] = handle.createBuffer(maxBufSize); + int bufSize = maxBufSize; + int lastBufSize = (int) (size % maxBufSize); + + for (int i = 0; i < bufCount; i++) { + if (lastBufSize != 0 && i == (bufCount - 1)) { + bufSize = lastBufSize; + } + buffers[i] = handle.createBuffer(bufSize); } - buffers[bufCount - 1] = handle.createBuffer(sizeLastBuf); try { for (DBBuffer buffer : buffers) { diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java index d9a6e732d4..394d6c4dfe 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java @@ -86,7 +86,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { this.lock = lock; defaultEndian = isBigEndian ? BIG_ENDIAN : LITTLE_ENDIAN; adapter = MemoryMapDBAdapter.getAdapter(handle, openMode, this, monitor); - fileBytesAdapter = FileBytesAdapter.getAdapter(handle, openMode, this, monitor); + fileBytesAdapter = FileBytesAdapter.getAdapter(handle, openMode, monitor); initializeBlocks(); buildAddressSets(); } @@ -2049,7 +2049,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { } private void checkFileBytes(FileBytes fileBytes) { - if (fileBytes.getMemMap() != this) { + if (fileBytes.adapter != fileBytesAdapter) { throw new IllegalArgumentException( "Attempted to delete FileBytes that doesn't belong to this program"); } diff --git a/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/FileBytesTest.java b/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/FileBytesTest.java index efdf8c8666..81108face6 100644 --- a/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/FileBytesTest.java +++ b/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/FileBytesTest.java @@ -37,7 +37,11 @@ import ghidra.util.task.TaskMonitor; public class FileBytesTest extends AbstractGenericTest { + // Use of small buffer size will not exercise use of indexed ChainedBuffer, + // therefor those tests which need to exercise this should use a size which + // exceeds 16-KBytes. private static final int MAX_BUFFER_SIZE_FOR_TESTING = 200; + private Program program; private Memory mem; private int transactionID; @@ -186,6 +190,59 @@ public class FileBytesTest extends AbstractGenericTest { } } + @Test + public void testGetLayeredBytesAfterUndo() throws Exception { + // NOTE: need to induce use of indexed ChainedBuffer + FileBytesAdapter.setMaxBufferSize(FileBytesAdapter.MAX_BUF_SIZE); + FileBytes fileBytes = createFileBytes("file1", 20000); + + program.endTransaction(transactionID, true); + transactionID = program.startTransaction("modify"); + + incrementFileBytes(fileBytes, 0, 10); + incrementFileBytes(fileBytes, 18999, 10); + + // undo layered buffer changes + program.endTransaction(transactionID, true); + program.undo(); + transactionID = program.startTransaction("resume"); + + // check that the layered bytes are unchanged from the originals + assertEquals(1, fileBytes.getOriginalByte(1)); + assertEquals(1, fileBytes.getModifiedByte(1)); + + byte b = (byte) 19000; + assertEquals(b, fileBytes.getOriginalByte(19000)); + assertEquals(b, fileBytes.getModifiedByte(19000)); + } + + @Test + public void testGetLayeredBytesAfterUndoRedo() throws Exception { + // NOTE: need to induce use of indexed ChainedBuffer + FileBytesAdapter.setMaxBufferSize(FileBytesAdapter.MAX_BUF_SIZE); + FileBytes fileBytes = createFileBytes("file1", 20000); + + program.endTransaction(transactionID, true); + transactionID = program.startTransaction("modify"); + + incrementFileBytes(fileBytes, 0, 10); + incrementFileBytes(fileBytes, 18999, 10); + + // undo layered buffer changes + program.endTransaction(transactionID, true); + program.undo(); + program.redo(); + transactionID = program.startTransaction("resume"); + + // check that the layered bytes are unchanged from the originals + assertEquals(1, fileBytes.getOriginalByte(1)); + assertEquals(2, fileBytes.getModifiedByte(1)); + + byte b = (byte) 19000; + assertEquals(b, fileBytes.getOriginalByte(19000)); + assertEquals((byte) (b + 1), fileBytes.getModifiedByte(19000)); + } + private FileBytes createFileBytes(String name, int size) throws Exception { byte[] bytes = new byte[size]; for (int i = 0; i < size; i++) { diff --git a/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/MemBlockDBTest.java b/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/MemBlockDBTest.java index 85a97d6764..c302821f28 100644 --- a/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/MemBlockDBTest.java +++ b/Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/mem/MemBlockDBTest.java @@ -64,7 +64,7 @@ public class MemBlockDBTest extends AbstractGenericTest { MemoryMapDBAdapter adapter = new MemoryMapDBAdapterV3(handle, mem, MAX_SUB_BLOCK_SIZE, true); - FileBytesAdapter fileBytesAdapter = new FileBytesAdapterV0(handle, mem, true); + FileBytesAdapter fileBytesAdapter = new FileBytesAdapterV0(handle, true); mem.init(adapter, fileBytesAdapter); mem.setProgram(program);