GT-3126 corrected FileBytes issue with undo/redo. Also corrected

FileBytes bug which could result in empty DBBuffer.

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
 	 */

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

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)

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

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<FileBytes> 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) {

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

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++) {

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