Uninjection thread safety
Re: Uninjection thread safety
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
--Iconic
Re: Uninjection thread safety
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.
All you do is to call Uninject and Inject DLL again when the dialog is open, when the explorer is copying.
Re: Uninjection thread safety
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
--Iconic
Re: Uninjection thread safety
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. 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.
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.
Re: Uninjection thread safety
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
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
Re: Uninjection thread safety
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?
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?
Re: Uninjection thread safety
Well we are dependent only on the system ones: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.
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.
May I suggest a moving a code modifying a loadcount lower? And abusing that loader lock like this?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.
Code: Select all
AutoUnhook();
LoaderLock->Lock();
ModifyLoader Counts;
FreeLibrary;
LoaderLock->Unlock();
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, 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.
We have no our coded SubDll. It was just an POC examplemadshi 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?
Re: Uninjection thread safety
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 nameThanks iconic for the info about the load count in Windows 8, wasn't aware of that.
http://wj32.org/processhacker/forums/vi ... b1fc7da7af
--Iconic
Re: Uninjection thread safety
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: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.
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:May I suggest a moving a code modifying a loadcount lower? And abusing that loader lock like this?
CriticalSections are recursive so, that shouldn't be a problem.Code: Select all
AutoUnhook(); LoaderLock->Lock(); ModifyLoader Counts; FreeLibrary; LoaderLock->Unlock();
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 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?
Re: Uninjection thread safety
We never call LoadLibrary.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?
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...
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 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...
Re: Uninjection thread safety
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.
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.
Re: Uninjection thread safety
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.