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

Status: NEW

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Makoto Kato [:m_kato]]

(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().

Comment 2

[Mike Hommey [:glandium]]

(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.