From 1b22bac1102b916ec3f05c566c4d12c3e37d7024 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Tue, 27 Feb 2024 10:35:11 -0500 Subject: [PATCH 1/2] GP-4368: Fix breakpoint command list parsing. (closes #6257) --- .../gdb/manager/parsing/GdbMiParser.java | 63 +++++++------------ .../gdb/manager/parsing/GdbMiParserTest.java | 55 +++++++++++++--- 2 files changed, 69 insertions(+), 49 deletions(-) diff --git a/Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/parsing/GdbMiParser.java b/Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/parsing/GdbMiParser.java index e4d41063f5..52404c6335 100644 --- a/Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/parsing/GdbMiParser.java +++ b/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 * + *

* 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. * + *

* 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 { - 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; - } + public record Entry(String key, Object value) { } - private MultiValuedMap map = new HashSetValuedHashMap() { - @Override - protected HashSet createCollection() { - return new LinkedHashSet<>(); - } - }; + private MultiValuedMap map = new ArrayListValuedHashMap(); private MultiValuedMap unmodifiableMap = MultiMapUtils.unmodifiableMultiValuedMap(map); private final List 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 * + *

* 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 (((List) obj).isEmpty()) { - return GdbMiFieldList.builder().build(); - } + if (obj instanceof List list && list.isEmpty()) { + 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(); diff --git a/Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/manager/parsing/GdbMiParserTest.java b/Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/manager/parsing/GdbMiParserTest.java index 890da36f54..fa09bcd0c2 100644 --- a/Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/manager/parsing/GdbMiParserTest.java +++ b/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\""); - assertEquals("basic=\n\b\t\f\r c=\033[0m\007 delim=\\\" octal=\000\177", parser.parseString()); + GdbMiParser parser = new GdbMiParser(""" + "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="",\ + 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 bkpts = List.copyOf(body.get("bkpt")); + assertEquals(1, bkpts.size()); + GdbMiFieldList bkpt0 = (GdbMiFieldList) bkpts.get(0); + GdbMiFieldList script = bkpt0.getFieldList("script"); + List lines = List.copyOf(script.get(null)); + assertEquals(List.of("echo asdf", "echo ghjk", "echo asdf"), lines); + parser.checkEmpty(false); } } From 5942e95994b26982ae551286d5a02afe014eda64 Mon Sep 17 00:00:00 2001 From: caheckman <48068198+caheckman@users.noreply.github.com> Date: Tue, 27 Feb 2024 16:23:26 +0000 Subject: [PATCH 2/2] GP-4332 Treat software break as non-returning call --- .../Decompiler/src/decompile/cpp/flow.cc | 33 ++++++++++++--- .../Decompiler/src/decompile/cpp/funcdata.hh | 2 +- .../src/decompile/cpp/funcdata_block.cc | 41 ++++++++++--------- .../Decompiler/src/decompile/cpp/jumptable.hh | 5 ++- 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/flow.cc b/Ghidra/Features/Decompiler/src/decompile/cpp/flow.cc index 38c0d5c825..a7db1774d3 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/flow.cc +++ b/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 - data.getCallSpecs(op)->setBadJumpTable(true); // Consider using special name for switch variable + FuncCallSpecs *fc = data.getCallSpecs(op); + 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()); } } diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/funcdata.hh b/Ghidra/Features/Decompiler/src/decompile/cpp/funcdata.hh index 84a46b4240..327294571c 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/funcdata.hh +++ b/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 diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/funcdata_block.cc b/Ghidra/Features/Decompiler/src/decompile/cpp/funcdata_block.cc index 80e40074ef..a58daed29a 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/funcdata_block.cc +++ b/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 -bool Funcdata::earlyJumpTableFail(PcodeOp *op) +/// \return \b success if there is no early failure, or the failure mode otherwise +JumpTable::RecoveryMode Funcdata::earlyJumpTableFail(PcodeOp *op) { Varnode *vn = op->getIn(0); @@ -559,9 +559,9 @@ bool Funcdata::earlyJumpTableFail(PcodeOp *op) list::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(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(userOp) != (JumpAssistOp *)0) - return false; + return JumpTable::success; if (dynamic_cast(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; - if (!op->getIn(1)->isConstant()) return false; // Don't back-track thru binary op, don't assume failure + return JumpTable::success; + 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); diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/jumptable.hh b/Ghidra/Features/Decompiler/src/decompile/cpp/jumptable.hh index f550d24325..d3b1f4fbe4 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/jumptable.hh +++ b/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