more tests and tweaks/fixes as a result

src/arm64/detours_arm64.h

@@ -7,6 +7,7 @@
 
 #include "detours_internal.h"
 
+#include <inttypes.h>
 #include <stdint.h>
 
 #include <mach/mach.h>

src/mach_detours.c

@@ -44,16 +44,20 @@ static detour_region* s_default_region = nullptr;
 
 static mach_error_t internal_detour_writable_trampoline_regions()
 {
+    mach_error_t result = err_none;
+
     // Mark all the regions as writable.
     const mach_port_t port = mach_task_self();
     for (detour_region* pRegion = s_regions_head; pRegion != NULL; pRegion = pRegion->next) {
         const mach_error_t error = mach_vm_protect(port, (mach_vm_address_t)pRegion, DETOUR_REGION_SIZE, false,
                                                    VM_PROT_READ | VM_PROT_WRITE);
         if (error != err_none) {
-            return error;
+            DETOUR_BREAK();
+            result = error;
         }
+        DETOUR_TRACE(("mach_vm_protect(%p, %" PRIu32 ", %d)\n", (void*)pRegion, DETOUR_REGION_SIZE, VM_PROT_READ | VM_PROT_WRITE));
     }
-    return err_none;
+    return result;
 }
 
 static void internal_detour_runnable_trampoline_regions()
@@ -66,6 +70,7 @@ static void internal_detour_runnable_trampoline_regions()
         if (error != err_none) {
             DETOUR_BREAK();
         }
+        DETOUR_TRACE(("mach_vm_protect(%p, %" PRIu32 ", %d)\n", (void*)pRegion, DETOUR_REGION_SIZE, VM_PROT_READ | VM_PROT_EXECUTE));
     }
 }
 
@@ -96,7 +101,7 @@ static void* internal_detour_alloc_region_from_lo(const uint8_t* lo, const uint8
     const vm_map_t task_self = mach_task_self();
 
     for (vm_address_t page = (vm_address_t)try_addr; page < (vm_address_t)hi; page += PAGE_SIZE) {
-        DETOUR_TRACE(("  Try %p\n", (void*)page));
+        //DETOUR_TRACE(("  Try %p\n", (void*)page));
 
         const mach_error_t err = vm_allocate(task_self, &page, DETOUR_REGION_SIZE, 0);
         if (err == err_none) {
@@ -121,7 +126,7 @@ static void* internal_detour_alloc_region_from_hi(const uint8_t* lo, const uint8
     const vm_map_t task_self = mach_task_self();
 
     for (vm_address_t page = try_addr; page > (vm_address_t)lo; page -= PAGE_SIZE) {
-        DETOUR_TRACE(("  Try %p\n", (void*)page));
+        //DETOUR_TRACE(("  Try %p\n", (void*)page));
         if ((void*)page >= s_system_region_lower_bound && (void*)page <= s_system_region_upper_bound) {
             // Skip region reserved for system DLLs, but preserve address space entropy.
             try_addr -= 0x08000000;
@@ -339,6 +344,9 @@ mach_error_t detour_transaction_begin()
 
 mach_error_t detour_transaction_abort()
 {
+    if (s_transaction_thread == THREAD_NULL) {
+        return err_none;
+    }
     if (s_transaction_thread != mach_thread_self()) {
         return detour_err_wrong_thread;
     }
@@ -349,6 +357,7 @@ mach_error_t detour_transaction_abort()
         DETOUR_CHECK(
             mach_vm_protect(port, (mach_vm_address_t)operation->target, operation->trampoline->restore_code_size, false,
                 operation->perm));
+        DETOUR_TRACE(("mach_vm_protect(%p, %" PRIu8 ", %d)\n", (void*)operation->target, operation->trampoline->restore_code_size, operation->perm));
 
         if (operation->kind == detour_operation_kind_attach) {
             if (operation->trampoline) {
@@ -422,7 +431,7 @@ mach_error_t detour_transaction_commit_ex(detour_func_t** out_failed_target)
                     operation->target[8], operation->target[9], operation->target[10], operation->target[11]));
 
                 detour_platform_operation_commit_detour(operation);
-                *operation->pointer = detour_platform_operation_get_trampoline_ptr(operation)
+                *operation->pointer = detour_platform_operation_get_trampoline_ptr(operation);
 
                 DETOUR_TRACE(("detours: target=%p: "
                     "%02x %02x %02x %02x "
@@ -467,6 +476,7 @@ mach_error_t detour_transaction_commit_ex(detour_func_t** out_failed_target)
         DETOUR_CHECK(
             mach_vm_protect(port, (mach_vm_address_t)operation->target, operation->trampoline->restore_code_size, false,
                 operation->perm));
+        DETOUR_TRACE(("mach_vm_protect(%p, %" PRIu8 ", %d)\n", (void*)operation->target, operation->trampoline->restore_code_size, operation->perm));
 
         if (operation->kind == detour_operation_kind_detach && operation->trampoline) {
             internal_detour_free_trampoline(operation->trampoline);
@@ -675,10 +685,10 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
     const uint8_t* src = target;
     uint8_t* trampoline_code = trampoline->code;
     uint8_t* trampoline_code_limit = trampoline_code + sizeof(trampoline->code);
-    uint32_t offset_target = 0;
+    uint32_t target_override_len = 0;
     uint32_t align_idx = 0;
 
-    while (offset_target < DETOUR_PLATFORM_SIZE_OF_JMP) {
+    while (target_override_len < DETOUR_PLATFORM_SIZE_OF_JMP) {
         const uint8_t* curr_op = src;
         uint32_t extra_len = 0;
 
@@ -686,8 +696,8 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
         src = internal_detour_copy_instruction(trampoline_code, src, &extra_len);
         DETOUR_TRACE(("   after: src=%p (copied %d bytes)\n", src, (int)(src - curr_op)));
         trampoline_code += (src - curr_op) + extra_len;
-        offset_target = (int32_t)(src - target);
+        target_override_len = (int32_t)(src - target);
-        trampoline->align[align_idx].offset_target = offset_target;
+        trampoline->align[align_idx].offset_target = target_override_len;
         trampoline->align[align_idx].offset_trampoline = trampoline_code - trampoline->code;
         align_idx++;
 
@@ -701,14 +711,14 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
     }
 
     // Consume, but don't duplicate padding if it is needed and available.
-    while (offset_target < DETOUR_PLATFORM_SIZE_OF_JMP) {
+    while (target_override_len < DETOUR_PLATFORM_SIZE_OF_JMP) {
         const uint32_t len_filler = detour_platform_is_code_filler(src);
         if (len_filler == 0) {
             break;
         }
 
         src += len_filler;
-        offset_target = (int32_t)(src - target);
+        target_override_len = (int32_t)(src - target);
     }
 
 #if DETOUR_DEBUG
@@ -725,7 +735,7 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
     }
 #endif
 
-    if (offset_target < DETOUR_PLATFORM_SIZE_OF_JMP || align_idx > ARRAYSIZE(trampoline->align)) {
+    if (target_override_len < DETOUR_PLATFORM_SIZE_OF_JMP || align_idx > ARRAYSIZE(trampoline->align)) {
         // Too few instructions.
         error = detour_err_too_small;
         if (s_ignore_too_small) {
@@ -741,17 +751,17 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
     }
 
     trampoline->code_size = (uint8_t)(trampoline_code - trampoline->code);
-    trampoline->restore_code_size = (uint8_t)offset_target;
+    trampoline->restore_code_size = (uint8_t)target_override_len;
-    memcpy(trampoline->restore_code, target, offset_target);
+    memcpy(trampoline->restore_code, target, target_override_len);
 
-    if (offset_target > sizeof(trampoline->code) - DETOUR_PLATFORM_SIZE_OF_JMP) {
+    if (target_override_len > sizeof(trampoline->code) - DETOUR_PLATFORM_SIZE_OF_JMP) {
         // Too many instructions.
         error = detour_err_too_large;
         DETOUR_BREAK();
         goto fail;
     }
 
-    trampoline->ptr_remain = target + offset_target;
+    trampoline->ptr_remain = target + target_override_len;
     trampoline->ptr_detour = (uint8_t*)detour;
 
     trampoline_code = trampoline->code + trampoline->code_size;
@@ -762,7 +772,7 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
     UNUSED_VARIABLE(trampoline_code);
 
     const mach_port_t port = mach_task_self();
-    const mach_vm_address_t page_addr = internal_detour_round_down_to_page((uintptr_t)target);
+    //const mach_vm_address_t page_addr = internal_detour_round_down_to_page((uintptr_t)target);
 
     vm_region_submap_short_info_data_64_t region_info;
     {
@@ -779,11 +789,12 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
     }
     const vm_prot_t old_perm = region_info.protection;
 
-    error = mach_vm_protect(port, page_addr, PAGE_SIZE, false, VM_PROT_READ | VM_PROT_WRITE | VM_PROT_COPY);
+    error = mach_vm_protect(port, (mach_vm_address_t)target, target_override_len, false, VM_PROT_READ | VM_PROT_WRITE | VM_PROT_COPY);
     if (error != err_none) {
         DETOUR_BREAK();
         goto fail;
     }
+    DETOUR_TRACE(("mach_vm_protect(%p, %" PRIu32 ", %d)\n", (void*)target, target_override_len, VM_PROT_READ | VM_PROT_WRITE | VM_PROT_COPY));
 
     DETOUR_TRACE(("detours: target=%p: "
         "%02x %02x %02x %02x "
@@ -793,7 +804,7 @@ mach_error_t detour_attach_ex(detour_func_t* inout_pointer, detour_func_t detour
         target[0], target[1], target[2], target[3],
         target[4], target[5], target[6], target[7],
         target[8], target[9], target[10], target[11]));
-    DETOUR_TRACE(("detours: trampoline =%p: "
+    DETOUR_TRACE(("detours: trampoline=%p: "
         "%02x %02x %02x %02x "
         "%02x %02x %02x %02x "
         "%02x %02x %02x %02x\n",
@@ -860,7 +871,7 @@ mach_error_t detour_detach(detour_func_t* inout_pointer, detour_func_t detour)
     detour = detour_platform_skip_jmp(detour);
 
     // Verify that Trampoline is in place.
-    const int32_t restore_code_size = trampoline->restore_code_size;
+    const uint32_t restore_code_size = trampoline->restore_code_size;
     uint8_t* target = trampoline->ptr_remain - restore_code_size;
     if (restore_code_size == 0 || restore_code_size > sizeof(trampoline->code)) {
         error = KERN_FAILURE;
@@ -899,12 +910,13 @@ mach_error_t detour_detach(detour_func_t* inout_pointer, detour_func_t detour)
     }
     const vm_prot_t old_perm = region_info.protection;
 
-    error = mach_vm_protect(port, (mach_vm_address_t)target, PAGE_SIZE, false,
+    error = mach_vm_protect(port, (mach_vm_address_t)target, restore_code_size, false,
                             VM_PROT_READ | VM_PROT_WRITE | VM_PROT_COPY);
     if (error != err_none) {
         DETOUR_BREAK();
         goto fail;
     }
+    DETOUR_TRACE(("mach_vm_protect(%p, %" PRIu32 ", %d)\n", (void*)target, restore_code_size, VM_PROT_READ | VM_PROT_WRITE | VM_PROT_COPY));
 
     op->kind = detour_operation_kind_detach;
     op->pointer = (uint8_t**)inout_pointer;
@@ -926,6 +938,7 @@ mach_error_t detour_attach_and_commit_ex(detour_func_t* inout_pointer, detour_fu
 {
     const mach_error_t error = detour_attach_ex(inout_pointer, detour, out_real_trampoline, out_real_target, out_real_detour);
     if (error != err_none) {
+        detour_transaction_abort();
         return error;
     }
     return detour_transaction_commit();
@@ -935,6 +948,7 @@ mach_error_t detour_detach_and_commit(detour_func_t* inout_pointer, detour_func_
 {
     const mach_error_t error = detour_detach(inout_pointer, detour);
     if (error != err_none) {
+        detour_transaction_abort();
         return error;
     }
     return detour_transaction_commit();

tests/CMakeLists.txt

@@ -1,7 +1,9 @@
 # Copyright (c) Lysann Tranvouez.  All rights reserved.
 
 add_executable(mach_detours_tests
-        test.cpp)
+        test_dylib_function.cpp
+        test_local_function.cpp
+)
 
 # The target function must be in a shared library because otherwise it might be in the same code page as the test.cpp functions.
 # Between attach and commit the target function's code page is not executable, which can mean our test driver code would not

tests/test.cpp

@@ -1,79 +0,0 @@
-// Copyright (c) Lysann Tranvouez.  All rights reserved.
-
-#include <catch2/catch_test_macros.hpp>
-
-#include <mach_detours.h>
-
-////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-
-#include "lib_function.h"
-int (*realLibFunction)() = libFunction;
-
-static int libFunctionDetourCounter = 0;
-int libFunctionDetour()
-{
-    libFunctionDetourCounter++;
-    return 94;
-}
-
-TEST_CASE( "Overriding custom function in dylib" )
-{
-    libFunctionCounter = 0;
-    libFunctionDetourCounter = 0;
-
-    REQUIRE( libFunction() == 42 );
-    REQUIRE( libFunctionCounter == 1 );
-    REQUIRE( libFunctionDetourCounter == 0 );
-
-    CHECK( detour_transaction_begin() == err_none );
-    CHECK( detour_attach(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
-    CHECK( detour_transaction_commit() == err_none );
-
-    REQUIRE( realLibFunction != libFunction );
-
-    REQUIRE( libFunctionCounter == 1 );
-    REQUIRE( libFunctionDetourCounter == 0 );
-    REQUIRE( libFunction() == 94 );
-    REQUIRE( libFunctionCounter == 1 );
-    REQUIRE( libFunctionDetourCounter == 1 );
-}
-
-////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-
-static int localFunctionCounter = 0;
-int localFunction()
-{
-    localFunctionCounter++;
-    return 67;
-}
-static int localFunctionDetourCounter = 0;
-int localFunctionDetour()
-{
-    localFunctionDetourCounter++;
-    return 12;
-}
-
-TEST_CASE( "Overriding local function" )
-{
-    int (*realFunction)() = localFunction;
-    localFunctionCounter = 0;
-    localFunctionDetourCounter = 0;
-
-    REQUIRE( localFunction() == 67 );
-    REQUIRE( localFunctionCounter == 1 );
-    REQUIRE( localFunctionDetourCounter == 0 );
-
-    CHECK( detour_transaction_begin() == err_none );
-    // Overriding a local function requires using detour_attach_and_commit instead of calling attach and commit individually.
-    // Otherwise when we return from attach (and before commit), the code page with the local function is marked as read+write (but _not_ executable).
-    // There is a good chance the code we return to (in this case the test function) is on the same memory page and can therefore not be executed (until we call commit).
-    CHECK( detour_attach_and_commit(reinterpret_cast<detour_func_t*>(&realFunction), reinterpret_cast<detour_func_t>(localFunctionDetour)) == err_none );
-
-    REQUIRE( realFunction != localFunction );
-
-    REQUIRE( localFunctionCounter == 1 );
-    REQUIRE( localFunctionDetourCounter == 0 );
-    REQUIRE( localFunction() == 12 );
-    REQUIRE( localFunctionCounter == 1 );
-    REQUIRE( localFunctionDetourCounter == 1 );
-}

tests/test_dylib_function.cpp

@@ -0,0 +1,108 @@
+// Copyright (c) Lysann Tranvouez.  All rights reserved.
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <mach_detours.h>
+
+#include "lib_function.h"
+int (*realLibFunction)() = libFunction;
+
+static int libFunctionDetourCounter = 0;
+int libFunctionDetour()
+{
+    libFunctionDetourCounter++;
+    return 94;
+}
+
+TEST_CASE( "Overriding custom function in dylib", "[dylib]" )
+{
+    libFunctionCounter = 0;
+    libFunctionDetourCounter = 0;
+
+    SECTION( "attaching installs a detour" )
+    {
+        REQUIRE( realLibFunction == libFunction );
+        REQUIRE( libFunction() == 42 );
+        REQUIRE( libFunctionCounter == 1 );
+        REQUIRE( libFunctionDetourCounter == 0 );
+
+        REQUIRE( detour_transaction_begin() == err_none );
+
+        SECTION( "attach + transaction_commit" )
+        {
+            REQUIRE( detour_attach(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+            CHECK( detour_transaction_commit() == err_none );
+        }
+        SECTION( "attach_and_commit" )
+        {
+            CHECK( detour_attach_and_commit(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+        }
+
+        CHECK( realLibFunction != libFunction );
+        CHECK( libFunctionCounter == 1 );
+        CHECK( libFunctionDetourCounter == 0 );
+        CHECK( libFunction() == 94 );
+        CHECK( libFunctionCounter == 1 );
+        CHECK( libFunctionDetourCounter == 1 );
+
+        // clean up
+        REQUIRE( detour_transaction_begin() == err_none );
+        CHECK( detour_detach_and_commit(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+    }
+
+    SECTION( "detaching in separate transaction removes detour" )
+    {
+        REQUIRE( detour_transaction_begin() == err_none );
+        REQUIRE( detour_attach_and_commit(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+        REQUIRE( realLibFunction != libFunction );
+
+        REQUIRE( detour_transaction_begin() == err_none );
+
+        SECTION( "detach + transaction_commit" )
+        {
+            REQUIRE( detour_detach(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+            CHECK( detour_transaction_commit() == err_none );
+        }
+        SECTION( "detach_and_commit" )
+        {
+            CHECK( detour_detach_and_commit(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+        }
+
+        CHECK( realLibFunction == libFunction );
+        CHECK( libFunctionCounter == 0 );
+        CHECK( libFunctionDetourCounter == 0 );
+        CHECK( libFunction() == 42 );
+        CHECK( libFunctionCounter == 1 );
+        CHECK( libFunctionDetourCounter == 0 );
+    }
+
+    SECTION( "aborting transaction means no detour" )
+    {
+        REQUIRE( detour_transaction_begin() == err_none );
+        REQUIRE( detour_attach(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+        CHECK( detour_transaction_abort() == err_none );
+
+        CHECK( realLibFunction == libFunction );
+        CHECK( libFunctionCounter == 0 );
+        CHECK( libFunctionDetourCounter == 0 );
+        CHECK( libFunction() == 42 );
+        CHECK( libFunctionCounter == 1 );
+        CHECK( libFunctionDetourCounter == 0 );
+    }
+
+    SECTION( "an error in a transaction means no detour" )
+    {
+        REQUIRE( detour_transaction_begin() == err_none );
+        REQUIRE( detour_attach(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == err_none );
+        // cannot detach because trampoline is not yet in place
+        CHECK( detour_detach(reinterpret_cast<detour_func_t*>(&realLibFunction), reinterpret_cast<detour_func_t>(libFunctionDetour)) == KERN_FAILURE );
+        CHECK( detour_transaction_commit() == KERN_FAILURE );
+
+        CHECK( realLibFunction == libFunction );
+        CHECK( libFunctionCounter == 0 );
+        CHECK( libFunctionDetourCounter == 0 );
+        CHECK( libFunction() == 42 );
+        CHECK( libFunctionCounter == 1 );
+        CHECK( libFunctionDetourCounter == 0 );
+    }
+}

tests/test_local_function.cpp

@@ -0,0 +1,47 @@
+// Copyright (c) Lysann Tranvouez.  All rights reserved.
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <mach_detours.h>
+
+static int localFunctionCounter = 0;
+int localFunction()
+{
+    localFunctionCounter++;
+    return 67;
+}
+int (*realLocalFunction)() = localFunction;
+static int localFunctionDetourCounter = 0;
+int localFunctionDetour()
+{
+    localFunctionDetourCounter++;
+    return 12;
+}
+
+TEST_CASE( "Overriding local function", "[local][attach]" )
+{
+    localFunctionCounter = 0;
+    localFunctionDetourCounter = 0;
+
+    CHECK( realLocalFunction == localFunction );
+    CHECK( localFunction() == 67 );
+    CHECK( localFunctionCounter == 1 );
+    CHECK( localFunctionDetourCounter == 0 );
+
+    CHECK( detour_transaction_begin() == err_none );
+    // Overriding a local function requires using detour_attach_and_commit instead of calling attach and commit individually.
+    // Otherwise when we return from attach (and before commit), the code page with the local function is marked as read+write (but _not_ executable).
+    // There is a good chance the code we return to (in this case the test function) is on the same memory page and can therefore not be executed (until we call commit).
+    CHECK( detour_attach_and_commit(reinterpret_cast<detour_func_t*>(&realLocalFunction), reinterpret_cast<detour_func_t>(localFunctionDetour)) == err_none );
+
+    CHECK( realLocalFunction != localFunction );
+    CHECK( localFunctionCounter == 1 );
+    CHECK( localFunctionDetourCounter == 0 );
+    CHECK( localFunction() == 12 );
+    CHECK( localFunctionCounter == 1 );
+    CHECK( localFunctionDetourCounter == 1 );
+
+    // clean up
+    CHECK( detour_transaction_begin() == err_none );
+    CHECK( detour_detach_and_commit(reinterpret_cast<detour_func_t*>(&realLocalFunction), reinterpret_cast<detour_func_t>(localFunctionDetour)) == err_none );
+}