Last Comment Bug 742491 - Make nsWindowsDllInterceptor::AddHook thread-safe on x86-32
: Make nsWindowsDllInterceptor::AddHook thread-safe on x86-32
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla14
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 670967 742849
  Show dependency treegraph
 
Reported: 2012-04-04 12:46 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-04-11 09:16 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (13.12 KB, patch)
2012-04-05 12:03 PDT, Justin Lebar (not reading bugmail)
mh+mozilla: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-04-04 12:46:57 PDT
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...
Comment 1 Justin Lebar (not reading bugmail) 2012-04-04 12:51:41 PDT
> 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.
Comment 2 Justin Lebar (not reading bugmail) 2012-04-04 14:09:14 PDT
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.
Comment 3 Mike Hommey [:glandium] 2012-04-04 15:24:26 PDT
> 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.
Comment 4 Ilan Fraiman 2012-04-05 01:17:08 PDT
(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).
Comment 5 Justin Lebar (not reading bugmail) 2012-04-05 06:13:43 PDT
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?
Comment 6 Ilan Fraiman 2012-04-05 08:51:48 PDT
(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
Comment 7 Justin Lebar (not reading bugmail) 2012-04-05 09:07:07 PDT
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
Comment 8 Justin Lebar (not reading bugmail) 2012-04-05 11:57:12 PDT
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.
Comment 9 Justin Lebar (not reading bugmail) 2012-04-05 12:03:54 PDT
Created attachment 612633 [details] [diff] [review]
Patch v1

It should go without saying that this needs a careful review. :)
Comment 10 Mozilla RelEng Bot 2012-04-05 13:04:38 PDT
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 11 Mike Hommey [:glandium] 2012-04-10 00:20:30 PDT
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.
Comment 12 Justin Lebar (not reading bugmail) 2012-04-10 08:19:50 PDT
> 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
Comment 13 Matt Brubeck (:mbrubeck) 2012-04-11 09:16:39 PDT
https://hg.mozilla.org/mozilla-central/rev/3a89af02f119

Note You need to log in before you can comment on or make changes to this bug.