remove old error bookmark

refactor to more correct API call
fix address range bug
refactor find already set length to default search and short-circuit
Correct error message to be more appropriate for all cases
Only suggest properly aligned offsets
Don't consider the first address
bugfix, iterator can be null. Check all references.
Implement fixed suggestions to avoid getNextInstruction and ClearCmd
usage.
switch to isFlow test
make easier to read
implement flow check
implement script message
strict variable rename suggestion
fix location of check and address mark
typo/grammar catch
Simplify as requested
add length check and fix a few iteration bugs
suggest offcut length if override doesn't already exist
comment is overcomplicated for a previous explanation
this comment was no longer true
explain this as what is actually happening now
other summary fixups
add suggestion
Length override action should disassemble the resulting instructions
remove info alert that isn't needed since there is a bookmark
remove unused check
Only init once per run
refactor to flow the code
Don't let a failed fix block the rest of the script iteration
Replace print with Msg statements
Provide more thorough error reporting
imply to user that work is still being done
apply language suggestion
check alignment and fix max address bug
restrict this again
remove unused memory handle
Refactor to use declared types
Simplify disassembly and computations
pass offcutAddress instead of recomputing some other one.
decrease level of specificity and disqualify.
apply suggestion
apply indentation suggestion
correct ref type filter to correct
Apply simplification
fix max address issue.
implemented suggestion with flow follow
update description to include location fallback info
Simplify bookmark
Add missing final

Ghidra/Features/Base/ghidra_scripts/FixOffcutInstructionScript.java

@@ -0,0 +1,184 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// This script looks for offcut instruction(s) in the current selection or location and
+// automatically fixes "safe" offcuts. This script is suitable for correcting polyglot instruction
+// executable size optimizations, LOCK prefix issues, and offcut code used for code obfuscation.
+// 
+// Offcuts are determined to be safe if they don't have additional conflicting offcuts in the same
+// base instruction.
+// The new instruction length override will be set by assuming there actually is an instruction at
+// the safe offcut reference. If a failure to flow this instruction occurs the script will emit
+// a warning about the exception and continue processing.
+// A check is done for pseudo-disassembly viability before setting the instruction or flowing
+// the code so these exceptions shouldn't be reached.
+//
+// When fixups are applied any existing Error level bookmarks for the Bad Instruction will be
+// removed and replaced with info that an offcut was fixed. These can be interpreted that
+// assumptions were made about the context flowed locally to the fixed instruction that should be
+// taken as fact cautiously since the binary is already confirmed to be well behaved, that is
+// strictly flowed.
+//
+//@category Analysis
+
+import ghidra.app.script.GhidraScript;
+import ghidra.app.script.ScriptMessage;
+import ghidra.app.util.PseudoDisassembler;
+import ghidra.app.util.PseudoInstruction;
+import ghidra.program.disassemble.Disassembler;
+import ghidra.program.model.address.Address;
+import ghidra.program.model.address.AddressSet;
+import ghidra.program.model.lang.*;
+import ghidra.program.model.listing.*;
+import ghidra.program.model.symbol.*;
+import ghidra.program.model.util.CodeUnitInsertionException;
+import ghidra.util.Msg;
+
+public class FixOffcutInstructionScript extends GhidraScript {
+
+	public final String INFO_BOOKMARK_CATEGORY = "Offcut Instruction";
+	public final String INFO_BOOKMARK_COMMENT = "Fixed offcut instruction";
+	public final int MAX_OFFCUT_DISTANCE = 64;
+	private Listing currentListing;
+	private BookmarkManager currentBookmarkManager;
+	private ReferenceManager currentReferenceManager;
+	private int alignment;
+
+	@Override
+	protected void run() throws Exception {
+		currentListing = currentProgram.getListing();
+		currentBookmarkManager = currentProgram.getBookmarkManager();
+		currentReferenceManager = currentProgram.getReferenceManager();
+		alignment = currentProgram.getLanguage().getInstructionAlignment();
+		// run in strict mode if a selection
+		final boolean doExtraValidation = currentSelection != null;
+
+		// restrict processing to the current selection
+		final AddressSet restrictedSet =
+			(currentSelection != null) ? (new AddressSet(currentSelection))
+					: (new AddressSet(currentLocation.getAddress()));
+
+		final InstructionIterator instrIter = currentListing.getInstructions(restrictedSet, true);
+
+		while (instrIter.hasNext() && !monitor.isCancelled()) {
+			final Instruction curInstr = instrIter.next();
+
+			if (curInstr.isLengthOverridden()) {
+				continue;
+			}
+
+			final Address offcutAddress = getQualifiedOffcutAddress(curInstr, doExtraValidation);
+			if (offcutAddress == null) {
+				continue;
+			}
+
+			// This script is only useful for static offcut instruction fixing. Dynamic offcuts
+			// will raise an exception that will be logged here.
+			try {
+				fixOffcutInstruction(curInstr, offcutAddress);
+			}
+			catch (Exception e) {
+				Msg.error(this, new ScriptMessage("Failed to fix offuct instruction at " +
+					curInstr.getAddressString(false, true)), e);
+			}
+		}
+	}
+
+	private Address getQualifiedOffcutAddress(final Instruction instr,
+			final boolean doExtraValidation) {
+		// short-circuit too small instructions
+		if (instr.getLength() < 2) {
+			return null;
+		}
+		final Address instrAddr = instr.getAddress();
+		final AddressSet instrBody =
+			new AddressSet(instr.getMinAddress().add(1), instr.getMaxAddress());
+		Address offcutAddress = null;
+		for (final Address address : currentReferenceManager
+				.getReferenceDestinationIterator(instrBody, true)) {
+			if ((address.getOffset() % alignment) != 0) {
+				continue;
+			}
+			for (final Reference reference : currentReferenceManager.getReferencesTo(address)) {
+				final RefType refType = reference.getReferenceType();
+				if (doExtraValidation && Math.abs(
+					instrAddr.subtract(reference.getFromAddress())) > MAX_OFFCUT_DISTANCE) {
+					continue;
+				}
+				if (refType.isJump() && refType.hasFallthrough()) {
+					if (offcutAddress == null) {
+						offcutAddress = address;
+					}
+				}
+				else {
+					continue;
+				}
+			}
+		}
+		return offcutAddress;
+	}
+
+	private void fixOffcutInstruction(Instruction instr, Address offcutAddress)
+			throws CodeUnitInsertionException {
+		if (!canDisassembleAt(instr, offcutAddress)) {
+			Msg.warn(this,
+				new ScriptMessage("\t> Offcut construction would not be valid. Skipping..."));
+			return;
+		}
+
+		instr.setLengthOverride((int) offcutAddress.subtract(instr.getMinAddress()));
+
+		// Once the override is complete there will be code to disassemble.
+		disassemble(offcutAddress);
+
+		// Usually there will be a bookmark complaining about how there is a well formed instruction
+		// already at this location which this change has obsoleted
+		fixBookmark(offcutAddress);
+	}
+
+	private void fixBookmark(Address at) {
+		final Bookmark bookmark = currentBookmarkManager.getBookmark(at, BookmarkType.ERROR,
+			Disassembler.ERROR_BOOKMARK_CATEGORY);
+		if (bookmark != null) {
+			currentBookmarkManager.removeBookmark(bookmark);
+
+			// inform the user this instruction was fixed. even though the disassembly appears
+			// fixed the fact remains that there are two potentially conflicting context flows
+			// happening at this instruction and it was assumed that the exposed instruction holds
+			// flow attention for execution here due to the direct references.
+			// team opted for a simple remark rather repeat this fact about context since
+			// this script being applied implies the user understands the potential for conflicts
+			currentBookmarkManager.setBookmark(at, BookmarkType.INFO, INFO_BOOKMARK_CATEGORY,
+				INFO_BOOKMARK_COMMENT);
+		}
+	}
+
+	protected boolean canDisassembleAt(Instruction instr, Address at) {
+		try {
+			// only the instruction prototype is needed to determine if an instruction can exist
+			// in the offcut location
+			final PseudoDisassembler pdis = new PseudoDisassembler(currentProgram);
+			final PseudoInstruction testInstr = pdis.disassemble(at);
+			return (testInstr != null && testInstr.getMaxAddress().equals(instr.getMaxAddress()));
+		}
+		catch (InsufficientBytesException | UnknownInstructionException
+				| UnknownContextException e) {
+			Msg.error(this,
+				"Could not disassemble instruction at " + at + " (" + e.getMessage() + ")", e);
+			return false;
+		}
+	}
+
+}

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/disassembler/SetLengthOverrideAction.java

@@ -18,11 +18,15 @@ package ghidra.app.plugin.core.disassembler;
 import db.Transaction;
 import docking.action.MenuData;
 import docking.widgets.dialogs.NumberInputDialog;
+import ghidra.app.cmd.disassemble.DisassembleCommand;
 import ghidra.app.context.ListingActionContext;
 import ghidra.app.context.ListingContextAction;
 import ghidra.framework.plugintool.PluginTool;
+import ghidra.program.disassemble.Disassembler;
 import ghidra.program.model.address.Address;
+import ghidra.program.model.address.AddressSet;
 import ghidra.program.model.listing.*;
+import ghidra.program.model.symbol.Reference;
 import ghidra.program.model.util.CodeUnitInsertionException;
 import ghidra.util.Msg;
 
@@ -70,28 +74,15 @@ class SetLengthOverrideAction extends ListingContextAction {
 
 		int minLength = 0;
 		long maxLength = Math.min(Instruction.MAX_LENGTH_OVERRIDE, protoLen - 1);
-		Instruction nextInstr = listing.getInstructionAfter(address);
-		if (nextInstr != null &&
-			nextInstr.getAddress().getAddressSpace().equals(address.getAddressSpace())) {
-			long limit = nextInstr.getAddress().subtract(address);
-			maxLength = Math.min(limit, maxLength);
-			if (limit < instr.getParsedLength()) {
-				minLength = 1; // unable to restore to default length using 0 value
-				restoreTip = "";
-			}
-		}
 
 		if (maxLength == 0) {
 			// Assume we have an instruction whose length can't be changed
 			Msg.showError(this, null, "Length Override Error",
-				"Insufficient space to alter current length override of 1-byte");
+				"The length of a " + protoLen + "-byte instruction may not be overridden!");
 			return;
 		}
 
-		int currentLengthOverride = 0;
-		if (instr.isLengthOverridden()) {
-			currentLengthOverride = instr.getLength();
-		}
+		final int currentLengthOverride = getDefaultOffcutLength(program, instr);
 
 		NumberInputDialog dialog = new NumberInputDialog("Override/Restore Instruction Length",
 			"Enter byte-length [" + minLength + ".." + maxLength + restoreTip + alignTip + "]",
@@ -112,7 +103,20 @@ class SetLengthOverrideAction extends ListingContextAction {
 		}
 
 		try (Transaction tx = instr.getProgram().openTransaction(kind + " Length Override")) {
+			if (lengthOverride == 0) {
+				// Clear any code units that may have been created in the offcut
+				final int trueLength = instr.getParsedLength();
+				listing.clearCodeUnits(address.add(currentLengthOverride),
+					address.add(trueLength - 1), false);
+			}
 			instr.setLengthOverride(lengthOverride);
+
+			final Address offcutStart = address.add(lengthOverride);
+			if (lengthOverride != 0 && isOffcutFlowReference(program, offcutStart)) {
+				tool.executeBackgroundCommand(new DisassembleCommand(offcutStart, null, true),
+					program);
+				removeErrorBookmark(program, offcutStart);
+			}
 		}
 		catch (CodeUnitInsertionException e) {
 			Msg.showError(this, null, "Length Override Error", e.getMessage());
@@ -134,4 +138,39 @@ class SetLengthOverrideAction extends ListingContextAction {
 		return instr.getParsedLength() > alignment;
 	}
 
+	private int getDefaultOffcutLength(final Program program, final Instruction instr) {
+		if (instr.isLengthOverridden()) {
+			return instr.getLength();
+		}
+		final AddressSet instrBody = new AddressSet(instr.getMinAddress().next(),
+			instr.getMinAddress().add(instr.getParsedLength() - 1));
+		final Address addr =
+			program.getReferenceManager().getReferenceDestinationIterator(instrBody, true).next();
+		if (addr != null) {
+			final int offset = (int) addr.subtract(instr.getMinAddress());
+			if (offset % program.getLanguage().getInstructionAlignment() == 0) {
+				return offset;
+			}
+		}
+		return 0;
+	}
+
+	private boolean isOffcutFlowReference(final Program program, final Address address) {
+		for (Reference reference : program.getReferenceManager().getReferencesTo(address)) {
+			if (reference.getReferenceType().isFlow()) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	private void removeErrorBookmark(final Program program, final Address at) {
+		final BookmarkManager bookmarkManager = program.getBookmarkManager();
+		final Bookmark bookmark = bookmarkManager.getBookmark(at, BookmarkType.ERROR,
+			Disassembler.ERROR_BOOKMARK_CATEGORY);
+		if (bookmark != null) {
+			bookmarkManager.removeBookmark(bookmark);
+		}
+	}
+
 }

Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/script/FixOffcutInstructionScriptTest.java

@@ -0,0 +1,119 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.app.plugin.core.script;
+
+import static org.junit.Assert.*;
+
+import java.io.File;
+
+import org.junit.*;
+
+import ghidra.app.plugin.core.codebrowser.CodeBrowserPlugin;
+import ghidra.framework.Application;
+import ghidra.framework.plugintool.PluginTool;
+import ghidra.program.database.ProgramBuilder;
+import ghidra.program.model.address.Address;
+import ghidra.program.model.listing.Instruction;
+import ghidra.program.model.listing.Program;
+import ghidra.program.model.mem.MemoryAccessException;
+import ghidra.test.*;
+import ghidra.util.NumericUtilities;
+
+/**
+ * Test the FixOffcutInstructionScript.
+ */
+public class FixOffcutInstructionScriptTest extends AbstractGhidraHeadedIntegrationTest {
+
+	private TestEnv env;
+	private PluginTool tool;
+	private Program program;
+	private ProgramBuilder programBuilder;
+	private CodeBrowserPlugin cb;
+	private File script;
+	private Address offcutInstructionAddress;
+
+	@Before
+	public void setUp() throws Exception {
+		env = new TestEnv();
+		program = buildProgram();
+		tool = env.launchDefaultTool(program);
+		cb = env.getPlugin(CodeBrowserPlugin.class);
+
+		script = Application.getModuleFile("Base", "ghidra_scripts/FixOffcutInstructionScript.java")
+				.getFile(true);
+
+		offcutInstructionAddress = addr("1001cea");
+	}
+
+	private Program buildProgram() throws Exception {
+		programBuilder = new ProgramBuilder("Test", ProgramBuilder._X64);
+		programBuilder.createMemory(".text", "0x1001000", 0x4000);
+
+		programBuilder.setBytes("1001cd8", "48 8d 4a 01");
+		programBuilder.setBytes("1001cdc", "48 89 d0");
+		programBuilder.setBytes("1001cdf", "64 83 3c 25 18 00 00 00 00");
+		// JZ with well formed reference into offcut instruction
+		programBuilder.setBytes("1001ce8", "74 01");
+		// LOCK CMPXCHG example offcut instruction
+		programBuilder.setBytes("1001cea", "f0 48 0f b1 0d 75 65 15 00");
+		programBuilder.setBytes("1001cf3", "48 39 d0");
+		programBuilder.setBytes("1001cf6", "0f 84 bd 00 00 00");
+		programBuilder.setBytes("1001cfc", "48 8b 15 65 65 15 00");
+		programBuilder.disassemble("1001cd8", 0x100, false);
+		return programBuilder.getProgram();
+	}
+
+	@After
+	public void tearDown() throws Exception {
+		env.dispose();
+	}
+
+	@Test
+	public void testFixOffcutInsruction() throws Exception {
+		makeSelection(tool, program, program.getMinAddress(), program.getMaxAddress());
+		ScriptTaskListener scriptID = env.runScript(script);
+		waitForScript(scriptID);
+
+		assertInstruction(offcutInstructionAddress, 1, "f0", "CMPXCHG.LOCK");
+		assertInstruction(offcutInstructionAddress.add(1), 8, "48 0f b1 0d 75 65 15 00", "CMPXCHG");
+	}
+
+	private Address addr(String address) {
+		return program.getAddressFactory().getAddress(address);
+	}
+
+	private void waitForScript(ScriptTaskListener scriptID) {
+		waitForScriptCompletion(scriptID, 100000);
+		program.flushEvents();
+		waitForBusyTool(tool);
+	}
+
+	private void assertInstruction(Address addr, int byteCount, String bytes, String mnemonic)
+			throws MemoryAccessException {
+
+		assertBytes(addr, byteCount, bytes);
+		Instruction instructionAt = program.getListing().getInstructionAt(addr);
+		assertEquals(byteCount, instructionAt.getLength());
+		assertEquals(mnemonic, instructionAt.getMnemonicString());
+	}
+
+	private void assertBytes(Address addr, int count, String bytes) throws MemoryAccessException {
+
+		byte[] x = new byte[count];
+		program.getMemory().getBytes(addr, x);
+		assertEquals(bytes, NumericUtilities.convertBytesToString(x, " "));
+	}
+}