Squashed commit of the following:

commit 641958d

    Rearrange fns to make merge back cleaner.

commit 801f8e4

    Fix benign warning.

commit 8d2a092

    Clean up hooking.

    Detours is now used only for add_jmp; add_iat is back to using hook_iat.
    This also removes the need for detach(), since undoing a hook_iat is
    simple.

commit 7e30cfb

    Repair our IAT after hooking.

    Clink is working now, using Detours.

commit 71186e4

    Use Detours to hook APIs.

    This compiles and runs, but isn't hooking kernelbase!ReadConsoleW quite
    right yet and so it isn't intercepting CMD's calls yet.  And it isn't
    repairing the IAT correctly yet.

TODO.md

@@ -5,6 +5,7 @@ ChrisAnt Plans
 # Bugs
 
 ## Soon
+- `LOG()` certain important failure information inside Detours.
 - **Alt+P** then **Ctrl+G** internally resets the prompt, but `rl_redisplay()` gets confused into still drawing the cached `local_prompt`.
 - If the last line of the prompt is "too long" then `rl_message()` in **Alt+P** fails to draw the adjusted prompt correctly; the old prompt continues to be drawn.
   - The cutoff is 136 characters -- less and the message shows up, or more and no message.

clink/app/src/host/host_cmd.cpp

@@ -259,14 +259,13 @@ bool host_cmd::validate()
 //------------------------------------------------------------------------------
 bool host_cmd::initialise()
 {
-    void* base = GetModuleHandle(nullptr);
     hook_setter hooks;
 
     // Hook the setting of the 'prompt' environment variable so we can tag
     // it and detect command entry via a write hook.
     tag_prompt();
-    hooks.add_iat(base, "SetEnvironmentVariableW",  &host_cmd::set_env_var);
-    hooks.add_iat(base, "WriteConsoleW",            &host_cmd::write_console);
+    hooks.add_iat(nullptr, "SetEnvironmentVariableW", &host_cmd::set_env_var);
+    hooks.add_iat(nullptr, "WriteConsoleW", &host_cmd::write_console);
 
     // Set a trap to get a callback when cmd.exe fetches PROMPT environment
     // variable.  GetEnvironmentVariableW is always called before displaying the
@@ -285,12 +284,12 @@ bool host_cmd::initialise()
         return ret;
     };
     auto* as_stdcall = static_cast<DWORD (__stdcall *)(LPCWSTR, LPWSTR, DWORD)>(get_environment_variable_w);
-    hooks.add_iat(base, "GetEnvironmentVariableW", as_stdcall);
+    hooks.add_iat(nullptr, "GetEnvironmentVariableW", as_stdcall);
 
     rl_add_funmap_entry("clink-expand-env-var", expand_env_var);
     rl_add_funmap_entry("clink-expand-doskey-alias", expand_doskey_alias);
 
-    return (hooks.commit() == 3);
+    return hooks.commit();
 }
 
 //------------------------------------------------------------------------------
@@ -483,17 +482,12 @@ BOOL WINAPI host_cmd::set_env_var(const wchar_t* name, const wchar_t* value)
 //------------------------------------------------------------------------------
 bool host_cmd::initialise_system()
 {
-    // Get the base address of module that exports ReadConsoleW.  (Can't use the
-    // address of ReadConsoleW directly, because that's our import library stub,
-    // not the actual API address.)
-    HMODULE hlib = LoadLibraryW(L"kernelbase.dll");
-    if (hlib == nullptr)
-        return false;
-    FARPROC proc = GetProcAddress(hlib, "ReadConsoleW");
-    if (proc == nullptr)
-        return false;
-    void* kernel_module = vm().get_alloc_base(proc);
-    if (kernel_module == nullptr)
+    // Must hook the one in kernelbase.dll.  CMD links with kernelbase.dll, but
+    // we link with kernel32.dll.  So hooking our ReadConsoleW pointer wouldn't
+    // affect CMD.
+    hook_setter hooks;
+    hooks.add_jmp("kernelbase.dll", "ReadConsoleW", &host_cmd::read_console);
+    if (!hooks.commit())
         return false;
 
     // Add an alias to Clink so it can be run from anywhere. Similar to adding
@@ -516,7 +510,5 @@ bool host_cmd::initialise_system()
     // setlocal/endlocal in a boot Batch script.
     tag_prompt();
 
-    hook_setter hooks;
-    hooks.add_jmp(kernel_module, "ReadConsoleW",    &host_cmd::read_console);
-    return (hooks.commit() == 1);
+    return true;
 }

clink/app/src/utils/hook_setter.cpp

@@ -7,96 +7,159 @@
 #include <core/base.h>
 #include <core/log.h>
 #include <process/hook.h>
-#include <process/pe.h>
 #include <process/vm.h>
+#include <detours.h>
 
 //------------------------------------------------------------------------------
 hook_setter::hook_setter()
-: m_desc_count(0)
 {
+    LONG err = NOERROR;
+
+    // In order to repair our IAT, we need the base address of our module.
+    if (!err)
+    {
+        m_self = vm().get_alloc_base("clink");
+        if (m_self == nullptr)
+            err = GetLastError();
+    }
+
+    // Start a detour transaction.
+    if (!err)
+        err = DetourTransactionBegin();
+
+    if (err)
+    {
+        LOG("Unable to start hook transaction (error %u).", err);
+        return;
+    }
+
+    LOG(">>> Started hook transaction.");
+    m_pending = true;
 }
 
 //------------------------------------------------------------------------------
-int hook_setter::commit()
+hook_setter::~hook_setter()
 {
-    // Each hook needs fixing up, so we find the base address of our module.
-    void* self = vm().get_alloc_base("clink");
-    if (self == nullptr)
-        return 0;
+    if (m_pending)
+    {
+        LOG("<<< Hook transaction aborted.");
+        DetourTransactionAbort();
+    }
 
-    // Apply all the hooks add to the setter.
-    int success = 0;
+    free_repair_iat_list(m_repair_iat);
+}
+
+//------------------------------------------------------------------------------
+bool hook_setter::commit()
+{
+    m_pending = false;
+
+    // TODO: suspend threads?  Currently this relies on CMD being essentially
+    // single threaded.
+
+    LONG err = DetourTransactionCommit();
+    if (!err)
+    {
+        apply_repair_iat_list(m_repair_iat);
+    }
+    else
+    {
+        LOG("<<< Unable to commit hooks (error %u).", err);
+        free_repair_iat_list(m_repair_iat);
+        m_desc_count = 0;
+        return false;
+    }
+
+    // Apply any IAT hooks.
+    int failed = 0;
     for (int i = 0; i < m_desc_count; ++i)
     {
-        const hook_desc& desc = m_descs[i];
-        switch (desc.type)
-        {
-        case hook_type_iat_by_name: success += !!commit_iat(self, desc);  break;
-        case hook_type_jmp:         success += !!commit_jmp(self, desc);  break;
-        }
+        const hook_iat_desc& desc = m_descs[i];
+        failed += !commit_iat(m_self, desc);
+    }
+    m_desc_count = 0;
+
+    if (failed)
+    {
+        LOG("<<< Unable to commit hooks.");
+        return false;
     }
 
-    return success;
+    LOG("<<< Hook transaction committed.");
+
+    // TODO: resume threads?  Currently this relies on CMD being essentially
+    // single threaded.
+
+    return true;
 }
 
 //------------------------------------------------------------------------------
-hook_setter::hook_desc* hook_setter::add_desc(
-    hook_type type,
-    void* module,
-    const char* name,
-    hookptr_t hook)
+bool hook_setter::add_desc(const char* module, const char* name, hookptr_t hook)
 {
+    assert(m_desc_count < sizeof_array(m_descs));
     if (m_desc_count >= sizeof_array(m_descs))
-        return nullptr;
+    {
+        assert(false);
+        return false;
+    }
 
-    hook_desc& desc = m_descs[m_desc_count];
-    desc.type = type;
-    desc.module = module;
+    void* base = GetModuleHandleA(module);
+
+    hook_iat_desc& desc = m_descs[m_desc_count++];
+    desc.base = base;
     desc.hook = hook;
     desc.name = name;
-
-    ++m_desc_count;
-    return &desc;
+    return true;
 }
 
 //------------------------------------------------------------------------------
-bool hook_setter::commit_iat(void* self, const hook_desc& desc)
+void* follow_jump(void* addr);
+bool hook_setter::add_detour(const char* module, const char* name, hookptr_t detour)
 {
-    hookptr_t addr = hook_iat(desc.module, nullptr, desc.name, desc.hook, 1);
-    if (addr == nullptr)
+    LOG("Attempting to hook %s in %s with %p.", name, module, detour);
+    PVOID proc = DetourFindFunction(module, name);
+    if (!proc)
     {
-        LOG("Unable to hook %s in IAT at base %p", desc.name, desc.module);
+        LOG("Unable to find %s in %s.", name, module);
         return false;
     }
 
-    // If the target's IAT was hooked then the hook destination is now
-    // stored in 'addr'. We hook ourselves with this address to maintain
-    // any IAT hooks that may already exist.
-    if (hook_iat(self, nullptr, desc.name, addr, 1) == 0)
+    // Get the target pointer to hook.
+    PVOID replace = follow_jump(proc);
+    if (!replace)
     {
-        LOG("Failed to hook own IAT for %s", desc.name);
+        LOG("Unable to get target address.");
+        return false;
+    }
+
+    // Hook the target pointer.
+    PDETOUR_TRAMPOLINE trampoline;
+    LONG err = DetourAttachEx(&replace, detour, &trampoline, nullptr, nullptr);
+    if (err != NOERROR)
+    {
+        LOG("Unable to hook %s (error %u).", name, err);
         return false;
     }
 
+    // Hook our IAT back to the original.
+    add_repair_iat_node(m_repair_iat, m_self, module, name, hookptr_t(trampoline));
+
     return true;
 }
 
 //------------------------------------------------------------------------------
-bool hook_setter::commit_jmp(void* self, const hook_desc& desc)
+bool hook_setter::commit_iat(void* self, const hook_iat_desc& desc)
 {
-    // Hook into a DLL's import by patching the start of the function. 'addr' is
-    // the trampoline that can be used to call the original. This method doesn't
-    // use the IAT.
-
-    auto* addr = hook_jmp(desc.module, desc.name, desc.hook);
+    hookptr_t addr = hook_iat(desc.base, nullptr, desc.name, desc.hook, 1);
     if (addr == nullptr)
     {
-        LOG("Unable to hook %s in %p", desc.name, desc.module);
+        LOG("Unable to hook %s in IAT at base %p", desc.name, desc.base);
         return false;
     }
 
-    // Patch our own IAT with the address of a trampoline so out use of this
-    // function calls the original.
+    // If the target's IAT was hooked then the hook destination is now
+    // stored in 'addr'. We hook ourselves with this address to maintain
+    // any IAT hooks that may already exist.
     if (hook_iat(self, nullptr, desc.name, addr, 1) == 0)
     {
         LOG("Failed to hook own IAT for %s", desc.name);

clink/app/src/utils/hook_setter.h

@@ -3,54 +3,56 @@
 
 #pragma once
 
+struct repair_iat_node;
+
 //------------------------------------------------------------------------------
 class hook_setter
 {
-public:
-    typedef void (__stdcall *hookptr_t)();
+    typedef void (__stdcall* hookptr_t)();
 
+    struct hook_iat_desc
+    {
+        void*                   base;
+        const char*             name;
+        hookptr_t               hook;
+    };
+
+public:
                                 hook_setter();
+                                ~hook_setter();
+
     template <typename RET,
-             typename... ARGS>
-    bool                        add_iat(void* module, const char* name, RET (__stdcall *hook)(ARGS...));
+              typename... ARGS>
+    bool                        add_iat(const char* module, const char* name, RET (__stdcall *hook)(ARGS...));
     template <typename RET,
-             typename... ARGS>
-    bool                        add_jmp(void* module, const char* name, RET (__stdcall *hook)(ARGS...));
-    int                         commit();
+              typename... ARGS>
+    bool                        add_jmp(const char* module, const char* name, RET (__stdcall *hook)(ARGS...));
+    bool                        commit();
 
 private:
-    enum hook_type
-    {
-        hook_type_iat_by_name,
-        //hook_type_iat_by_addr,
-        hook_type_jmp,
-    };
-
-    struct hook_desc
-    {
-        void*                   module;
-        const char*             name;
-        hookptr_t               hook;
-        hook_type               type;
-    };
+    bool                        add_desc(const char* module, const char* name, hookptr_t hook);
+    bool                        add_detour(const char* module, const char* name, hookptr_t hook);
+    bool                        commit_iat(void* self, const hook_iat_desc& desc);
+    void                        free_repair_list();
 
-    hook_desc*                  add_desc(hook_type type, void* module, const char* name, hookptr_t hook);
-    bool                        commit_iat(void* self, const hook_desc& desc);
-    bool                        commit_jmp(void* self, const hook_desc& desc);
-    hook_desc                   m_descs[4];
-    int                         m_desc_count;
+private:
+    PVOID                       m_self = nullptr;
+    repair_iat_node*            m_repair_iat = nullptr;
+    hook_iat_desc               m_descs[4];
+    int                         m_desc_count = 0;
+    bool                        m_pending = false;
 };
 
 //------------------------------------------------------------------------------
 template <typename RET, typename... ARGS>
-bool hook_setter::add_iat(void* module, const char* name, RET (__stdcall *hook)(ARGS...))
+bool hook_setter::add_iat(const char* module, const char* name, RET (__stdcall *hook)(ARGS...))
 {
-    return (add_desc(hook_type_iat_by_name, module, name, hookptr_t(hook)) != nullptr);
+    return add_desc(module, name, hookptr_t(hook));
 }
 
 //------------------------------------------------------------------------------
 template <typename RET, typename... ARGS>
-bool hook_setter::add_jmp(void* module, const char* name, RET (__stdcall *hook)(ARGS...))
+bool hook_setter::add_jmp(const char* module, const char* name, RET (__stdcall *hook)(ARGS...))
 {
-    return (add_desc(hook_type_jmp, module, name, hookptr_t(hook)) != nullptr);
+    return add_detour(module, name, hookptr_t(hook));
 }

clink/process/include/process/hook.h

@@ -7,3 +7,9 @@ typedef void (__stdcall *hookptr_t)();
 
 hookptr_t hook_iat(void* base, const char* dll, const char* func_name, hookptr_t hook, int find_by_name);
 hookptr_t hook_jmp(void* module, const char* func_name, hookptr_t hook);
+
+struct repair_iat_node;
+
+bool add_repair_iat_node(repair_iat_node*&list, void* base, const char* dll, const char* func_name, hookptr_t trampoline, bool find_by_name=true);
+void apply_repair_iat_list(repair_iat_node*& list);
+void free_repair_iat_list(repair_iat_node*& list);