The default bug view has changed. See this FAQ.

Make nsWindowsDllInterceptor::AddHook thread-safe on x86-32

RESOLVED FIXED in mozilla14

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla14
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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...
(Assignee)

Updated

5 years ago
Blocks: 670967
(Assignee)

Comment 1

5 years ago
> 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.
(Assignee)

Comment 2

5 years ago
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.

Comment 4

5 years ago
(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).
(Assignee)

Comment 5

5 years ago
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

5 years ago
(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
(Assignee)

Comment 7

5 years ago
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
Component: General → GFX: Color Management
QA Contact: general → color-management
(Assignee)

Updated

5 years ago
Component: GFX: Color Management → General
QA Contact: color-management → general
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Updated

5 years ago
OS: Linux → Windows XP
(Assignee)

Updated

5 years ago
Hardware: x86_64 → x86
(Assignee)

Comment 9

5 years ago
Created attachment 612633 [details] [diff] [review]
Patch v1

It should go without saying that this needs a careful review. :)
Attachment #612633 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Summary: nsWindowsDllInterceptor::AddHook is not thread-safe → Make nsWindowsDllInterceptor::AddHook thread-safe on x86-32
(Assignee)

Updated

5 years ago
Blocks: 742849

Comment 10

5 years ago
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.
Attachment #612633 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 12

5 years ago
> 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
(Assignee)

Updated

5 years ago
Attachment #612633 - Attachment description: Bug 742491 - Use a thread-safe DLL patcher on Windows, when possible. (v0.1) → Patch v1
https://hg.mozilla.org/mozilla-central/rev/3a89af02f119
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.