Merge remote-tracking branch 'origin/patch'

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/parsing/GdbMiParser.java

@@ -22,7 +22,7 @@ import java.util.regex.Pattern;
 
 import org.apache.commons.collections4.MultiMapUtils;
 import org.apache.commons.collections4.MultiValuedMap;
-import org.apache.commons.collections4.multimap.HashSetValuedHashMap;
+import org.apache.commons.collections4.multimap.ArrayListValuedHashMap;
 
 import agent.gdb.manager.parsing.GdbParsingUtils.AbstractGdbParser;
 import agent.gdb.manager.parsing.GdbParsingUtils.GdbParseError;
@@ -30,11 +30,13 @@ import agent.gdb.manager.parsing.GdbParsingUtils.GdbParseError;
 /**
  * A parser for GDB/MI records
  * 
+ * <p>
  * While this is a much more machine-friendly format, it has some interesting idiosyncrasies that
  * make it annoying even within a machine. This class attempts to impose a nice abstraction of these
  * records while dealing with nuances particular to certain records, but in general. Examine GDB's
  * documentation for some example records.
  * 
+ * <p>
  * There seem to be one primitive type and two (and a half?) aggregate types in these records. The
  * one primitive type is a string. The aggregates are lists and maps, and maybe "field lists" which
  * behave like multi-valued maps. Maps introduce IDs, which comprise the map keys or field names.
@@ -88,7 +90,7 @@ public class GdbMiParser extends AbstractGdbParser {
 			/**
 			 * Build the field list
 			 * 
-			 * @return
+			 * @return the field list
 			 */
 			public GdbMiFieldList build() {
 				return list;
@@ -97,41 +99,14 @@ public class GdbMiParser extends AbstractGdbParser {
 
 		/**
 		 * A key-value entry in the field list
+		 * 
+		 * @param key the key
+		 * @param value the value
 		 */
-		public static class Entry {
+		public record Entry(String key, Object value) {
-			private final String key;
-			private final Object value;
-
-			private Entry(String key, Object value) {
-				this.key = key;
-				this.value = value;
-			}
-
-			/**
-			 * Get the key
-			 * 
-			 * @return the key
-			 */
-			public String getKey() {
-				return key;
-			}
-
-			/**
-			 * Get the value
-			 * 
-			 * @return the value
-			 */
-			public Object getValue() {
-				return value;
-			}
 		}
 
-		private MultiValuedMap<String, Object> map = new HashSetValuedHashMap<String, Object>() {
+		private MultiValuedMap<String, Object> map = new ArrayListValuedHashMap<String, Object>();
-			@Override
-			protected HashSet<Object> createCollection() {
-				return new LinkedHashSet<>();
-			}
-		};
 		private MultiValuedMap<String, Object> unmodifiableMap =
 			MultiMapUtils.unmodifiableMultiValuedMap(map);
 		private final List<Entry> entryList = new ArrayList<>();
@@ -198,6 +173,7 @@ public class GdbMiParser extends AbstractGdbParser {
 		/**
 		 * Assume only a single list is associated with the key, and get that list
 		 * 
+		 * <p>
 		 * For convenience, the list is cast to a list of elements of a given type. This cast is
 		 * unchecked.
 		 * 
@@ -220,10 +196,8 @@ public class GdbMiParser extends AbstractGdbParser {
 		 */
 		public GdbMiFieldList getFieldList(String key) {
 			Object obj = getSingleton(key);
-			if (obj instanceof List) {
+			if (obj instanceof List<?> list && list.isEmpty()) {
-				if (((List<?>) obj).isEmpty()) {
+				return GdbMiFieldList.builder().build();
-					return GdbMiFieldList.builder().build();
-				}
 			}
 			return (GdbMiFieldList) obj;
 		}
@@ -334,7 +308,7 @@ public class GdbMiParser extends AbstractGdbParser {
 	 * 
 	 * @see #parseObject(CharSequence)
 	 * @return the object
-	 * @throws GdbParseError
+	 * @throws GdbParseError if no text matches
 	 */
 	public Object parseObject() throws GdbParseError {
 		switch (peek(true)) {
@@ -369,9 +343,11 @@ public class GdbMiParser extends AbstractGdbParser {
 			char ch = buf.get();
 			if (ch > 0xff) {
 				throw new GdbParseError("byte", "U+" + String.format("%04X", ch));
-			} else if (ch == '"') {
+			}
+			else if (ch == '"') {
 				break;
-			} else if (ch != '\\') {
+			}
+			else if (ch != '\\') {
 				baos.write(ch);
 				continue;
 			}
@@ -495,6 +471,11 @@ public class GdbMiParser extends AbstractGdbParser {
 				result.add(UNNAMED, fieldVal);
 				continue;
 			}
+			if (c == '"') {
+				String bareString = parseString();
+				result.add(null, bareString);
+				continue;
+			}
 			String fieldId = match(FIELD_ID, true);
 			match(EQUALS, true);
 			Object fieldVal = parseObject();

Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/manager/parsing/GdbMiParserTest.java

@@ -18,12 +18,12 @@ package agent.gdb.manager.parsing;
 import static org.junit.Assert.assertEquals;
 
 import java.util.Arrays;
+import java.util.List;
 import java.util.function.Consumer;
 import java.util.regex.Pattern;
 
 import org.junit.Test;
 
-import agent.gdb.manager.parsing.GdbMiParser;
 import agent.gdb.manager.parsing.GdbMiParser.GdbMiFieldList;
 import agent.gdb.manager.parsing.GdbParsingUtils.GdbParseError;
 
@@ -36,41 +36,80 @@ public class GdbMiParserTest {
 
 	@Test
 	public void testMatch() throws GdbParseError {
-		GdbMiParser parser = new GdbMiParser("Hello, World!");
+		GdbMiParser parser = new GdbMiParser("""
+				Hello, World""");
 		assertEquals("Hello", parser.match(Pattern.compile("\\w+"), true));
 		assertEquals(",", parser.match(GdbMiParser.COMMA, true));
 	}
 
 	@Test
 	public void testParseString() throws GdbParseError {
-		GdbMiParser parser = new GdbMiParser("\"Hello, World!\\n\"");
+		GdbMiParser parser = new GdbMiParser("""
+				"Hello, World!\\n"\
+				""");
 		assertEquals("Hello, World!\n", parser.parseString());
+		parser.checkEmpty(false);
 	}
 
 	@Test
 	public void testParseList() throws GdbParseError {
-		GdbMiParser parser = new GdbMiParser("[\"Hello\",\"World\"]");
+		GdbMiParser parser = new GdbMiParser("""
+				["Hello","World"]""");
 		assertEquals(Arrays.asList(new String[] { "Hello", "World" }), parser.parseList());
+		parser.checkEmpty(false);
 	}
 
 	@Test
 	public void testParseMap() throws GdbParseError {
-		GdbMiParser parser = new GdbMiParser("{h=\"Hello\",w=\"World\"}");
+		GdbMiParser parser = new GdbMiParser("""
+				{h="Hello",w="World"}""");
 		assertEquals(buildFieldList((exp) -> {
 			exp.add("h", "Hello");
 			exp.add("w", "World");
 		}), parser.parseMap());
+		parser.checkEmpty(false);
 	}
 
 	@Test
 	public void testParseStringEscapes() throws GdbParseError {
-		GdbMiParser parser = new GdbMiParser("\"basic=\\n\\b\\t\\f\\r c=\\e[0m\\a delim=\\\\\\\" octal=\\000\\177\"");
+		GdbMiParser parser = new GdbMiParser("""
-		assertEquals("basic=\n\b\t\f\r c=\033[0m\007 delim=\\\" octal=\000\177", parser.parseString());
+				"basic=\\n\\b\\t\\f\\r c=\\e[0m\\a delim=\\\\\\" octal=\\000\\177"\
+				""");
+		assertEquals("basic=\n\b\t\f\r c=\033[0m\007 delim=\\\" octal=\000\177",
+			parser.parseString());
+		parser.checkEmpty(false);
 	}
 
 	@Test
 	public void testParseStringUTF8() throws GdbParseError {
-		GdbMiParser parser = new GdbMiParser("\"\\302\\244 \\342\\204\\212 \\343\\201\\251 \\351\\276\\231 \\360\\237\\230\\200\"");
+		GdbMiParser parser = new GdbMiParser("""
+				"\\302\\244 \\342\\204\\212 \\343\\201\\251 \\351\\276\\231 \\360\\237\\230\\200"\
+				""");
 		assertEquals("\u00a4 \u210a \u3069 \u9f99 \ud83d\ude00", parser.parseString());
+		parser.checkEmpty(false);
+	}
+
+	@Test
+	public void testParseBreakpointCommandList() throws GdbParseError {
+		GdbMiParser parser = new GdbMiParser("""
+				BreakpointTable={nr_rows="1",nr_cols="6",hdr=[{width="7",alignment="-1",\
+				col_name="number",colhdr="Num"},{width="14",alignment="-1",col_name="type",\
+				colhdr="Type"},{width="4",alignment="-1",col_name="disp",colhdr="Disp"},\
+				{width="3",alignment="-1",col_name="enabled",colhdr="Enb"},{width="18",\
+				alignment="-1",col_name="addr",colhdr="Address"},{width="40",alignment="2",\
+				col_name="what",colhdr="What"}],body=[bkpt={number="1",type="breakpoint",\
+				disp="keep",enabled="y",addr="0x00007ffff779c96f",at="<poll+31>",\
+				thread-groups=["i1"],times="0",script={"echo asdf","echo ghjk","echo asdf"},\
+				original-location="*0x7ffff779c96f"}]}""");
+		GdbMiFieldList result = parser.parseFields(false);
+		GdbMiFieldList table = result.getFieldList("BreakpointTable");
+		GdbMiFieldList body = table.getFieldList("body");
+		List<Object> bkpts = List.copyOf(body.get("bkpt"));
+		assertEquals(1, bkpts.size());
+		GdbMiFieldList bkpt0 = (GdbMiFieldList) bkpts.get(0);
+		GdbMiFieldList script = bkpt0.getFieldList("script");
+		List<Object> lines = List.copyOf(script.get(null));
+		assertEquals(List.of("echo asdf", "echo ghjk", "echo asdf"), lines);
+		parser.checkEmpty(false);
 	}
 }

Ghidra/Features/Decompiler/src/decompile/cpp/flow.cc

@@ -722,14 +722,37 @@ void FlowInfo::truncateIndirectJump(PcodeOp *op,JumpTable::RecoveryMode mode)
   else {
     data.opSetOpcode(op,CPUI_CALLIND); // Turn jump into call
     setupCallindSpecs(op,(FuncCallSpecs *)0);
-    if (mode != JumpTable::fail_thunk)			// Unless the switch was a thunk mechanism
+    FuncCallSpecs *fc = data.getCallSpecs(op);
-      data.getCallSpecs(op)->setBadJumpTable(true);	// Consider using special name for switch variable
+    uint4 returnType;
+    bool noParams;
+
+    if (mode == JumpTable::fail_thunk) {
+      returnType = 0;
+      noParams = false;
+    }
+    else if (mode == JumpTable::fail_callother) {
+      returnType = PcodeOp::noreturn;
+      fc->setNoReturn(true);
+      data.warning("Does not return", op->getAddr());
+      noParams = true;
+    }
+    else {
+      returnType = 0;
+      noParams = false;
+      fc->setBadJumpTable(true);		// Consider using special name for switch variable
+      data.warning("Treating indirect jump as call",op->getAddr());
+    }
+    if (noParams) {
+      if (!fc->hasModel()) {
+	fc->setInternal(glb->defaultfp, glb->types->getTypeVoid());
+	fc->setInputLock(true);
+	fc->setOutputLock(true);
+      }
+    }
 
     // Create an artificial return
-    PcodeOp *truncop = artificialHalt(op->getAddr(),0);
+    PcodeOp *truncop = artificialHalt(op->getAddr(),returnType);
     data.opDeadInsertAfter(truncop,op);
-
-    data.warning("Treating indirect jump as call",op->getAddr());
   }
 }
 

Ghidra/Features/Decompiler/src/decompile/cpp/funcdata.hh

@@ -521,7 +521,7 @@ public:
   JumpTable *findJumpTable(const PcodeOp *op) const;	///< Find a jump-table associated with a given BRANCHIND
   JumpTable *installJumpTable(const Address &addr);	///< Install a new jump-table for the given Address
   JumpTable *recoverJumpTable(Funcdata &partial,PcodeOp *op,FlowInfo *flow,JumpTable::RecoveryMode &mode);
-  bool earlyJumpTableFail(PcodeOp *op);	///< Try to determine, early, if jump-table analysis will fail
+  JumpTable::RecoveryMode earlyJumpTableFail(PcodeOp *op);	///< Try to determine, early, if jump-table analysis will fail
   int4 numJumpTables(void) const { return jumpvec.size(); }	///< Get the number of jump-tables for \b this function
   JumpTable *getJumpTable(int4 i) { return jumpvec[i]; }	///< Get the i-th jump-table
   void removeJumpTable(JumpTable *jt);			///< Remove/delete the given jump-table

Ghidra/Features/Decompiler/src/decompile/cpp/funcdata_block.cc

@@ -548,10 +548,10 @@ JumpTable::RecoveryMode Funcdata::stageJumpTable(Funcdata &partial,JumpTable *jt
 
 /// Backtrack from the BRANCHIND, looking for ops that might affect the destination.
 /// If a CALLOTHER, which is not injected/inlined in some way, is in the flow path of
-/// the destination calculation, we know the jump-table analysis will fail and return \b true.
+/// the destination calculation, we know the jump-table analysis will fail and the failure mode is returned.
 /// \param op is the BRANCHIND op
-/// \return \b true if jump-table analysis is guaranteed to fail
+/// \return \b success if there is no early failure, or the failure mode otherwise
-bool Funcdata::earlyJumpTableFail(PcodeOp *op)
+JumpTable::RecoveryMode Funcdata::earlyJumpTableFail(PcodeOp *op)
 
 {
   Varnode *vn = op->getIn(0);
@@ -559,9 +559,9 @@ bool Funcdata::earlyJumpTableFail(PcodeOp *op)
   list<PcodeOp *>::const_iterator startiter = beginOpDead();
   int4 countMax = 8;
   while(iter != startiter) {
-    if (vn->getSize() == 1) return false;
+    if (vn->getSize() == 1) return JumpTable::success;
     countMax -= 1;
-    if (countMax < 0) return false;		// Don't iterate too many times
+    if (countMax < 0) return JumpTable::success;	// Don't iterate too many times
     --iter;
     op = *iter;
     Varnode *outvn = op->getOut();
@@ -575,33 +575,33 @@ bool Funcdata::earlyJumpTableFail(PcodeOp *op)
 	  int4 id = (int4)op->getIn(0)->getOffset();
 	  UserPcodeOp *userOp = glb->userops.getOp(id);
 	  if (dynamic_cast<InjectedUserOp *>(userOp) != (InjectedUserOp *)0)
-	    return false;	// Don't try to back track through injection
+	    return JumpTable::success;	// Don't try to back track through injection
 	  if (dynamic_cast<JumpAssistOp *>(userOp) != (JumpAssistOp *)0)
-	    return false;
+	    return JumpTable::success;
 	  if (dynamic_cast<SegmentOp *>(userOp) != (SegmentOp *)0)
-	    return false;
+	    return JumpTable::success;
 	  if (outhit)
-	    return true;	// Address formed via uninjected CALLOTHER, analysis will fail
+	    return JumpTable::fail_callother;	// Address formed via uninjected CALLOTHER, analysis will fail
 	  // Assume CALLOTHER will not interfere with address and continue backtracking
 	}
 	else {
 	  // CALL or CALLIND - Output has not been established yet
-	  return false;	// Don't try to back track through CALL
+	  return JumpTable::success;	// Don't try to back track through CALL
 	}
       }
       else if (op->isBranch())
-	return false;	// Don't try to back track further
+	return JumpTable::success;	// Don't try to back track further
       else {
-	if (op->code() == CPUI_STORE) return false;	// Don't try to back track through STORE
+	if (op->code() == CPUI_STORE) return JumpTable::success;	// Don't try to back track through STORE
 	if (outhit)
-	  return false;		// Some special op (CPOOLREF, NEW, etc) generates address, don't assume failure
+	  return JumpTable::success;	// Some special op (CPOOLREF, NEW, etc) generates address, don't assume failure
 	// Assume special will not interfere with address and continue backtracking
       }
     }
     else if (op->getEvalType() == PcodeOp::unary) {
       if (outhit) {
 	Varnode *invn = op->getIn(0);
-	if (invn->getSize() != vn->getSize()) return false;
+	if (invn->getSize() != vn->getSize()) return JumpTable::success;
 	vn = invn;		// Treat input as address
       }
       // Continue backtracking
@@ -610,20 +610,20 @@ bool Funcdata::earlyJumpTableFail(PcodeOp *op)
       if (outhit) {
 	OpCode opc = op->code();
 	if (opc != CPUI_INT_ADD && opc != CPUI_INT_SUB && opc != CPUI_INT_XOR)
-	  return false;
+	  return JumpTable::success;
-	if (!op->getIn(1)->isConstant()) return false;		// Don't back-track thru binary op, don't assume failure
+	if (!op->getIn(1)->isConstant()) return JumpTable::success;	// Don't back-track thru binary op, don't assume failure
 	Varnode *invn = op->getIn(0);
-	if (invn->getSize() != vn->getSize()) return false;
+	if (invn->getSize() != vn->getSize()) return JumpTable::success;
 	vn = invn;		// Treat input as address
       }
       // Continue backtracking
     }
     else {
       if (outhit)
-	return false;
+	return JumpTable::success;
     }
   }
-  return false;
+  return JumpTable::success;
 }
 
 /// \brief Recover control-flow destinations for a BRANCHIND
@@ -657,7 +657,8 @@ JumpTable *Funcdata::recoverJumpTable(Funcdata &partial,PcodeOp *op,FlowInfo *fl
 
   if ((flags & jumptablerecovery_dont)!=0)
     return (JumpTable *)0;	// Explicitly told not to recover jumptables
-  if (earlyJumpTableFail(op))
+  mode = earlyJumpTableFail(op);
+  if (mode != JumpTable::success)
     return (JumpTable *)0;
   JumpTable trialjt(glb);
   mode = stageJumpTable(partial,&trialjt,op,flow);

Ghidra/Features/Decompiler/src/decompile/cpp/jumptable.hh

@@ -528,11 +528,12 @@ class JumpTable {
 public:
   /// \brief Recovery status for a specific JumpTable
   enum RecoveryMode {
-    success = 0,			///< JumpTable is fully recovered
+    success = 0,		///< JumpTable is fully recovered
     fail_normal = 1,		///< Normal failure to recover
     fail_thunk = 2,		///< Likely \b thunk
     fail_noflow = 3,		///< No legal flow to BRANCHIND
-    fail_return = 4  		///< Likely \b return operation
+    fail_return = 4,  		///< Likely \b return operation
+    fail_callother = 5		///< Address formed by CALLOTHER
   };
 private:
   /// \brief An address table index and its corresponding out-edge