Unsafe hooks, lpOriginal is not set right after resuming threads

[prsyahmi]

Here's the problem, when we call Hook(), the function calls RemoteHook() which then build the trampoline, suspend the threads and do the actual hooking. Once hooked, all the threads is resumed and one of the threads which calls the hooked functions will crash if the detoured function try to call lpOriginal function too, before the Hook() actually return and give proper lpOriginal address.

Let me illustrate a bit:

Thread 1 -------------------------- Thread 2
Hook(&lpOriginal) -----------------
RemoteHook() ----------------------
SuspendThreads() ------------------ Suspended
ActualHooking ---------------------
ResumeThreads() ------------------- Running
----------------------------------- Call hooked function
----------------------------------- Call original function (lpOriginal)
----------------------------------- crash due to lpOriginal not set yet
RemoteHook() return ---------------
Hook() return with lpOriginal set -

[mxmauro]

Hi Syahmi,

Are you trying to hook a function used by Deviare when it is hooking? Or the call is done by another thread after resuming them but before the Hook method return?

Regards,
Mauro.

[prsyahmi]

Hi Mauro,

I think the call is done by another thread after resuming them but before the Hook method return if I understand you correctly.

I will show pesudo code example to illustrate the problem better.

void* lpCalculateRealtimeOrig = nullptr;

int CalculateRealtime()
{
    return 1;
}

int MyCalculateRealtime()
{
     return lpCalculateRealtimeOrig();
}

DWORD WINAPI Thread2(LPVOID)
{
   for (;;) {
       CalculateRealtime();
   }
}

int main()
{
    CNktHookLib hook;
    CreateThread(Thread2);

    // The hooking by default pauses all other threads leaving only current thread running
    hook.Hook(&idCalcRt, (LPVOID*)&lpCalculateRealtimeOrig, CalculateRealtime, MyCalculateRealtime);
    // Once the hooking is done, it resumes all other threads...
    // However, the calls above isn't returned yet. So the lpCalculateRealtimeOrig is/might still null while the thread 2 already run.
}

Above illustrate the problem better, what should be done for safe hooking is the lplpCallOriginal is set right before hooking on this line: https://github.com/nektra/Deviare-InProc/blob/master/Src/Lib/NktHookLib.cpp#L662 instead of passing the value and assign it later after hooking is done and threads resumed here: https://github.com/nektra/Deviare-InProc/blob/master/Src/Lib/NktHookLib.cpp#L152

Regards,

[mxmauro]

Hi Syahmi,

Thanks for the hint. I will apply the change on next working day.

Regards.

[mxmauro]

Hi Syahmi,

After checking the code, you can call the Hook method that uses the HOOK_INFO parameter. The fields are filled before actual hooking occurs. In the other Hook method, fields are copied from a temporary HOOK_INFO variable to destination.

Regards,
Mauro.

[prsyahmi]

Hi Mauro,

I think I will propose a fix for this problem. Stay tune.

---

Sorry, I had an unexpected errand. So I'll try to fix when there is time.

There are several ways of fixing this:

1. Move all real hooking function to this signature DWORD CNktHookLib::RemoteHook(__out SIZE_T *lpnHookId, __out LPVOID *lplpCallOriginal, __in DWORD dwPid, __in LPVOID lpProcToHook, __in LPVOID lpNewProcAddr, __in DWORD dwFlags)
   In this case, we'll get access to lplpCallOriginal directly so we can assign it before the hooking begin or thread resumed. While the other RemoteHook() with the HOOK_INFO array is passed to this function as address for example &aHookInfo[nHookIdx].lpCallOriginal to the lplpCallOriginal. This should give consistent result for all 4 hook functions call. So it is just flipping the function signature with some minor modification on the code. This is what I want to do in my proposal.

2. This might requires more changes, by resuming thread after lplpCallOriginal assignment. So instead it resume the thread on RemoteHook it resume on Hook or 3 others that call this very RemoteHook. This might introduce more headache than actually to solve this problem.

3. Introduce lplpCallOriginal to HOOK_API structure. Since it is an API change, this is my least preferred.

[mxmauro]

Hi Syahmi,

I uploaded a fix (among other for low IL processes). Please try it.

Regards.

[prsyahmi]

Hi Mauro,

Sorry I couldn't get back to you earlier. The commit should fix the problem. Thanks.