GP-6509: Fix ConPtyTests

Ghidra/Framework/Pty/src/main/java/ghidra/pty/PtySession.java

@@ -4,9 +4,9 @@
  * 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.
@@ -47,9 +47,12 @@ public interface PtySession {
 	void destroyForcibly();
 
 	/**
-	 * Get a human-readable description of the session
-	 * 
-	 * @return the description
+	 * {@return a human-readable description of the session}
 	 */
 	String description();
+
+	/**
+	 * {@return the process handle for the session leader}
+	 */
+	ProcessHandle handle();
 }

Ghidra/Framework/Pty/src/main/java/ghidra/pty/local/LocalProcessPtySession.java

@@ -4,9 +4,9 @@
  * 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.
@@ -57,4 +57,9 @@ public class LocalProcessPtySession implements PtySession {
 	public String description() {
 		return "process " + process.pid() + " on " + ptyName;
 	}
+
+	@Override
+	public ProcessHandle handle() {
+		return process.toHandle();
+	}
 }

Ghidra/Framework/Pty/src/main/java/ghidra/pty/local/LocalWindowsNativeProcessPtySession.java

@@ -4,9 +4,9 @@
  * 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.
@@ -15,6 +15,7 @@
  */
 package ghidra.pty.local;
 
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
@@ -96,4 +97,15 @@ public class LocalWindowsNativeProcessPtySession implements PtySession {
 	public String description() {
 		return "process " + pid + " on " + ptyName;
 	}
+
+	@Override
+	public ProcessHandle handle() {
+		IntByReference lpExitCode = new IntByReference();
+		Kernel32.INSTANCE.GetExitCodeProcess(processHandle.getNative(), lpExitCode);
+		Optional<ProcessHandle> result = ProcessHandle.of(pid);
+		if (lpExitCode.getValue() != WinBase.STILL_ACTIVE) {
+			return null;
+		}
+		return result.orElseThrow(() -> new AssertionError());
+	}
 }

Ghidra/Framework/Pty/src/test/java/ghidra/pty/windows/ConPtyTest.java

@@ -15,13 +15,12 @@
  */
 package ghidra.pty.windows;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 import static org.junit.Assume.assumeTrue;
 
 import java.io.*;
 import java.lang.ProcessBuilder.Redirect;
+import java.util.concurrent.TimeUnit;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -31,8 +30,14 @@ import com.sun.jna.LastErrorException;
 import ghidra.framework.OperatingSystem;
 import ghidra.pty.*;
 import ghidra.pty.testutil.DummyProc;
+import ghidra.util.Msg;
 
 public class ConPtyTest extends AbstractPtyTest {
+	public static final int JOIN_TIMEOUT_MS = 3000;
+
+	public static final String CMD = DummyProc.which("cmd.exe");
+	public static final String GDB = DummyProc.which("gdb.exe");
+	public static final String NOTEPAD = DummyProc.which("notepad.exe");
 
 	@Before
 	public void checkWindows() {
@@ -40,11 +45,11 @@ public class ConPtyTest extends AbstractPtyTest {
 	}
 
 	@Test
-	public void testSessionCmd() throws IOException, InterruptedException {
+	public void testSessionCmd() throws Exception {
 		try (Pty pty = ConPtyFactory.INSTANCE.openpty()) {
-			PtySession cmd = pty.getChild().session(new String[] { DummyProc.which("cmd") }, null);
+			PtySession cmd = pty.getChild().session(new String[] { CMD }, null);
 			pty.getParent().getOutputStream().write("exit\r\n".getBytes());
-			assertEquals(0, cmd.waitExited());
+			assertEquals(0, cmd.waitExited(JOIN_TIMEOUT_MS, TimeUnit.MILLISECONDS));
 		}
 	}
 
@@ -60,12 +65,12 @@ public class ConPtyTest extends AbstractPtyTest {
 	}
 
 	@Test
-	public void testSessionCmdEchoTest() throws IOException, InterruptedException {
+	public void testSessionCmdEchoTest() throws Exception {
 		try (Pty pty = ConPtyFactory.INSTANCE.openpty()) {
 			PtyParent parent = pty.getParent();
 			PrintWriter writer = new PrintWriter(parent.getOutputStream());
 			BufferedReader reader = loggingReader(parent.getInputStream());
-			PtySession cmd = pty.getChild().session(new String[] { DummyProc.which("cmd") }, null);
+			PtySession cmd = pty.getChild().session(new String[] { CMD }, null);
 			runExitCheck(3, cmd);
 
 			writer.println("echo test");
@@ -77,38 +82,37 @@ public class ConPtyTest extends AbstractPtyTest {
 					do {
 						line = reader.readLine();
 					}
-					while (!"test".equals(line));	
-				} catch (IOException e) {
-					// TODO Auto-generated catch block
-					e.printStackTrace();
+					while (!"test".equals(line));
+				}
+				catch (IOException e) {
+					Msg.info(this, "done reading");
 				}
 			});
-			
+
 			t.setDaemon(true);
 			t.start();
-			
+
 			writer.println("exit 3");
 			writer.flush();
 
-			assertEquals(3, cmd.waitExited());
-			assertFalse("Content read successfully from the cmd", t.isAlive());
+			assertEquals(3, cmd.waitExited(JOIN_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+			t.join(JOIN_TIMEOUT_MS);
 		}
 	}
 
 	@Test
-	public void testSessionGdbLineLength() throws IOException, InterruptedException {
+	public void testSessionGdbLineLength() throws Exception {
 		try (Pty pty = ConPtyFactory.INSTANCE.openpty()) {
 			PtyParent parent = pty.getParent();
 			PrintWriter writer = new PrintWriter(parent.getOutputStream());
 			BufferedReader reader = loggingReader(parent.getInputStream());
-			PtySession gdb =
-				pty.getChild().session(new String[] { "C:\\msys64\\mingw64\\bin\\gdb.exe" }, null);
+			PtySession gdb = pty.getChild().session(new String[] { GDB }, null);
 
 			writer.println(
 				"echo This line is cleary much, much, much, much, much, much, much, much, much " +
 					" longer than 80 characters");
 			writer.flush();
-			
+
 			// set up reading cmd output on a thread since "readLine" is blocking
 			Thread t = new Thread(() -> {
 				String line;
@@ -116,87 +120,145 @@ public class ConPtyTest extends AbstractPtyTest {
 					do {
 						line = reader.readLine();
 					}
-					while (!"test".equals(line));	
-				} catch (IOException e) {
-					// TODO Auto-generated catch block
-					e.printStackTrace();
+					while (!"test".equals(line));
+				}
+				catch (IOException e) {
+					Msg.info(this, "done reading");
 				}
 			});
-			
+
 			t.setDaemon(true);
 			t.start();
-			
+
 			writer.println("exit 3");
 			writer.flush();
 
-			assertEquals(3, gdb.waitExited());
-			assertFalse("Content read successfully from the cmd", t.isAlive());
+			assertEquals(3, gdb.waitExited(JOIN_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+			t.join(JOIN_TIMEOUT_MS);
 		}
 	}
 
-	@Test
+	/**
+	 * Verifies that the ConPty is actually necessary to send interrupts to child processes.
+	 * <p>
+	 * Sending char 3 down the stdin is not sufficient, as demonstrated in this experiment. GDB does
+	 * not receive the interrupt, and so the target process remains running, and none of the
+	 * subsequent gdb commands are processed. Thus, the target and gdb are still running by the time
+	 * we get to the {@link Process#waitFor(long, TimeUnit)} call. It will return false, thus
+	 * causing the expected {@link AssertionError}.
+	 * 
+	 * @throws Exception
+	 *             'tis a test
+	 */
+	@Test(expected = AssertionError.class)
 	public void testGdbInterruptPlain() throws Exception {
-		ProcessBuilder builder = new ProcessBuilder("C:\\msys64\\mingw64\\bin\\gdb.exe");
-		builder.redirectOutput(Redirect.PIPE);
-		builder.redirectInput(Redirect.PIPE);
-		builder.redirectErrorStream(true);
 
-		Process gdb = builder.start();
+		boolean terminated = false;
+		Process gdb = null;
+		try {
+			ProcessBuilder builder = new ProcessBuilder(GDB);
+			builder.redirectOutput(Redirect.PIPE);
+			builder.redirectInput(Redirect.PIPE);
+			builder.redirectErrorStream(true);
 
-		PrintWriter writer = new PrintWriter(gdb.getOutputStream());
-		pump(gdb.getInputStream(), System.err);
+			gdb = builder.start();
 
-		System.out.println("Testing");
-		writer.println("echo test");
-		writer.println("set new-console on");
-		System.out.println("Launching notepad");
-		writer.println("file C:\\\\Windows\\\\notepad.exe");
-		writer.println("run");
-		writer.flush();
-		System.out.println("Waiting");
-		Thread.sleep(3000);
-		System.out.println("Interrupting");
-		writer.write(3);
-		writer.println();
-		writer.flush();
-		System.out.println("Killing");
-		writer.println("kill");
-		writer.flush();
-		writer.println("y");
-		writer.flush();
-	}
+			PrintWriter writer = new PrintWriter(gdb.getOutputStream());
+			pump(gdb.getInputStream(), System.out);
 
-	@Test
-	public void testGdbInterruptConPty() throws Exception {
-		try (Pty pty = ConPtyFactory.INSTANCE.openpty()) {
-			PtyParent parent = pty.getParent();
-			PrintWriter writer = new PrintWriter(parent.getOutputStream());
-			//BufferedReader reader = loggingReader(parent.getInputStream());
-			PtySession gdb =
-				pty.getChild().session(new String[] { "C:\\msys64\\mingw64\\bin\\gdb.exe" }, null);
-
-			pump(parent.getInputStream(), System.err);
-
-			System.out.println("Testing");
+			Msg.info(this, "Testing");
 			writer.println("echo test");
 			writer.println("set new-console on");
-			System.out.println("Launching notepad");
-			writer.println("file C:\\\\Windows\\\\notepad.exe");
+			Msg.info(this, "Launching notepad");
+			writer.println("file %s".formatted(NOTEPAD.replace("\\", "\\\\")));
 			writer.println("run");
 			writer.flush();
-			System.out.println("Waiting");
+			Msg.info(this, "Waiting");
 			Thread.sleep(3000);
-			System.out.println("Interrupting");
+			Msg.info(this, "Interrupting");
 			writer.write(3);
 			writer.println();
 			writer.flush();
-			System.out.println("Killing");
+			Thread.sleep(1000);
+			Msg.info(this, "Killing");
 			writer.println("kill");
 			writer.flush();
 			writer.println("y");
 			writer.flush();
+			writer.println("quit");
+			writer.flush();
 
-			Thread.sleep(100000);
+			terminated = gdb.waitFor(JOIN_TIMEOUT_MS, TimeUnit.MILLISECONDS);
+			assertTrue("Gdb did not terminate", terminated);
+		}
+		finally {
+			if (gdb != null && !terminated) {
+				for (ProcessHandle child : gdb.descendants().toList()) {
+					Msg.info(this, "Killing descendant process: %d".formatted(child.pid()));
+					child.destroyForcibly();
+				}
+				Msg.info(this, "Killing gdb: %d".formatted(gdb.pid()));
+				gdb.destroyForcibly();
+			}
+		}
+	}
+
+	/**
+	 * STRANGENESS: This test will fail if Eclipse was started from git-bash on Windows. I can only
+	 * guess this is because of some strange interaction between ConPty (under test here) and the
+	 * WinPty hack that git-bash still uses? Specifically, the interrupt (char 3) is not causing the
+	 * signal to actually get sent to gdb. I haven't the slightest idea where it goes instead, if
+	 * anywhere.
+	 * 
+	 * @throws Exception
+	 *             'tis a test
+	 */
+	@Test
+	public void testGdbInterruptConPty() throws Exception {
+		PtySession gdb = null;
+		try (Pty pty = ConPtyFactory.INSTANCE.openpty()) {
+			PtyParent parent = pty.getParent();
+			PrintWriter writer = new PrintWriter(parent.getOutputStream());
+			gdb = pty.getChild().session(new String[] { GDB }, null);
+
+			pump(parent.getInputStream(), System.err);
+
+			Msg.info(this, "Testing");
+			writer.println("echo test");
+			writer.println("set new-console on");
+			Msg.info(this, "Launching notepad");
+			writer.println("file %s".formatted(NOTEPAD.replace("\\", "\\\\")));
+			writer.println("run");
+			writer.flush();
+			Msg.info(this, "Waiting");
+			Thread.sleep(3000);
+
+			Msg.info(this, "Interrupting");
+			writer.write(3);
+			writer.println();
+			writer.flush();
+			Thread.sleep(1000);
+
+			Msg.info(this, "Killing");
+			writer.println("kill");
+			writer.flush();
+			writer.println("y");
+			writer.flush();
+			writer.println("quit");
+			writer.flush();
+
+			gdb.waitExited(JOIN_TIMEOUT_MS, TimeUnit.MILLISECONDS);
+		}
+		finally {
+			ProcessHandle handle = gdb.handle();
+			if (handle != null) {
+				for (ProcessHandle child : handle.descendants().toList()) {
+					Msg.info(this, "Killing descendant process: %d".formatted(child.pid()));
+					child.destroyForcibly();
+				}
+				Msg.info(this, "Killing gdb: %d".formatted(handle.pid()));
+				gdb.destroyForcibly();
+			}
 		}
 	}
 
@@ -207,8 +269,7 @@ public class ConPtyTest extends AbstractPtyTest {
 			PrintWriter writer = new PrintWriter(parent.getOutputStream());
 			//BufferedReader reader = loggingReader(parent.getInputStream());
 			PtySession gdb = pty.getChild()
-					.session(new String[] { "C:\\msys64\\mingw64\\bin\\gdb.exe", "-i", "mi2" },
-						null);
+					.session(new String[] { GDB, "-i", "mi2" }, null);
 
 			InputStream inputStream = parent.getInputStream();
 			inputStream = new AnsiBufferedInputStream(inputStream);
@@ -218,8 +279,7 @@ public class ConPtyTest extends AbstractPtyTest {
 			writer.println("-interpreter-exec console \"quit\"");
 			writer.flush();
 
-			gdb.waitExited();
-			//System.out.println("Exited");
+			gdb.waitExited(JOIN_TIMEOUT_MS, TimeUnit.MILLISECONDS);
 		}
 	}
 }

Ghidra/Test/DebuggerIntegrationTest/src/test/java/ghidra/app/plugin/core/debug/gui/tracermi/launcher/ScriptTraceRmiLaunchOfferTest.java

@@ -224,6 +224,11 @@ public class ScriptTraceRmiLaunchOfferTest extends AbstractGhidraHeadedDebuggerT
 		public String description() {
 			return null;
 		}
+
+		@Override
+		public ProcessHandle handle() {
+			return null;
+		}
 	}
 
 	record MockPty() implements Pty {