DLL interceptor's AddHook should fail if the function has already been hooked

NEW
Unassigned

Status

Firefox Build System
General
7 years ago
4 months ago

People

(Reporter: glandium, Unassigned)

Tracking

Trunk
All
Windows 7

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 years ago
We got a repeatable crash in plugins (bug 684215) after the landing of bug 681238. That crash was fixed by landing bug 681924. The reason this happened is that AddHook was called repeatedly on functions that were already hooked.

Before bug 681238, the first AddHook call would create the hook, which effectively replaces the first few instructions of the function by a jump to the given function. The second AddHook call would fail because the function would now contain a jump instruction, that the trampoline creation code doesn't handle.

Bug 681238 added support for jump instructions, so the second AddHook effectively works, so what happens then is that the newly created trampoline contains a jump to the function given the first time to AddHook, and the hooked function contains a jump to the function given the second time. Since the given function is supposed to call the trampoline, what effectively happens when calling the hooked function is that the given function is called a first time, calls the new trampoline, which calls the function again, etc.

I think we should force AddHook to fail the second time to avoid such situations in the future.
(In reply to Mike Hommey [:glandium] from comment #0)
> I think we should force AddHook to fail the second time to avoid such
> situations in the future.

Root cause of bug 681924 is that sUser32* is overwritten.  So I don't think that AddHook disallow multiple injection.

But we should assert if passing same trampo address (3rd param) to AddHook().
(Reporter)

Comment 2

7 years ago
(In reply to Makoto Kato from comment #1)
> (In reply to Mike Hommey [:glandium] from comment #0)
> > I think we should force AddHook to fail the second time to avoid such
> > situations in the future.
> 
> Root cause of bug 681924 is that sUser32* is overwritten.  So I don't think
> that AddHook disallow multiple injection.
> 
> But we should assert if passing same trampo address (3rd param) to AddHook().

That however doesn't entirely guarantee that the caller is not doing something fishy. On the other hand, before I added the support for the jump instruction on x86, AddHook simply didn't work the second time, so we shouldn't have code relying on being able to rehook an already hooked function. I think avoiding a second hook on the same function is the safest thing. Especially when you also consider the DllInterceptor destructor. Actually, I think the safest would be to disallow *any* further AddHook (even from a different DllInterceptor for the same dll), though detecting it might be tricky.

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.