Split off from bug 741540. Even if we call nsWindowsDllInterceptor::AddHook before we start any threads, someone might inject code which starts a thread and calls a method that we add a hook to. This is particularly important because we override VirtualAlloc (basically, mmap) in AvailableMemoryTracker -- it's likely that any new thread is going to call this early in its lifecycle. There was some discussion in bug 741540 as to whether this is feasible. It sounds like it should be feasible on x86-32, due to the padding within and without the function. But perhaps this is not feasible on x86-64, because the padding isn't there. On x86-64, perhaps we could look to see if there are any live threads, and bail if there are. That might not be racy, assuming that when I start a new thread, that |have other threads been started?| flag is immediately true, from my point of view. I wonder: Is there an equivalent of the global offset table we could patch? That would be a lot easier...

> I wonder: Is there an equivalent of the global offset table we could patch? That would be a lot > easier... At least for my VirtualAlloc wrapper, I don't much care if the code in the same DLL which calls it goes through my wrapper. I'm happy if that code calls the unwrapped VirtualAlloc. So it's OK if that code doesn't go through the global offset table.

So upon some thought, I think we should first solve this problem on x86-32, where we know how to solve it, and worry about x86-64 later. Win64 is not a tier-1 platform for us atm, and even if we tried to solve the problem there, we don't have a large enough audience to effectively test our solution.

> I wonder: Is there an equivalent of the global offset table we could patch? That would be a lot > easier... I don't know the gory details, but there are stubs for library calls in each executable or library. But that would be a whole lot of work to find their location in all loaded libraries.

(In reply to Justin Lebar [:jlebar] from comment #2) > So upon some thought, I think we should first solve this problem on x86-32, > where we know how to solve it, and worry about x86-64 later. Win64 is not a > tier-1 platform for us atm, and even if we tried to solve the problem there, > we don't have a large enough audience to effectively test our solution. to think of it, why you chose to use inline patching and not iat+eat patch ? of course inline patching has some advantages and can cope with some situations that iat+eat patches don't cope, but I'm not sure that in your case you need it. the iat patch approach has several advantages: 1) coping with multiple threads is trivial and the actual memory write is safe because it's only a replacement of a rva in the relevant tables. 2) this approach works well on 32bit and 64bit without (almost) any change, so you can easily reuse the code on both platforms. 3) there aren't many gory details here. pe file format is documented well and windows header files contain all relevant structures. 4) this also copes well with 3rd party products that may patch the functions you are patching before you put the patch (and change the function prefix - for example already putting a jmp there).

I can't seem to find anything about iat/eat patching, aside from the fact that it's related to rootkits. :) Can someone point me to details?

(In reply to Justin Lebar [:jlebar] from comment #5) > I can't seem to find anything about iat/eat patching, aside from the fact > that it's related to rootkits. :) Can someone point me to details? basically it's hooking the import address table and export address table. this is probably the most popular method of hooking in windows, because of simplicity and because it was documented by a famous windows internals writer Jeffrey Richter and the pe file was well documented by the equally famous Matt Pietrek. There's an extensive talk about it in the "DLL Injection and API hooking" chapter in Jeffrey Richter's book "Windows VIA C/C++". you can also get information at : http://www.codeproject.com/Articles/2082/API-hooking-revealed http://drdobbs.com/184416246 http://www.exploit-db.com/wp-content/themes/exploit/docs/17671.pdf http://help.madshi.net/ApiHookingMethods.htm

Here's my first attempt at making AddHook thread-safe, at least in the limited win32 case: https://tbpl.mozilla.org/?tree=Try&rev=c132e62bb097

Filed bug 742833 on moving the assignment to the out param in AddHook so the trampoline gets instantiated earlier. This doesn't make us totally thread-safe, but it eliminates one large race.

It should go without saying that this needs a careful review. :)

Try run for c132e62bb097 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=c132e62bb097 Results (out of 49 total builds): success: 43 warnings: 6 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-c132e62bb097

Comment on attachment 612633 [details] [diff] [review] Patch v1 Review of attachment 612633 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/xre/nsWindowsDllInterceptor.h @@ +49,5 @@ > + * Using the built-in nop space works as follows: On x86-32, DLL functions > + * begin with a two-byte nop (mov edi, edi) and are preceeded by five bytes of > + * NOP instructions. > + * > + * When we detect a function with this prelude, we do the following: Is it really a prelude, or "simple" alignment? (i.e. functions are aligned at n-bytes boundary, and space between end of previous function and the current function is nopped?) @@ +202,5 @@ > + return false; > + } > + > + // mov edi, edi. Yes, there are two ways to encode the same thing. > + // Windows seems to use 8bff; I include 89ff out of paranoia. Might as well say why there are two ways to encode it: because 89 is for mov r/m, r and 8b is for mov r, r/m, where r is register and r/m is register or memory. @@ +609,5 @@ > +{ > + internal::WindowsDllNopSpacePatcher mNopSpacePatcher; > + internal::WindowsDllDetourPatcher mDetourPatcher; > + > + bool mDetourPatcherInitialized; Nit: this should be part of the WindowsDllDetourPatcher class.

> Is it really a prelude, or "simple" alignment? (i.e. functions are aligned at n-bytes boundary, and > space between end of previous function and the current function is nopped?) It's really a prelude. Or at least...there may be more than 5 nop's at the beginning for alignment or whatever, but there are at least 5. http://blogs.msdn.com/b/oldnewthing/archive/2011/09/21/10214405.aspx