Fix wrong sync after UC_ERR_[READ, WRITE, FETCH]_[UNMAPPED, PROT] (#1368)

* Fix wrong sync after UC_ERR_[READ, WRITE, FETCH]_[UNMAPPED, PROT]

Note that:
1. We only guarantee the pc (and other internal states) is correct
if and only of `uc_emu_start` returns without any error (or errors
have been handled in callbacks.).
2. If memory read/write error isn't handled by hooks, the state is
undefined and the pc is probably wrong if no hook is installed.

This fixes #1323.

* Rename variables

* Add note in unicorn.h

* Refine test_i386_invalid_mem_read_in_tb

include/unicorn/unicorn.h

@@ -533,6 +533,10 @@ uc_err uc_mem_read(uc_engine *uc, uint64_t address, void *bytes, size_t size);
         we will emulate the code in infinite time, until the code is finished.
  @count: the number of instructions to be emulated. When this value is 0,
         we will emulate all the code available, until the code is finished.
+  
+ NOTE: The internal states of the engine is guranteed to be correct if and only
+       if uc_emu_start returns without any errors or errors have been handled in
+       the callbacks.
 
  @return UC_ERR_OK on success, or other value on failure (refer to uc_err enum
    for detailed error).

qemu/cpu-exec.c

@@ -327,17 +327,31 @@ static tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t *tb_ptr)
          * or timer mode is in effect, since these already fix the PC.
          */
         if (!HOOK_EXISTS(env->uc, UC_HOOK_CODE) && !env->uc->timeout) {
-            if (cc->synchronize_from_tb) {
+            // We should sync pc for R/W error.
-                // avoid sync twice when helper_uc_tracecode() already did this.
+            switch (env->invalid_error) {
-                if (env->uc->emu_counter <= env->uc->emu_count &&
+                case UC_ERR_WRITE_PROT:
-                        !env->uc->stop_request && !env->uc->quit_request)
+                case UC_ERR_READ_PROT:
-                    cc->synchronize_from_tb(cpu, tb);
+                case UC_ERR_FETCH_PROT:
-            } else {
+                case UC_ERR_WRITE_UNMAPPED:
-                assert(cc->set_pc);
+                case UC_ERR_READ_UNMAPPED:
-                // avoid sync twice when helper_uc_tracecode() already did this.
+                case UC_ERR_FETCH_UNMAPPED:
-                if (env->uc->emu_counter <= env->uc->emu_count &&
+                case UC_ERR_WRITE_UNALIGNED:
-                        !env->uc->stop_request && !env->uc->quit_request)
+                case UC_ERR_READ_UNALIGNED:
-                    cc->set_pc(cpu, tb->pc);
+                case UC_ERR_FETCH_UNALIGNED:
+                    break;
+                default:
+                    if (cc->synchronize_from_tb) {
+                        // avoid sync twice when helper_uc_tracecode() already did this.
+                        if (env->uc->emu_counter <= env->uc->emu_count &&
+                                !env->uc->stop_request && !env->uc->quit_request)
+                            cc->synchronize_from_tb(cpu, tb);
+                    } else {
+                        assert(cc->set_pc);
+                        // avoid sync twice when helper_uc_tracecode() already did this.
+                        if (env->uc->emu_counter <= env->uc->emu_count &&
+                                !env->uc->stop_request && !env->uc->quit_request)
+                            cc->set_pc(cpu, tb->pc);
+                    }
             }
         }
     }

samples/sample_x86.c

@@ -15,6 +15,7 @@
 #define X86_CODE32_LOOP "\x41\x4a\xeb\xfe" // INC ecx; DEC edx; JMP self-loop
 #define X86_CODE32_MEM_WRITE "\x89\x0D\xAA\xAA\xAA\xAA\x41\x4a" // mov [0xaaaaaaaa], ecx; INC ecx; DEC edx
 #define X86_CODE32_MEM_READ "\x8B\x0D\xAA\xAA\xAA\xAA\x41\x4a" // mov ecx,[0xaaaaaaaa]; INC ecx; DEC edx
+#define X86_CODE32_MEM_READ_IN_TB "\x40\x8b\x1d\x00\x00\x10\x00\x42" // inc eax; mov ebx, [0x100000]; inc edx
 
 #define X86_CODE32_JMP_INVALID "\xe9\xe9\xee\xee\xee\x41\x4a" //  JMP outside; INC ecx; DEC edx
 #define X86_CODE32_INOUT "\x41\xE4\x3F\x4a\xE6\x46\x43" // INC ecx; IN AL, 0x3f; DEC edx; OUT 0x46, AL; INC ebx
@@ -81,6 +82,14 @@ static bool hook_mem_invalid(uc_engine *uc, uc_mem_type type,
     }
 }
 
+// dummy callback
+static bool hook_mem_invalid_dummy(uc_engine *uc, uc_mem_type type,
+        uint64_t address, int size, int64_t value, void *user_data)
+{
+    // Stop emulation.
+    return false;
+}
+
 static void hook_mem64(uc_engine *uc, uc_mem_type type,
         uint64_t address, int size, int64_t value, void *user_data)
 {
@@ -1026,6 +1035,63 @@ static void test_x86_16(void)
     uc_close(uc);
 }
 
+static void test_i386_invalid_mem_read_in_tb(void)
+{
+    uc_engine *uc;
+    uc_err err;
+    uc_hook trace1;
+
+    int r_eax = 0x1234;     // EAX register
+    int r_edx = 0x7890;     // EDX register
+    int r_eip = 0;
+
+    printf("===================================\n");
+    printf("Emulate i386 code that read invalid memory in the middle of a TB\n");
+
+    // Initialize emulator in X86-32bit mode
+    err = uc_open(UC_ARCH_X86, UC_MODE_32, &uc);
+    if (err) {
+        printf("Failed on uc_open() with error returned: %u\n", err);
+        return;
+    }
+
+    // map 2MB memory for this emulation
+    uc_mem_map(uc, ADDRESS, 2 * 1024 * 1024, UC_PROT_ALL);
+
+    // write machine code to be emulated to memory
+    if (uc_mem_write(uc, ADDRESS, X86_CODE32_MEM_READ_IN_TB, sizeof(X86_CODE32_MEM_READ_IN_TB) - 1)) {
+        printf("Failed to write emulation code to memory, quit!\n");
+        return;
+    }
+
+    // initialize machine registers
+    uc_reg_write(uc, UC_X86_REG_EAX, &r_eax);
+    uc_reg_write(uc, UC_X86_REG_EDX, &r_edx);
+
+    // Add a dummy callback.
+    uc_hook_add(uc, &trace1, UC_HOOK_MEM_READ, hook_mem_invalid_dummy, NULL, 1, 0);
+    
+    // Let it crash by design.
+    err = uc_emu_start(uc, ADDRESS, ADDRESS + sizeof(X86_CODE32_MEM_READ_IN_TB) - 1, 0, 0);
+    if (err) {
+        printf("uc_emu_start() failed BY DESIGN with error returned %u: %s\n",
+                err, uc_strerror(err));
+    }
+
+    printf(">>> Emulation done. Below is the CPU context\n");
+
+    uc_reg_read(uc, UC_X86_REG_EIP, &r_eip);
+    printf(">>> EIP = 0x%x\n", r_eip);
+
+    if (r_eip != ADDRESS + 1) {
+        printf(">>> ERROR: Wrong PC 0x%x when reading unmapped memory in the middle of TB!\n", r_eip);
+    } else {
+        printf(">>> The PC is correct after reading unmapped memory in the middle of TB.\n");
+    }
+
+    uc_close(uc);
+}
+
 int main(int argc, char **argv, char **envp)
 {
     if (argc == 2) {
@@ -1066,7 +1132,7 @@ int main(int argc, char **argv, char **envp)
         //test_i386_invalid_c6c7();
         test_x86_64();
         test_x86_64_syscall();
-
+        test_i386_invalid_mem_read_in_tb();
     }
 
     return 0;