Uninjection thread safety

c++ / delphi package - dll injection and api hooking
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

This still seems that you're attempting a stress test and nothing else, one which is highly unrealistic in real-world scenarios considering the original topic of trying to inject and/or uninject from multiple threads at the same time. I fail to see any logic in this, want to enlighten us? You've been presented very scientific answers to your concerns thus far, from what I see

--Iconic
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

Well I do see a simple logic. If a dll hooks functions that takes a long time to finish, like GetFileOpenDialog, IFileOperation::PerformOperations and so on, it is just a matter of time until it crashes.
All you do is to call Uninject and Inject DLL again when the dialog is open, when the explorer is copying.
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

In a "SAFE_HOOKING" scenario nothing should be uninjected until said operation has completed since hooks need be unhooked prior to freeing said DLL. Clearly, this is not what is happening, correct? Also, Explorer's "dialogs" during copying rely on interfaces, as you know in modern versions of Windows, and function asynchronously mostly. Regardless, this doesn't seem like a tough problem to remedy in MCH. I believe that misunderstanding the true problem is causing too much speculation

--Iconic
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

Yes, you are right. But this is not about safe unhooking. It works just fine. The problem is about how the Inject and Uninject routines are written in the source code... I mean order of operations (unhooking vs. load/free libs), thread safety and some rare "else cases" that are not covered in there. 99% of MCH's source code is well written. I am just trying to show Madshi how to see the bugs comming from the 1%. If anyone wants to use MCH in production code, that 1% needs to be addressed... it is quite a lot in a real world. :D The more we use MCH in a complex way, the more we see there rare bugs.

Well I could modify the source code myself a be done with it. But I want a discussion about this. I want to have it added into the MCH so other won't have to solve this.

Enough with the flame war. I've explained problem well enough, hope Madshi will see the bug and fix it.
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

Rozumím ;)

Never a flame war my friend, just trying to understand the true root of what you consider an issue within the parameters or real-world usage.

--Iconic
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

Thanks iconic for the info about the load count in Windows 8, wasn't aware of that.

Ok, at least we're collecting some more information: Seems the problem occurs only if injection and uninjection is performed at the same time, *and* if the hook dll statically links to another dll. Is that correct?

There's a reason the hooking rule 4 says:

http://help.madshi.net/HookingRules.htm

> In your hook DLL link to as few DLLs as possible.

Anyway, I wonder what the best way is to solve this problem? The injecting EXE doesn't really keep track of when injection or uninjection is complete, because in theory it can take a long time. Uninjection can even take "forever" (due to the SAFE_UNHOOKING logic). I suppose the cleanest solution would be to avoid running multiple (un)injecting actions at the same time. But it's hard for me to avoid that because (as said above) I don't fully keep track on when an (un)injection has truely completed, for every running process.

I suppose I could add some critical section to the (un)injection thread that blocks until injection or uninjection has run through. But if hook DLL "A" fails to uninject, this would effectively stop any other hook DLL from ever be injected or uninjected again in that process. I don't think that's a very good solution.

@EaSy, I hate to put this on you, but isn't there some way you can avoid having uninjection and injection run "at the same time"? No other user has ever reported a problem like this to me, probably because nobody performs uninjection and injection simultanouesly. I could try to block things within madCodeHook, but since madCodeHook can be used for multiple different hook dlls, I'm not sure if that's a good idea. Waiting for hook dll A might block actions for hook dll B.

@EaSy, another question: Would it be hard to get rid of the static (and ideally also dynamic) linking to that subdll? E.g. can't you combine both into one?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

madshi wrote: There's a reason the hooking rule 4 says:
http://help.madshi.net/HookingRules.htm
> In your hook DLL link to as few DLLs as possible.
Well we are dependent only on the system ones:
WTSAPI32.DLL, PSAPI.DLL, WS2_32.DLL, KERNEL32.DLL,
USER32.DLL, GDI32.DLL, WINSPOOL.DRV, ADVAPI32.DLL,
SHELL32.DLL, OLE32.DLL, OLEAUT32.DLL, RPCRT4.DLL,
MSVCP120.DLL, MSVCR120.DLL, SHLWAPI.DLL,
VERSION.DLL, MPR.DLL, SECUR32.DLL, NETAPI32.DLL,
USERENV.DLL, IPHLPAPI.DLL, WSOCK32.DLL

All of them are used because of hooks or because we use some API from them.
Dunno which one it causes a crash, but I suspect that any of them dynamically loaded with the HOOKDLL.
madshi wrote: Anyway, I wonder what the best way is to solve this problem? The injecting EXE doesn't really keep track of when injection or uninjection is complete, because in theory it can take a long time. Uninjection can even take "forever" (due to the SAFE_UNHOOKING logic). I suppose the cleanest solution would be to avoid running multiple (un)injecting actions at the same time. But it's hard for me to avoid that because (as said above) I don't fully keep track on when an (un)injection has truely completed, for every running process.

I suppose I could add some critical section to the (un)injection thread that blocks until injection or uninjection has run through. But if hook DLL "A" fails to uninject, this would effectively stop any other hook DLL from ever be injected or uninjected again in that process. I don't think that's a very good solution.
May I suggest a moving a code modifying a loadcount lower? And abusing that loader lock like this?

Code: Select all

AutoUnhook();

LoaderLock->Lock();
ModifyLoader Counts;
FreeLibrary;
LoaderLock->Unlock();
CriticalSections are recursive so, that shouldn't be a problem.
madshi wrote: @EaSy, I hate to put this on you, but isn't there some way you can avoid having uninjection and injection run "at the same time"? No other user has ever reported a problem like this to me, probably because nobody performs uninjection and injection simultanouesly. I could try to block things within madCodeHook, but since madCodeHook can be used for multiple different hook dlls, I'm not sure if that's a good idea. Waiting for hook dll A might block actions for hook dll B.
Well, calling MCH functions is threadsafe in our code, but since MCH uses timeouts, I can not do anything about it... or can I somehow?
madshi wrote: @EaSy, another question: Would it be hard to get rid of the static (and ideally also dynamic) linking to that subdll? E.g. can't you combine both into one?
We have no our coded SubDll. It was just an POC example ;)
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

Thanks iconic for the info about the load count in Windows 8, wasn't aware of that.
No problem, I have described the changes on another forum over 3 years ago with the loader in Windows 8+. You can see it here, you know my first name
http://wj32.org/processhacker/forums/vi ... b1fc7da7af

--Iconic
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

EaSy wrote:Well we are dependent only on the system ones:
WTSAPI32.DLL, PSAPI.DLL, WS2_32.DLL, KERNEL32.DLL,
USER32.DLL, GDI32.DLL, WINSPOOL.DRV, ADVAPI32.DLL,
SHELL32.DLL, OLE32.DLL, OLEAUT32.DLL, RPCRT4.DLL,
MSVCP120.DLL, MSVCR120.DLL, SHLWAPI.DLL,
VERSION.DLL, MPR.DLL, SECUR32.DLL, NETAPI32.DLL,
USERENV.DLL, IPHLPAPI.DLL, WSOCK32.DLL

All of them are used because of hooks or because we use some API from them.
Dunno which one it causes a crash, but I suspect that any of them dynamically loaded with the HOOKDLL.
Well, you don't need to load them in order to hook them. Also, I can't imagine it's a dynamic load, because if I call FreeLibrary on your hook dll, this doesn't have any effect on other dynamically loaded dlls at all, unless you call FreeLibrary in your dll's DLL_PROCESS_DETACH handling. But then, as we figured out, calling FreeLibrary itself is safe. So my best guess would be that it must be a static link to a dll which wouldn't be loaded in the target process without your hook dll. Because only then freeing your library could possibly have an crash effect on another loaded dll. Wouldn't you agree?
EaSy wrote:May I suggest a moving a code modifying a loadcount lower? And abusing that loader lock like this?

Code: Select all

AutoUnhook();

LoaderLock->Lock();
ModifyLoader Counts;
FreeLibrary;
LoaderLock->Unlock();
CriticalSections are recursive so, that shouldn't be a problem.
That sounds like a good idea to me. I'm not 100% sure it would solve the problem, but I can see that it should be an improvement and shouldn't harm in any way, as far as I can see.
EaSy wrote:Well, calling MCH functions is threadsafe in our code, but since MCH uses timeouts, I can not do anything about it... or can I somehow?
I see the problem. But I have the same problem in madCodeHook, as well. And a big problem is that it wouldn't even be good enough to add some critical sections or stuff into the madCodeHook lib, because in theory process A could uninject and process B could reinject the same DLL. So to make it absolutely fool proof, I'd have to add checks which surpass process boundaries. That's really complicated...
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

madshi wrote: Well, you don't need to load them in order to hook them. Also, I can't imagine it's a dynamic load, because if I call FreeLibrary on your hook dll, this doesn't have any effect on other dynamically loaded dlls at all, unless you call FreeLibrary in your dll's DLL_PROCESS_DETACH handling. But then, as we figured out, calling FreeLibrary itself is safe. So my best guess would be that it must be a static link to a dll which wouldn't be loaded in the target process without your hook dll. Because only then freeing your library could possibly have an crash effect on another loaded dll. Wouldn't you agree?
We never call LoadLibrary.
All of them are "static" in our DLL. But if MCH loads our DLL dynamically, won't the dependent dlls load dynamically too? I really dunno how the reference counts exactly work. But according to crashes they work that way... :(
madshi wrote: I see the problem. But I have the same problem in madCodeHook, as well. And a big problem is that it wouldn't even be good enough to add some critical sections or stuff into the madCodeHook lib, because in theory process A could uninject and process B could reinject the same DLL. So to make it absolutely fool proof, I'd have to add checks which surpass process boundaries. That's really complicated...
I think this is not needed. Once you have thread safe injection/uninjection threads, you don't need to syncronize it in higher levels. I would focus on routines running inside other processes.
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

Yeah, I suppose if your dll has them statically linked, the OS will load them dynamically. And maybe it then gets confused due to my loader count manipulations.

So do you need a new build from me with the suggested loader count manipulation changes to test if that solves the problem? Or do you want to change the code yourself?

FWIW, atm I'm only doing those manipulations for x86, but not for x64.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

It will be better if you do it. There are some bytearray versions too. It will be better to do it right first time. I will try to test it on my side with openfiledialoghook or something like that.
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

Test build available here:

http://madshi.net/madCollectionBeta.exe (2.7.11.7)
Post Reply