Where to clean up stuff in injected DLL

[EaSy]

Hi,
as a result of our last discussion viewtopic.php?f=7&t=27415 we don't use manual unhooking inside DLL detach anymore. Sadly, we have encountered another issue during runtime DLL uninjection (UninjectAllLibrariesW). Take a look at attached screenshot. You can see that we are already behind FinalizeMadCHook call, there is another madcodehook thread running and some of the hooks are still active. The problem is that after the FinalizeMadCHook's call we do some memory clean up in DllMain and this clearly creates some unexpected behaviour inside NtDeviceIoControlFileHook's aftercall logic that is using that memory.

So, when and where should we clean up memory and destroy our contexts? We have tried both DLLMain and global object destructor. Both have the same problem. Thx.

threads.png (81.57 KiB) Viewed 12944 times

[madshi]

Please look at the MS DllMain documentation:

http://msdn.microsoft.com/en-us/library ... 85%29.aspx

It says: "If fdwReason is DLL_PROCESS_DETACH, lpvReserved is NULL if FreeLibrary has been called or the DLL load failed and non-NULL if the process is terminating."

So basically, in DLL_PROCESS_DETACH, if lpvReserved is non-NULL, the process is terminating. In this situation I would suggest to simply not cleanup your memory at all. That should take care of the problem, because if the DLL is uninjected, DLL_PROCESS_DETACH is only called once all hooks are fully removed and inactive. Cleanup is only useful if your DLL is uninjected. Cleanup is not needed when the process is terminating because Windows will then cleanup automatically, anyway.

[EaSy]

Yes, I understand that during process termination we can safely leave memory leaks there (just to be sure). But this bug (crashing app) is happening during DLL uninjection and not during process termination. Am I missing something?

[madshi]

Hmmmm... That's weird. Are you using the "NO_SAFE_UNHOOKING" flag when calling HookAPI()? Otherwise uninjection should not reach DLL_PROCESS_DETACH until after all hooks are successfully uninstalled and inactive.

[EaSy]

Find all "NO_SAFE_UNHOOKING", Subfolders, Find Results 1, "Entire Solution"
Matching lines: 0 Matching files: 0 Total files searched: 3371

No.

[EaSy]

Flags in HookAPI are always ZERO. The only thing he is not using right is that he has -unsafeStopAllowed flag in driver configuration. Is it a factor in this case? He will restart and check it with -safeStopAllowed soon.

[madshi]

No, that shouldn't matter.

Is there any easy way for me to reproduce the problem on my PC? A small test project would be great...

[EaSy]

I'll do my best. First, I will try to reproduce it on my PC, then I will try to modify one of your samples and send it to you here.

[EaSy]

I was not able to reproduce issue on my computer. So I had to debug it myself and I managed to identify possible issue. You create remote thread that handles dll unload. The problem might be that on his PC (crashing) this thread is created more then once. In one case I counted as much as 4 remote threads running concurently in one moment! On my PC (unable to reproduce) I counted 2 remote threads at one time... Is there a reason to create more then one uninjecting remote thread at the time? I have also checked whether we mistakenly call UninjectAllLibrariesW more then once or not, but this was false. We call UninjectAllLibrariesW no more than once. If you look at the screenshot from my first post, you can see two threads related to the madchook callstack. My guess is that first thread finishes succesfully (the one in DLLMain), but the second one dies once it tries to access our DLL again. What do you think?

[madshi]

Sorry for the late reply.

Hmmmm... The remote thread just calls FreeLibrary, nothing else. And DLL_PROCESS_DETACH is only called when the load counter reaches 0. So having multiple remote threads calling FreeLibrary should not be a problem at all. So I would say that this is not likely to be the cause of the problem.

[EaSy]

Well, not quite true.

Let's take a look at your uninjection thread code. There is FreeLibrary call you mentioned, but there is also the AutoUnhook call called before that. AutoUnhook calls AutoUnhook2, that locks gHookCriticalSection, copies active hooks for current module from g_pHookCollection, unlocks gHookCriticalSection and calls UnhookInternal for every one of them. UnhookInternal locks gHookCriticalSection and then erases hook from g_pHookCollection, but then it unlocks gHookCriticalSection! and starts waiting for current (erased) active hook (this may be for example SleepHook(5000)). This allows another thread to get into AutoUnhook2, free to lock gHookCriticalSection. It copies active hooks again (without the SleepHook) and then unhooks the rest, AutoUnhook finishes, thread calls FreeLibrary. This happends concurently with the first thread still waiting for SleepHook till it ends. Meanwhile, our injected DLL is deatached, it calls AutoUnhook without waiting, so it does nothing and we cleanup stuff, frees madhook context, and unloads dll, while the first thread is still waiting for that one active hook (SleepHook). Then it is matter of time who dies first... SleepHook or the UnhookInternal busy waiting thread, I guess both because no instructions are available anymore. DLL is already detached, because you've set LoadCount from 0xFF to 1. So, the first FreeLibrary call destroys that DLL.

I hope you see it now

[EaSy]

I propose something like this.

from:

Code: Select all

if ((HMODULE) dll->DllBase == hModule)
{
   if (ir.Load)
     dll->LoadCount = 0xff;
   else
     dll->LoadCount = 1;
   break;
}

to:

Code: Select all

if ((HMODULE) dll->DllBase == hModule)
{
   if (ir.Load)
      dll->LoadCount = 0xff;
   else {
      if( dll->LoadCount == 0xff )
         dll->LoadCount = 0;
      dll->LoadCount++;
   }
   break;
}

[madshi]

You're right. Don't know how I could miss that...

I'll think about how to fix this. I'm not sure if your suggested fix covers all possible problems. Will let you know when a new build is available.

[EaSy]

Thx.

[EaSy]

Any progress? When can we expect fix? Thx.