Advanced Development Course Lab 5 solution doesn't compile and infinitely loops

[KipHamiltons]

Describe the bug
The included solution for Lab 5 of the Advanced Development Course does not compile. This is because the function getNextInstuction() has not been defined. If that error is fixed, by for example replacing the line with instruction = instruction.getNext(), or using an instruction iterator to reassign instruction, the code has a logic error which causes an infinite loop to occur. The infinite loop occurs whenever the script encounters an instruction which is not of the form Register <- Scalar, because in those cases instruction is not updated before the continue statements.

To Reproduce Compilation Error
Steps to reproduce the behavior:

1. Create new ghidra script named Lab5Script.java
2. Copy and paste the solution code provided [unaffiliated ghidra.re website] over the new script.
3. Run the script on a project file.

Expected behavior
The script compiles.

Example easy fix
Change the line instruction = getNextInstruction(); to instruction = instruction.getNext();

To Reproduce Infinite Loop Logic Error

1. Create new ghidra script named Lab5Script.java
2. Copy and paste the solution code provided [unaffiliated ghidra.re website] over the new script.
3. Replace the line instruction = getNextInstruction(); with instruction = instruction.getNext();
4. Run the script on a project file which has an instruction which is not a scalar value being placed into a register.

Expected behavior
The script adds EOL comments to instructions which are scalar values being placed into registers, and terminates.

Example easy fix (although this might make the solution less "clean")
Add a line before each of the three continue; lines which says instruction = instruction.getNext();
This allows the script to continue to evaluate instructions, instead of getting stuck on one, never updating which one is being evaluated.

Attachments
These problems are with the advanced development course lab 5 solution.
https://ghidra.re/courses/GhidraClass/AdvancedDevelopment/GhidraAdvancedDevelopment.html#60.0

Environment (please complete the following information):

• OS: Ubuntu 18.04
• Java Version: openjdk 11.0.9.1
• Ghidra Version: 9.3 DEV, built from source.

[astrelsky]

I'm assuming Lab5Script.java is somewhere in the repo. If it is you should submit a pull request with the correction.

[KipHamiltons]

It is in the source - now that I am inspecting the source version (as opposed to the web version I was using), I see that in the commit which closed #1028 the infinite loop bug was fixed, but the compilation bug remains (I assume 9.3 introduced the bug).
I'll submit a PR which fixes it 👍

[ghidra1]

Here is alternate implementation:

public class Lab5Script extends GhidraScript {

	@Override
	public void run() throws Exception {

		for (Instruction instruction : currentProgram.getListing().getInstructions(true)) {
			if (monitor.isCancelled()) {
				break;
			}
			if (instruction.getNumOperands() != 2) {
				continue;
			}

			Object[] opObjects0 = instruction.getOpObjects(0);
			if (opObjects0.length != 1 || !(opObjects0[0] instanceof Register)) {
				continue;
			}

			Object[] opObjects1 = instruction.getOpObjects(1);
			if (opObjects1.length != 1 || !(opObjects1[0] instanceof Scalar)) {
				continue;
			}

			Register register = (Register) opObjects0[0];
			Scalar scalar = (Scalar) opObjects1[0];
			String comment =
				"[" + register.getName() + "]=[" + scalar.toString(16, false, false, "", "") + "]";
			setEOLComment(instruction.getMinAddress(), comment);
		}
	}
}

[ghidra1]

The above solution avoids the infinite loop and is preferred to an incremental search which will be much slower.

[ghidra1]

Sticking with the FlatProgramAPI and slower approach would look like this:

public class Lab5Script extends GhidraScript {

	@Override
	public void run() throws Exception {

		for (Instruction instruction = getFirstInstruction(); instruction != null; instruction = getInstructionAfter(instruction)) {
			if (monitor.isCancelled()) {
				break;
			}
			if (instruction.getNumOperands() != 2) {
				continue;
			}

			Object[] opObjects0 = instruction.getOpObjects(0);
			if (opObjects0.length != 1 || !(opObjects0[0] instanceof Register)) {
				continue;
			}

			Object[] opObjects1 = instruction.getOpObjects(1);
			if (opObjects1.length != 1 || !(opObjects1[0] instanceof Scalar)) {
				continue;
			}

			Register register = (Register) opObjects0[0];
			Scalar scalar = (Scalar) opObjects1[0];
			String comment =
				"[" + register.getName() + "]=[" + scalar.toString(16, false, false, "", "") + "]";
			setEOLComment(instruction.getMinAddress(), comment);
		}
	}
}

[ghidra1]

I have corrected the class files in source control to reflect the later example above which sticks with using the FlatProgramAPI.