Infinite recursion from GetThreadID in Debugger

[michaelweiser]

Hi,

I'm seeing stack overflow exceptions on Windows 10 with even the simplest program doing a single API call:

#include <stdio.h>
#include <Windows.h>

int main()
{
    Sleep(10000);
    DWORD threadid = GetCurrentThreadId();
}

Unfortunately, I was not able to grab any meaningful backtrace beyond it happening in enter_hook() and operate_on_backtrace().
Through single stepping the code I think to have found the root cause but can only describe it verbally with links into the capemon source code:

• enter_hook() calls __called_by_hook() to prevent hook recursion:

  capemon/hooking.c

  Line 293 in e62f1a4

  if ((hookinfo->disable_count < 1) && (h->allow_hook_recursion || (!__called_by_hook(sp, ebp_or_rip) /*&& !is_ignored_thread(GetCurrentThreadId())*/))) {

• __called_by_hook() runs addr_in_our_dll_range() via operate_on_backtrace():

  capemon/hooking.c

  Line 181 in e62f1a4

  return operate_on_backtrace(stack_pointer, frame_pointer, NULL, addr_in_our_dll_range);

• operate_on_backtrace() in the 64 bit version runs our_stackwalk() to retrieve the number of strack frames to look at:

  capemon/hooking_64.c

  Line 1168 in e62f1a4

  frames = our_stackwalk(_rip, sp, backtrace, HOOK_BACKTRACE_DEPTH);

• our_stackwalk() will return zero if the SRW lock is held or an EXCEPTION_EXECUTE_HANDLER exception occurs (I'm fuzzy on the details of the latter):

  capemon/hooking_64.c

  Line 1126 in e62f1a4

  if (srw_lock_held())

  capemon/hooking_64.c

  Line 1150 in e62f1a4

  __except(EXCEPTION_EXECUTE_HANDLER)

• this will cause operate_on_backtrace() to never call addr_in_our_dll_range() and will default to returning zero

This in the context of __called_by_hook() means that enter_hook() was not triggered from another hook. This essentially creates potential for unwanted hook recursion whenever the SRW lock is held or that execution exception occurs during stack unwinding.

This seems to quite reliably be triggered and turned into infinite recursion by the Debugger:

• after the above __called_by_hook() having told enter_hook() that it was not called by a hook, api_dispatch() is called
• api_dispatch() may (and in my observation basically always does) call InitNewThreadBreakpoints()
• InitNewThreadBreakpoints() calls CreateThreadBreakpoints()
• CreateThreadBreakpoints() calls GetThreadId()
• GetThreadId() internally (at least on Windows 10) calls NtQueryInformationThread() -> which is hooked

This causes instantaneous inifinite hook recursion on any hooked API call (at the very least if the SRW lock is held), leading to the observed stack overflow.

To recap, the call chain is:

/any API call/ -> [recurse: enter_hook() + __called_by_hook() == 0 -> api_dispatch() -> InitNewThreadBreakpoints() -> CreateThreadBreakpoints() ->GetThreadId() -> NtQueryInformationThread()]

My workaround looks like this:

diff --git a/hooking.c b/hooking.c
index 443ae50..d1af8ef 100644
--- a/hooking.c
+++ b/hooking.c
@@ -178,7 +178,14 @@ int addr_in_our_dll_range(void *unused, ULONG_PTR addr)

 static int __called_by_hook(ULONG_PTR stack_pointer, ULONG_PTR frame_pointer)
 {
-       return operate_on_backtrace(stack_pointer, frame_pointer, NULL, addr_in_our_dll_range);
+       int rc = operate_on_backtrace(stack_pointer, frame_pointer, NULL, addr_in_our_dll_range);
+       if (rc < 0) {
+               // be cautious if we couldn't operate on the backtrace at all. This can be
+               // due to SRW lock being held or exceptions when trying to evaluate the backtrace.
+               return 1;
+       }
+
+       return rc;
 }

 int called_by_hook(void)
diff --git a/hooking_64.c b/hooking_64.c
index 153fba5..04d3230 100644
--- a/hooking_64.c
+++ b/hooking_64.c
@@ -1112,7 +1112,7 @@ BOOL srw_lock_held()
        return FALSE;
 }

-static unsigned int our_stackwalk(ULONG_PTR _rip, ULONG_PTR sp, PVOID *backtrace, unsigned int count)
+static int our_stackwalk(ULONG_PTR _rip, ULONG_PTR sp, PVOID *backtrace, unsigned int count)
 {
        /* derived from http://www.nynaeve.net/Code/StackWalk64.cpp */
        __declspec(align(64)) CONTEXT ctx;
@@ -1124,7 +1124,7 @@ static unsigned int our_stackwalk(ULONG_PTR _rip, ULONG_PTR sp, PVOID *backtrace
        unsigned int frame;

        if (srw_lock_held())
-               return 0;
+               return -1;

        __try
        {
@@ -1149,17 +1149,17 @@ static unsigned int our_stackwalk(ULONG_PTR _rip, ULONG_PTR sp, PVOID *backtrace
        }
        __except(EXCEPTION_EXECUTE_HANDLER)
        {
-               return 0;
+               return -1;
        }
 }

 int operate_on_backtrace(ULONG_PTR sp, ULONG_PTR _rip, void *extra, int(*func)(void *, ULONG_PTR))
 {
-       int ret = 0;
+       int ret = -1;
        PVOID backtrace[HOOK_BACKTRACE_DEPTH];
        lasterror_t lasterror;
-       WORD frames;
-       WORD i;
+       int frames;
+       int i;

        get_lasterrors(&lasterror);

What this does is make our_stackwalk() indicate the inability to walk the stack at all by returning -1. This will still make operate_on_backtrace() not call addr_in_our_dll_range() but the changed return code default of -1 will again indicate that fact to the caller. The only caller evaluating the return code at all is __called_by_hook(). There we now cautiously return 1, meaning "yes, we've been or at least could have been called from a hook". This successfully prevents the infinite recursion and subsequent stack overflow in my tests.

Does any of that make sense?

[kevoreilly]

Yes! It makes perfect sense. I now can't believe I didn't see this before. Thanks for shedding light - again, two issues here.

Looks like the underlying issue has been there for a long time - both from when I added the srw_lock_held() function and before this (a long time ago) when I added the __try() __except() by returning 0 and not considering this possibility. Not sure why it hasn't come up before, and disappointed in myself for not seeing that returning 0 has the potential for stack recursion.

While I think the CreateThreadBreakpoints() function could be improved by potentially avoiding always calling GetThreadId(), I guess the real point is that it is just triggering this underlying bug in enter_hook(), essentially bringing it to the fore where previously it lay hidden.

Your workaround could in fact be called a fix. I will implement it (or a variant thereof) and get it tested and published asap. I may try and improve CreateThreadBreakpoints() while I'm there.

A massive thanks for this work.

[kevoreilly]

Fix now pushed - thank you again!