steven-michaud/HookCase

`reset_hook()` fails in interpose library when `caller` calls hooked function

steven-michaud opened this issue · 1 comments

Here's an example patch hook from the hook library template:

    int (*patch_example_caller)(char *arg1, int (*arg2)(char *)) = NULL;
    
    static int Hooked_patch_example(char *arg1, int (*arg2)(char *))
    {
      int retval = patch_example_caller(arg1, arg2);
      LogWithFormat(true, "Hook.mm: patch_example(): arg1 \"%s\", arg2 \'%p\', returning \'%i\'",
                    arg1, arg2, retval);
      PrintStackTrace();
      // Example of using add_patch_hook() to dynamically add a patch hook for
      // 'arg2'.
      add_patch_hook(reinterpret_cast<void*>(arg2),
                     reinterpret_cast<void*>(dynamic_patch_example));
      // Not always required, but using it when not required does no harm.
      reset_hook(reinterpret_cast<void*>(Hooked_patch_example));
      return retval;
    }

The bug is that, whenever patch_example_caller() indirectly calls a function that has also been hooked with a patch hook, reset_hook() doesn't work -- it doesn't re-hook patch_example_caller().

Most of the time this doesn't matter. Most functions have a standard prologue in machine code:

    push  %rbp
    mov   %rsp, %rbp

For patch hooks on these functions, the call to reset_hook() is in effect a no-op, and isn't necessary. But for functions without a standard prologue, not calling reset_hook() at the end of your patch hook means that the hook is only called once. The same applies if the call to reset_hook() fails.

The bug is caused by a problem with the code I added (in HookCase versions 3.3 and 3.3.1) to support dynamic patch hooks. I assumed that while a patch hook is running, no other patch hook can run on the same thread. But this isn't true -- other patch hooks will run on the current thread if they're called (indirectly) from the hook's "caller" (which calls the hook's original function).

This only makes a difference if you patch a function with a non-standard prologue. This is something I don't do very often, so it took me a while to notice the bug.

I have a fix for this bug, which I'm about to land.