Merge remote-tracking branch 'origin/patch'

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/GdbPendingCommand.java

@@ -26,6 +26,7 @@ import agent.gdb.manager.impl.cmd.GdbCommandError;
 /**
  * A command queued on the GDB manager
  *
+ * <p>
  * A {@link GdbCommand} is queued by wrapping it in a {@link GdbPendingCommand} and submitting it to
  * the manager implementation's executor. This object also keep track of claimed/stolen events and
  * provides convenience methods for sifting through them.
@@ -92,6 +93,7 @@ public class GdbPendingCommand<T> extends CompletableFuture<T> implements GdbCau
 	/**
 	 * Claim an event
 	 * 
+	 * <p>
 	 * This stores the event for later retrieval and processing.
 	 * 
 	 * @param evt the event
@@ -104,6 +106,7 @@ public class GdbPendingCommand<T> extends CompletableFuture<T> implements GdbCau
 	/**
 	 * Steal an event
 	 * 
+	 * <p>
 	 * This stores the event for later retrieval and processing.
 	 * 
 	 * @param evt the event
@@ -205,6 +208,7 @@ public class GdbPendingCommand<T> extends CompletableFuture<T> implements GdbCau
 	/**
 	 * Check that the command completed with one of the given results
 	 * 
+	 * <p>
 	 * {@link GdbCommandErrorEvent} need not be listed. This method will handle it as a special case
 	 * already. To avoid the special treatment, list it explicitly.
 	 * 

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/cmd/AbstractGdbCommand.java

@@ -66,37 +66,6 @@ public abstract class AbstractGdbCommand<T> implements GdbCommand<T> {
 		return Interpreter.MI2;
 	}
 
-	/**
-	 * Check for an error reported in MI2 syntax via the CLI
-	 * 
-	 * <p>
-	 * This must be used in the {@link #handle(GdbEvent, GdbPendingCommand)} callback when the
-	 * command is encoded as a MI2 command (using {@code interpreter-exec mi2}) but issued via the
-	 * CLI. Depending on the GDB version and the outcome of the command, the result may be reported
-	 * via the CLI, but in MI2 syntax. As of yet, this has only been observed for {@code ^error}
-	 * results.
-	 * 
-	 * @param evt the event to check
-	 * @return the decoded error event, if applicable, or the original unmodified event.
-	 */
-	protected GdbEvent<?> checkErrorViaCli(GdbEvent<?> evt) {
-		if (evt instanceof GdbConsoleOutputEvent) {
-			GdbConsoleOutputEvent outEvt = (GdbConsoleOutputEvent) evt;
-			// This is quirky in 8.0.1.
-			// I don't know to what other version(s) it applies.
-			String out = outEvt.getOutput();
-			if (out.startsWith("^error")) {
-				try {
-					return GdbCommandErrorEvent.fromMi2(out.split(",", 2)[1].trim());
-				}
-				catch (GdbParseError e) {
-					Msg.error(this, "Could not parse error result", e);
-				}
-			}
-		}
-		return evt;
-	}
-
 	@Override
 	public boolean handle(GdbEvent<?> evt, GdbPendingCommand<?> pending) {
 		/**
@@ -106,7 +75,8 @@ public abstract class AbstractGdbCommand<T> implements GdbCommand<T> {
 		 * spurious {@code ^running} command-completion events actually complete any command, except
 		 * ones where we expect that result. This seems a bug in GDB to me.
 		 * 
-		 * UPDATE: It looks like this will be fixed in gdb-14.
+		 * UPDATE: It looks like this will be fixed in gdb-14. Despite the fix we leave this
+		 * workaround in place while we still support older gdb versions.
 		 */
 		if (evt instanceof GdbCommandRunningEvent) {
 			return false;

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/cmd/AbstractLaunchGdbCommand.java

@@ -41,7 +41,6 @@ public abstract class AbstractLaunchGdbCommand extends AbstractGdbCommand<GdbThr
 
 	@Override
 	public boolean handle(GdbEvent<?> evt, GdbPendingCommand<?> pending) {
-		evt = checkErrorViaCli(evt);
 		if (evt instanceof GdbThreadCreatedEvent) {
 			pending.claim(evt);
 		}

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/cmd/GdbContinueCommand.java

@@ -43,7 +43,6 @@ public class GdbContinueCommand extends AbstractGdbCommandWithThreadId<Void>
 
 	@Override
 	public boolean handle(GdbEvent<?> evt, GdbPendingCommand<?> pending) {
-		evt = checkErrorViaCli(evt); // TODO: Deprecated, since that hack can crash GDB
 		return handleExpectingRunning(evt, pending);
 	}
 

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/cmd/GdbStepCommand.java

@@ -47,7 +47,6 @@ public class GdbStepCommand extends AbstractGdbCommandWithThreadId<Void>
 
 	@Override
 	public boolean handle(GdbEvent<?> evt, GdbPendingCommand<?> pending) {
-		evt = checkErrorViaCli(evt); // TODO: Deprecated, since that hack can crash GDB
 		return handleExpectingRunning(evt, pending);
 	}
 

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/cmd/MixinResumeInCliGdbCommand.java

@@ -39,23 +39,55 @@ public interface MixinResumeInCliGdbCommand<T> extends GdbCommand<T> {
 	}
 
 	default boolean handleExpectingRunning(GdbEvent<?> evt, GdbPendingCommand<?> pending) {
-		if (evt instanceof GdbCommandRunningEvent) {
-			pending.claim(evt);
-			return pending.hasAny(GdbRunningEvent.class);
+		if (getInterpreter() == Interpreter.CLI) {
+			/**
+			 * As of gdb-14 (anticipated based on build from git commit b6ac461a), the MI2 console
+			 * will no longer receive ^running from commands issued to the CLI that resume the
+			 * target. I suspect only the console that issues the command is meant to receive the
+			 * ^running result, and that the behavior we had been taking advantage of was in fact a
+			 * bug. Thus, we should only expect the *running event. For the sake of older versions,
+			 * when we receive ^running, we'll still claim it, so long as it comes before *running.
+			 */
+			if (evt instanceof GdbRunningEvent) {
+				pending.claim(evt);
+				return true;
+			}
+			else if (evt instanceof AbstractGdbCompletedCommandEvent) {
+				pending.claim(evt);
+				return false;
+			}
 		}
-		else if (evt instanceof AbstractGdbCompletedCommandEvent) {
-			pending.claim(evt);
-			return true; // Not the expected Completed event 
-		}
-		else if (evt instanceof GdbRunningEvent) {
-			// Event happens no matter which interpreter received the command
-			pending.claim(evt);
-			return pending.hasAny(GdbCommandRunningEvent.class);
+		else {
+			/**
+			 * This situation should only occur with versions before about 8.0, anyway, since those
+			 * supporting new-ui should have the CLI. I guess a user could still pass -i mi2 to a
+			 * modern version, which might induce this. In any case, if we're issuing the command
+			 * from MI2, we should expect both the *running event and the ^running result.
+			 */
+			if (evt instanceof GdbCommandRunningEvent) {
+				pending.claim(evt);
+				return pending.hasAny(GdbRunningEvent.class);
+			}
+			else if (evt instanceof AbstractGdbCompletedCommandEvent) {
+				pending.claim(evt);
+				return true; // Not the expected Completed event
+			}
+			else if (evt instanceof GdbRunningEvent) {
+				// Event happens no matter which interpreter received the command
+				pending.claim(evt);
+				return pending.hasAny(GdbCommandRunningEvent.class);
+			}
 		}
 		return false;
 	}
 
 	default void completeOnRunning(GdbPendingCommand<?> pending) {
-		pending.checkCompletion(GdbCommandRunningEvent.class);
+		// See comments in handleExpectingRunning
+		if (getInterpreter() == Interpreter.CLI) {
+			pending.findSingleOf(GdbRunningEvent.class);
+		}
+		else { // MI2
+			pending.checkCompletion(GdbCommandRunningEvent.class);
+		}
 	}
 }

Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/manager/impl/AbstractGdbManagerTest.java

@@ -26,10 +26,12 @@ import java.math.BigInteger;
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
 import java.util.*;
-import java.util.concurrent.*;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
+import org.hamcrest.Matchers;
 import org.junit.*;
 
 import agent.gdb.manager.*;
@@ -51,7 +53,7 @@ public abstract class AbstractGdbManagerTest extends AbstractGhidraHeadlessInteg
 	protected File gdbBin;
 
 	protected File findGdbBin() {
-		return new File(GdbManager.DEFAULT_GDB_CMD);
+		return new File(System.getProperty("test.gdbmanager.path", GdbManager.DEFAULT_GDB_CMD));
 	}
 
 	@Before
@@ -71,12 +73,7 @@ public abstract class AbstractGdbManagerTest extends AbstractGhidraHeadlessInteg
 	}
 
 	protected <T> T waitOn(CompletableFuture<T> future) throws Throwable {
-		try {
-			return future.get(TIMEOUT_MILLISECONDS, TimeUnit.MILLISECONDS);
-		}
-		catch (ExecutionException e) {
-			throw e.getCause();
-		}
+		return future.get(TIMEOUT_MILLISECONDS, TimeUnit.MILLISECONDS);
 	}
 
 	@After
@@ -162,9 +159,16 @@ public abstract class AbstractGdbManagerTest extends AbstractGhidraHeadlessInteg
 	@Test
 	public void testListModulesAndSections() throws Throwable {
 		try (GdbManager mgr = GdbManager.newInstance(getPtyFactory())) {
+			// See testStartInterrupt
+			LibraryWaiter libcLoaded = new LibraryWaiter(name -> name.contains("libc"));
+			mgr.addEventsListener(libcLoaded);
 			waitOn(startManager(mgr));
 			GdbInferior inferior = mgr.currentInferior();
 			waitOn(inferior.fileExecAndSymbols("/usr/bin/echo"));
+			waitOn(inferior.start()); // listModules now requires info proc mappings
+			waitOn(libcLoaded);
+			Thread.sleep(100); // TODO: Why?
+			mgr.sendInterruptNow();
 			Map<String, GdbModule> modules = waitOn(inferior.listModules(false));
 			GdbModule modEcho = modules.get("/usr/bin/echo");
 			assertNotNull(modEcho);
@@ -285,6 +289,7 @@ public abstract class AbstractGdbManagerTest extends AbstractGhidraHeadlessInteg
 			Msg.debug(this, "Interrupting");
 			mgr.sendInterruptNow();
 			Msg.debug(this, "Verifying at syscall");
+			Thread.sleep(100); // TODO: Actually wait for *stopped event
 			String out = waitOn(mgr.consoleCapture("x/1i $pc-2"));
 			// TODO: This is x86-specific
 			assertTrue("Didn't stop at syscall", out.contains("syscall"));
@@ -315,16 +320,12 @@ public abstract class AbstractGdbManagerTest extends AbstractGhidraHeadlessInteg
 		}
 	}
 
-	protected String getExpectedDefaultArgsVar() {
-		return null;
-	}
-
 	@Test
 	public void testSetVarGetVar() throws Throwable {
 		try (GdbManager mgr = GdbManager.newInstance(getPtyFactory())) {
 			waitOn(startManager(mgr));
 			String val = waitOn(mgr.currentInferior().getVar("args"));
-			assertEquals(getExpectedDefaultArgsVar(), val);
+			assertThat(val, Matchers.emptyOrNullString());
 			waitOn(mgr.currentInferior().setVar("args", "test"));
 			val = waitOn(mgr.currentInferior().getVar("args"));
 			assertEquals("test", val);

Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/manager/impl/SpawnedMi2GdbHomeLocalManagerTest.java

@@ -41,9 +41,4 @@ public class SpawnedMi2GdbHomeLocalManagerTest extends AbstractGdbManagerTest {
 			throw new AssertionError(e);
 		}
 	}
-
-	@Override
-	protected String getExpectedDefaultArgsVar() {
-		return "";
-	}
 }