Uc.emu_stop() doesn't work in a hook if PC is updated

[drraid]

This is the same issue that was originally filed as #1133 , I am just filing to re-open (I don't think I can reopen that existing ticket?) because I encountered the same problem. Here is Python to repro the bug:

#!/usr/bin/python3

from unicorn import Uc, x86_const, UC_ARCH_X86, UC_MODE_64, UC_HOOK_INTR
from unicorn import UC_PROT_EXEC, UC_PROT_READ
import os
import sys

class Repro(object):
    def __init__(self):
        address  = 0x10000000
        #            nop int3 nop nop nop
        bytecode = b'\x90\xCC\x90\x90\x90'
        emu = Uc(UC_ARCH_X86, UC_MODE_64)
        emu.mem_map(address, 0x1000, UC_PROT_EXEC|UC_PROT_READ)
        emu.mem_write(address, bytecode)
        emu.hook_add(UC_HOOK_INTR, self.int3_callback)
        self.emu = emu
        self.address = address

    def int3_callback(self, emu, interrupt, size):
        if interrupt == 3:
            #print("writing nop back to %x" % (self.address+1))
            emu.mem_write(self.address+1, b'\x90')
            #print("press enter to set PC")
            #sys.stdin.readline()
            emu.reg_write(x86_const.UC_X86_REG_RIP, self.address+1)
            #print("press enter to emu_stop()")
            #sys.stdin.readline()
            emu.emu_stop()

    def run(self):
        emu = self.emu
        #print("press enter to emu_start()")
        #sys.stdin.readline()
        emu.emu_start(self.address, self.address+4)
        pc = emu.reg_read(x86_const.UC_X86_REG_RIP)
        expected = self.address + 1 
        print("expect: %x received: %x" % (expected, pc))

def main():
    #print(str(os.getpid()))
    repro = Repro()
    repro.run()

if __name__ == "__main__":
    main()

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[wtdcode]

Sorry I forget this issue. Will have a look ASAP.

[drraid]

I originally opened a PR with this project but I screwed it up because I was making changes against a v2 RC instead of v1. Anyway I have since forked unicorn and applied my own patch to v1, if you want to use that:

https://github.com/drraid/unicorn/commit/b237335e6f2a321b7e2f4acf4ce7fdfe91fcd61f

I can open a PR for these same changes against the real Unicorn repo v1 branch if you would like?

[wtdcode]

I originally opened a PR with this project but I screwed it up because I was making changes against a v2 RC instead of v1. Anyway I have since forked unicorn and applied my own patch to v1, if you want to use that:

drraid@b237335

I can open a PR for these same changes against the real Unicorn repo v1 branch if you would like?

To soft restart, why not write current PC to the PC register?

[drraid]

I didn't want to change any behavior of the existing "stop" functionality, so I renamed it to "soft_stop" to distinguish between its original functionality and the new full-stop function. My goal was simply to make it possible for a user to call stop() to set the flag to say "full stop requested" vs the "soft stop" used by other parts of code. As such I didn't really look how the code does restarts, as I only wanted to add the new flag, though I probably missed something :), so if you have other changes or a better way to fix it then please do!

[wtdcode]

I mean,

uc.reg_write(UC_X86_REG_RIP, uc.reg_read(UC_X86_REG_RIP))

should be exactly what you called "soft restart".

[drraid]

I don't understand, I don't call anything in https://github.com/drraid/unicorn/commit/b237335e6f2a321b7e2f4acf4ce7fdfe91fcd61f a "soft restart", only "uc_emu_soft_stop" and "uc_emu_stop". Again, all I did was add a flag to distinguish between a user calling stop() and other parts of the code which also called stop() for different reasons.

[wtdcode]

I don't understand, I don't call anything in drraid@b237335 a "soft restart", only "uc_emu_soft_stop" and "uc_emu_stop". Again, all I did was add a flag to distinguish between a user calling stop() and other parts of the code which also called stop() for different reasons.

Fine, I wrongly understand your problem. Anyway, if this issue is valid and addresses your problem, I would fix it.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[elicn]

I ran into the exact same problem in Unicorn 2.0.1.post1 @wtdcode, can you re-open this ticket?

[wtdcode]

I ran into the exact same problem in Unicorn 2.0.1.post1 @wtdcode, can you re-open this ticket?

Any short reproduction?

[elicn]

Here you go. Note the TRIGGER_BUG flag that allows you to choose whether to trigger the bug or not.

#!/usr/bin/env python3

# BUG: if the rip register is modified after unicorn is instructed to stop, the
# emulation will not stop. modifying any other register does not have the same
# effect and the emulation stops as expected.
#
# this piece of code sets a code hook to monitor the emulated address and stop
# when it reaches offset 0x7. by then, rax should reach to the value of 1.
#
# if the bug is triggered, the emulation will continue and crash upon reaching
# the ud2 instruction at the end.

from typing import Any
from unicorn import Uc, UC_ARCH_X86, UC_MODE_64, UC_HOOK_CODE
from unicorn.x86_const import UC_X86_REG_RAX, UC_X86_REG_RCX, UC_X86_REG_RIP

# toggle this to let the code run properly or trigger the bug
TRIGGER_BUG = True

uc = Uc(UC_ARCH_X86, UC_MODE_64)

mem_base = 0x100000
mem_size = 0x1000

payload = bytes.fromhex(
    '48 31 c0'  # 0x0    xor rax, rax    : rax = 0
    '90'        # 0x3    nop             :
    '48 ff c0'  # 0x4    inc rax         : rax++
    '90'        # 0x7    nop             : <-- going to stop here
    '48 ff c0'  # 0x8    inc rax         : rax++
    '90'        # 0xb    nop             :
    '0f 0b'     # 0xc    ud2             : <-- will raise UC_ERR_INSN_INVALID, but should not never be reached
    '90'        # 0xe    nop             :
    '90'        # 0xf    nop             :
)

uc.mem_map(mem_base, mem_size)
uc.mem_write(mem_base, payload)


def hook_code(uc: Uc, address: int, size: int, data: Any = None):
    print(f'about to emulate instruction at {address:#06x}')

    # if reached offset 0x7, stop the emualtion
    if address == (mem_base + 0x7):
        print(f'stopping!')
        uc.emu_stop()

        if TRIGGER_BUG:
            # modify rip: this will trigger the bug
            uc.reg_write(UC_X86_REG_RIP, mem_base + 0xb)

        # modify an arbitrary reg: this has no effect and will not trigger the bug
        uc.reg_write(UC_X86_REG_RCX, 0x1337)


uc.hook_add(UC_HOOK_CODE, hook_code)
uc.emu_start(mem_base, mem_base + len(payload))

# expected to show a value of 1
print(f'rax: {uc.reg_read(UC_X86_REG_RAX):d}')

[PhilippTakacs]

Hi

Here is a patch which should work, based on code from https://github.com/drraid/unicorn/commit/b237335e6f2a321b7e2f4acf4ce7fdfe91fcd61f :

diff --git a/include/unicorn/unicorn.h b/include/unicorn/unicorn.h
index 54ffd251..b9decb73 100644
--- a/include/unicorn/unicorn.h
+++ b/include/unicorn/unicorn.h
@@ -846,6 +846,14 @@ uc_err uc_emu_start(uc_engine *uc, uint64_t begin, uint64_t until,
 UNICORN_EXPORT
 uc_err uc_emu_stop(uc_engine *uc);
 
+
+/*
+ Soft-stop: this is an internal-only function which stops emulation,
+ it is called by uc_emu_stop and other functions within Unicorn, but
+ not intended to be called by user.
+*/
+uc_err uc_emu_soft_stop(uc_engine *uc);
+
 /*
  Register callback for a hook event.
  The callback will be run when the hook event is hit.
diff --git a/qemu/softmmu/cpus.c b/qemu/softmmu/cpus.c
index f6b242f3..22511ac7 100644
--- a/qemu/softmmu/cpus.c
+++ b/qemu/softmmu/cpus.c
@@ -96,7 +96,7 @@ static int tcg_cpu_exec(struct uc_struct *uc)
             r = cpu_exec(uc, cpu);
 
             // quit current TB but continue emulating?
-            if (uc->quit_request) {
+            if (uc->quit_request && !uc->stop_request) {
                 // reset stop_request
                 uc->stop_request = false;
 
diff --git a/qemu/target/arm/unicorn_aarch64.c b/qemu/target/arm/unicorn_aarch64.c
index fec0db68..748a3470 100644
--- a/qemu/target/arm/unicorn_aarch64.c
+++ b/qemu/target/arm/unicorn_aarch64.c
@@ -372,7 +372,7 @@ int arm64_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
         if (regid == UC_ARM64_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/arm/unicorn_arm.c b/qemu/target/arm/unicorn_arm.c
index bb39b348..9f6b6fce 100644
--- a/qemu/target/arm/unicorn_arm.c
+++ b/qemu/target/arm/unicorn_arm.c
@@ -515,7 +515,7 @@ int arm_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
         if (regid == UC_ARM_REG_R15) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/i386/unicorn.c b/qemu/target/i386/unicorn.c
index 4541044e..d970f85f 100644
--- a/qemu/target/i386/unicorn.c
+++ b/qemu/target/i386/unicorn.c
@@ -1521,7 +1521,7 @@ int x86_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
             case UC_X86_REG_IP:
                 // force to quit execution and flush TB
                 uc->quit_request = true;
-                uc_emu_stop(uc);
+                uc_emu_soft_stop(uc);
                 break;
             }
 
@@ -1535,7 +1535,7 @@ int x86_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
             case UC_X86_REG_IP:
                 // force to quit execution and flush TB
                 uc->quit_request = true;
-                uc_emu_stop(uc);
+                uc_emu_soft_stop(uc);
                 break;
             }
 #endif
diff --git a/qemu/target/m68k/unicorn.c b/qemu/target/m68k/unicorn.c
index d748ace7..508d915f 100644
--- a/qemu/target/m68k/unicorn.c
+++ b/qemu/target/m68k/unicorn.c
@@ -117,7 +117,7 @@ int m68k_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
         if (regid == UC_M68K_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/mips/unicorn.c b/qemu/target/mips/unicorn.c
index bd4d5595..cf7daf3e 100644
--- a/qemu/target/mips/unicorn.c
+++ b/qemu/target/mips/unicorn.c
@@ -170,7 +170,7 @@ int mips_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
         if (regid == UC_MIPS_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/ppc/unicorn.c b/qemu/target/ppc/unicorn.c
index b157ce5a..6f05c47b 100644
--- a/qemu/target/ppc/unicorn.c
+++ b/qemu/target/ppc/unicorn.c
@@ -361,7 +361,7 @@ int ppc_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
         if (regid == UC_PPC_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/riscv/unicorn.c b/qemu/target/riscv/unicorn.c
index a440a6bd..e68a4556 100644
--- a/qemu/target/riscv/unicorn.c
+++ b/qemu/target/riscv/unicorn.c
@@ -560,7 +560,7 @@ int riscv_reg_write(struct uc_struct *uc, unsigned int *regs, void *const *vals,
         if (regid == UC_RISCV_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/s390x/unicorn.c b/qemu/target/s390x/unicorn.c
index 469cda7c..4856e6e5 100644
--- a/qemu/target/s390x/unicorn.c
+++ b/qemu/target/s390x/unicorn.c
@@ -130,7 +130,7 @@ static int s390_reg_write(struct uc_struct *uc, unsigned int *regs,
         if (regid == UC_S390X_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/qemu/target/tricore/unicorn.c b/qemu/target/tricore/unicorn.c
index 88e937bb..c7cb02ec 100644
--- a/qemu/target/tricore/unicorn.c
+++ b/qemu/target/tricore/unicorn.c
@@ -229,7 +229,7 @@ int tricore_reg_write(struct uc_struct *uc, unsigned int *regs,
         if (regid == UC_TRICORE_REG_PC) {
             // force to quit execution and flush TB
             uc->quit_request = true;
-            uc_emu_stop(uc);
+            uc_emu_soft_stop(uc);
         }
     }
 
diff --git a/uc.c b/uc.c
index 829dda95..41d3cc4a 100644
--- a/uc.c
+++ b/uc.c
@@ -905,6 +905,12 @@ uc_err uc_emu_start(uc_engine *uc, uint64_t begin, uint64_t until,
 
 UNICORN_EXPORT
 uc_err uc_emu_stop(uc_engine *uc)
+{
+    uc->stop_request = true;
+    return uc_emu_soft_stop(uc);
+}
+
+uc_err uc_emu_soft_stop(uc_engine *uc)
 {
     UC_INIT(uc);
 
@@ -912,7 +918,6 @@ uc_err uc_emu_stop(uc_engine *uc)
         return UC_ERR_OK;
     }
 
-    uc->stop_request = true;
     // TODO: make this atomic somehow?
     if (uc->cpu) {
         // exit the current TB

Before committing this the uc_emu_soft_stop should be moved to the internal header. Also I want to add some tests and do some more debugging.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[PhilippTakacs]

I think this is fixed with b7b1a4d6b4eae2ae12e78ec6cd676a69e4ee9392 and the PoC is the testcase test_uc_emu_stop_set_ip in tests/unit/test_ctl.c