Fixup unconditional branch detection, do not annotate conditional instructions which are not taken. Add 3 new arm tests (#3358)

pwndbg/aglib/disasm/aarch64.py

@@ -362,7 +362,7 @@ class AArch64DisassemblyAssistant(pwndbg.aglib.disasm.arch.DisassemblyAssistant)
         if instruction.id == AARCH64_INS_B:
             # The B instruction can be made conditional by the condition codes
             if instruction.cs_insn.cc in (AArch64CC_Invalid, AArch64CC_AL):
-                instruction.declare_conditional = False
+                instruction.declare_is_unconditional_jump = True
             else:
                 flags = super()._read_register_name(instruction, "cpsr", emu)
                 if flags is not None:

pwndbg/aglib/disasm/arch.py

@@ -249,7 +249,10 @@ class DisassemblyAssistant:
         # Set the .target and .next fields
         self._enhance_next(instruction, emu, jump_emu)
 
-        if bool(pwndbg.config.disasm_annotations):
+        if (
+            bool(pwndbg.config.disasm_annotations)
+            and instruction.condition != InstructionCondition.FALSE
+        ):
             self._set_annotation_string(instruction, emu)
 
         # Disable emulation after CALL instructions. We do it after enhancement, as we can use emulation

pwndbg/aglib/disasm/arm.py

@@ -261,7 +261,7 @@ class ArmDisassemblyAssistant(pwndbg.aglib.disasm.arch.DisassemblyAssistant):
         # These condition codes indicate unconditionally/condition is not relevant
         if instruction.cs_insn.cc in (ARM_CC_AL, ARMCC_UNDEF):
             if instruction.id in (ARM_INS_B, ARM_INS_BL, ARM_INS_BLX, ARM_INS_BX, ARM_INS_BXJ):
-                instruction.declare_conditional = False
+                instruction.declare_is_unconditional_jump = True
             return InstructionCondition.UNDETERMINED
 
         value = self._read_register_name(instruction, self.flags_reg, emu)

pwndbg/aglib/disasm/instruction.py

@@ -23,6 +23,7 @@ from capstone.arm import ARM_INS_TBH
 from capstone.loongarch import LOONGARCH_INS_ALIAS_JR
 from capstone.loongarch import LOONGARCH_INS_B
 from capstone.loongarch import LOONGARCH_INS_BL
+from capstone.loongarch import LOONGARCH_INS_CALL36
 from capstone.loongarch import LOONGARCH_INS_JIRL
 from capstone.mips import MIPS_INS_ALIAS_B
 from capstone.mips import MIPS_INS_ALIAS_BAL
@@ -44,6 +45,8 @@ from capstone.riscv import RISCV_INS_C_JALR
 from capstone.riscv import RISCV_INS_C_JR
 from capstone.riscv import RISCV_INS_JAL
 from capstone.riscv import RISCV_INS_JALR
+from capstone.sparc import SPARC_INS_ALIAS_CALL
+from capstone.sparc import SPARC_INS_CALL
 from capstone.sparc import SPARC_INS_JMPL
 from capstone.systemz import SYSTEMZ_INS_B
 from capstone.systemz import SYSTEMZ_INS_BAL
@@ -55,6 +58,7 @@ from capstone.systemz import SYSTEMZ_INS_J
 from capstone.systemz import SYSTEMZ_INS_JL
 from capstone.x86 import X86_INS_CALL
 from capstone.x86 import X86_INS_JMP
+from capstone.x86 import X86_INS_RET
 from capstone.x86 import X86Op
 from typing_extensions import override
 
@@ -62,10 +66,8 @@ import pwndbg.dbg
 from pwndbg.dbg import DisassembledInstruction
 
 # Architecture specific instructions that mutate the instruction pointer unconditionally
-# The Capstone RET and CALL groups are also used to filter CALL and RET types when we check for unconditional jumps,
-# so we don't need to manually specify those for each architecture
 UNCONDITIONAL_JUMP_INSTRUCTIONS: Dict[int, Set[int]] = {
-    CS_ARCH_X86: {X86_INS_CALL, X86_INS_JMP},
+    CS_ARCH_X86: {X86_INS_CALL, X86_INS_RET, X86_INS_JMP},
     CS_ARCH_MIPS: {
         MIPS_INS_J,
         MIPS_INS_JR,
@@ -77,7 +79,7 @@ UNCONDITIONAL_JUMP_INSTRUCTIONS: Dict[int, Set[int]] = {
         MIPS_INS_B,
         MIPS_INS_ALIAS_B,
     },
-    CS_ARCH_SPARC: {SPARC_INS_JMPL},
+    CS_ARCH_SPARC: {SPARC_INS_CALL, SPARC_INS_ALIAS_CALL, SPARC_INS_JMPL},
     CS_ARCH_ARM: {
         ARM_INS_TBB,
         ARM_INS_TBH,
@@ -107,6 +109,7 @@ UNCONDITIONAL_JUMP_INSTRUCTIONS: Dict[int, Set[int]] = {
         LOONGARCH_INS_BL,
         LOONGARCH_INS_JIRL,
         LOONGARCH_INS_ALIAS_JR,
+        LOONGARCH_INS_CALL36,
     },
 }
 
@@ -119,12 +122,11 @@ BRANCH_AND_LINK_INSTRUCTIONS[CS_ARCH_MIPS] = {
     MIPS_INS_JALR,
 }
 
-# Everything that is a CALL or a RET is a unconditional jump
-GENERIC_UNCONDITIONAL_JUMP_GROUPS = {CS_GRP_CALL, CS_GRP_RET, CS_GRP_IRET}
 # All branch-like instructions - jumps thats are non-call and non-ret - should have one of these two groups in Capstone
 GENERIC_JUMP_GROUPS = {CS_GRP_JUMP, CS_GRP_BRANCH_RELATIVE}
+
 # All Capstone jumps should have at least one of these groups
-ALL_JUMP_GROUPS = GENERIC_JUMP_GROUPS | GENERIC_UNCONDITIONAL_JUMP_GROUPS
+ALL_JUMP_GROUPS = GENERIC_JUMP_GROUPS | {CS_GRP_CALL, CS_GRP_RET, CS_GRP_IRET}
 
 # All non-ret jumps
 FORWARD_JUMP_GROUP = {CS_GRP_CALL} | GENERIC_JUMP_GROUPS
@@ -179,7 +181,6 @@ class PwndbgInstruction(Protocol):
     target_string: str | None
     target_const: bool | None
     condition: InstructionCondition
-    declare_conditional: bool | None
     declare_is_unconditional_jump: bool
     force_unconditional_jump_target: bool
     annotation: str | None
@@ -332,21 +333,6 @@ class PwndbgInstructionImpl(PwndbgInstruction):
         FALSE if the instruction has a conditional action, and we know it is not taken.
         """
 
-        self.declare_conditional: bool | None = None
-        """
-        This field is used to declare if the instruction is a conditional instruction.
-        In most cases, we can determine this purely based on the instruction ID, and this field is irrelevent.
-        However, in some arches, like Arm, the same instruction can be made conditional by certain instruction attributes.
-        Ex:
-            Arm, `bls` instruction. This is encoded as a `b` under the code, with an additional condition code field.
-            In this case, sometimes a `b` instruction is unconditional (always branches), in other cases it is conditional.
-            We use this field to disambiguate these cases.
-
-        True if we manually determine this instruction is a conditional instruction
-        False if it's not a conditional instruction
-        None if we don't have a determination (most cases)
-        """
-
         self.declare_is_unconditional_jump: bool = False
         """
         This field is used to declare that this instruction is an unconditional jump.
@@ -467,8 +453,7 @@ class PwndbgInstructionImpl(PwndbgInstruction):
         This does not imply that we have resolved the .target
         """
         return (
-            self.declare_conditional is not False
+            self.declare_is_unconditional_jump is False
-            and self.declare_is_unconditional_jump is False
             and bool(self.groups & GENERIC_JUMP_GROUPS)
             and self.id not in UNCONDITIONAL_JUMP_INSTRUCTIONS[self.cs_insn._cs.arch]
         )
@@ -485,10 +470,8 @@ class PwndbgInstructionImpl(PwndbgInstruction):
         This does not imply that we have resolved the .target
         """
         return (
-            bool(self.groups & GENERIC_UNCONDITIONAL_JUMP_GROUPS)
+            self.declare_is_unconditional_jump
             or self.id in UNCONDITIONAL_JUMP_INSTRUCTIONS[self.cs_insn._cs.arch]
-            or self.declare_is_unconditional_jump
-            or self.declare_conditional is False
         )
 
     @property
@@ -564,7 +547,6 @@ class PwndbgInstructionImpl(PwndbgInstruction):
         Operands: [{operands_str}]
         Conditional jump: {self.is_conditional_jump}. Taken: {self.is_conditional_jump_taken}
         Unconditional jump: {self.is_unconditional_jump}
-        Declare conditional: {self.declare_conditional}
         Declare unconditional jump: {self.declare_is_unconditional_jump}
         Force jump target: {self.force_unconditional_jump_target}
         Can change PC: {self.has_jump_target}
@@ -737,7 +719,6 @@ class ManualPwndbgInstruction(PwndbgInstruction):
 
         self.condition = InstructionCondition.UNDETERMINED
 
-        self.declare_conditional = None
         self.declare_is_unconditional_jump = False
         self.force_unconditional_jump_target = False
 

tests/library/qemu_user/tests/test_arm.py

@@ -837,3 +837,153 @@ def test_arm_it_block_cached_thumb_mode(qemu_assembly_run):
     )
 
     assert dis == expected
+
+
+ARM_CONDITIONAL_INSTRUCTIONS = f"""
+{ARM_PREAMBLE}
+
+cmp r0, #0
+ldrbne r2, [r0]
+movne r0, #1
+cmpne r2, #0
+bxne lr
+
+nop
+nop
+nop
+nop
+nop
+nop
+"""
+
+
+def test_arm_conditional_instructions(qemu_assembly_run):
+    qemu_assembly_run(ARM_CONDITIONAL_INSTRUCTIONS, "arm")
+
+    dis = gdb.execute("context disasm", to_string=True)
+    dis = pwndbg.color.strip(dis)
+
+    expected = (
+        "LEGEND: STACK | HEAP | CODE | DATA | WX | RODATA\n"
+        "──────────────────[ DISASM / arm / arm mode / set emulate on ]──────────────────\n"
+        " ► 0x200b4 <_start>       cmp    r0, #0       0 - 0     CPSR => 0x60000010 [ n Z C v q j t e a i f ]\n"
+        "   0x200b8 <_start+4>   ✘ ldrbne r2, [r0]\n"
+        "   0x200bc <_start+8>   ✘ movne  r0, #1\n"
+        "   0x200c0 <_start+12>  ✘ cmpne  r2, #0\n"
+        "   0x200c4 <_start+16>  ✘ bxne   lr                          <0>\n"
+        " \n"
+        "   0x200c8 <_start+20>    nop    \n"
+        "   0x200cc <_start+24>    nop    \n"
+        "   0x200d0 <_start+28>    nop    \n"
+        "   0x200d4 <_start+32>    nop    \n"
+        "   0x200d8 <_start+36>    nop    \n"
+        "   0x200dc <_start+40>    nop    \n"
+        "────────────────────────────────────────────────────────────────────────────────\n"
+    )
+
+    assert dis == expected
+
+
+ARM_CONDITIONAL_INSTRUCTIONS_SUCCESSFUL_BRANCH = f"""
+{ARM_PREAMBLE}
+
+LDR lr, =jump_here
+cmp r0, #0
+ldrbne r2, [r0]
+movne r0, #1
+cmpne r2, #0
+bxeq lr
+
+nop
+nop
+nop
+nop
+nop
+nop
+nop
+nop
+
+jump_here:
+    nop
+    nop
+    nop
+    nop
+    nop
+    nop
+    nop
+"""
+
+
+def test_arm_conditional_instructions_successful_branch(qemu_assembly_run):
+    qemu_assembly_run(ARM_CONDITIONAL_INSTRUCTIONS_SUCCESSFUL_BRANCH, "arm")
+
+    dis = gdb.execute("context disasm", to_string=True)
+    dis = pwndbg.color.strip(dis)
+
+    expected = (
+        "LEGEND: STACK | HEAP | CODE | DATA | WX | RODATA\n"
+        "──────────────────[ DISASM / arm / arm mode / set emulate on ]──────────────────\n"
+        " ► 0x200b4 <_start>          ldr    lr, [pc, #0x4c]     R14, [jump_here+28] => 0x200ec (jump_here) ◂— nop \n"
+        "   0x200b8 <_start+4>        cmp    r0, #0              0 - 0     CPSR => 0x60000010 [ n Z C v q j t e a i f ]\n"
+        "   0x200bc <_start+8>      ✘ ldrbne r2, [r0]\n"
+        "   0x200c0 <_start+12>     ✘ movne  r0, #1\n"
+        "   0x200c4 <_start+16>     ✘ cmpne  r2, #0\n"
+        "   0x200c8 <_start+20>     ✔ bxeq   lr                          <jump_here>\n"
+        "    ↓\n"
+        "   0x200ec <jump_here>       nop    \n"
+        "   0x200f0 <jump_here+4>     nop    \n"
+        "   0x200f4 <jump_here+8>     nop    \n"
+        "   0x200f8 <jump_here+12>    nop    \n"
+        "   0x200fc <jump_here+16>    nop    \n"
+        "────────────────────────────────────────────────────────────────────────────────\n"
+    )
+
+    assert dis == expected
+
+
+ARM_CONDITIONAL_INSTRUCTIONS_CALL = f"""
+{ARM_PREAMBLE}
+
+cmp r0, #0
+bleq func
+
+nop
+nop
+nop
+nop
+nop
+nop
+nop
+nop
+nop
+
+func:
+    mov pc, lr
+"""
+
+
+def test_arm_conditional_call(qemu_assembly_run):
+    qemu_assembly_run(ARM_CONDITIONAL_INSTRUCTIONS_CALL, "arm")
+
+    dis = gdb.execute("context disasm", to_string=True)
+    dis = pwndbg.color.strip(dis)
+
+    expected = (
+        "LEGEND: STACK | HEAP | CODE | DATA | WX | RODATA\n"
+        "──────────────────[ DISASM / arm / arm mode / set emulate on ]──────────────────\n"
+        " ► 0x200b4 <_start>       cmp    r0, #0     0 - 0     CPSR => 0x60000010 [ n Z C v q j t e a i f ]\n"
+        "   0x200b8 <_start+4>   ✔ bleq   func                        <func>\n"
+        " \n"
+        "   0x200bc <_start+8>     nop    \n"
+        "   0x200c0 <_start+12>    nop    \n"
+        "   0x200c4 <_start+16>    nop    \n"
+        "   0x200c8 <_start+20>    nop    \n"
+        "   0x200cc <_start+24>    nop    \n"
+        "   0x200d0 <_start+28>    nop    \n"
+        "   0x200d4 <_start+32>    nop    \n"
+        "   0x200d8 <_start+36>    nop    \n"
+        "   0x200dc <_start+40>    nop    \n"
+        "────────────────────────────────────────────────────────────────────────────────\n"
+    )
+
+    assert dis == expected