Repeated failed function hook attempts cause exhaustion of interceptor trampoline space

RESOLVED FIXED in Firefox 62

Status

()

defect
P1
critical
RESOLVED FIXED
Last year
Last year

People

(Reporter: philipp, Assigned: handyman)

Tracking

61 Branch
mozilla62
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: inj+)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is
report bp-075ea3bc-7eb4-4b01-8703-679530180504.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128>::GetNextTrampoline mozglue/misc/interceptor/VMSharingPolicies.h:38
1 xul.dll mozilla::interceptor::WindowsDllDetourPatcher<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128> >::CreateTrampoline mozglue/misc/interceptor/PatcherDetour.h:402
2 xul.dll mozilla::interceptor::WindowsDllDetourPatcher<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128> >::AddHook mozglue/misc/interceptor/PatcherDetour.h:166
3 xul.dll mozilla::interceptor::WindowsDllInterceptor<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128> >::AddHook mozglue/misc/nsWindowsDllInterceptor.h:192
4 xul.dll mozilla::plugins::BasicFunctionHook<15, int >::Register dom/plugins/ipc/FunctionHook.h:168
5 xul.dll mozilla::plugins::PluginModuleChild::AllocPPluginInstanceChild dom/plugins/ipc/PluginModuleChild.cpp:1821
6 xul.dll mozilla::plugins::PPluginModuleChild::OnMessageReceived ipc/ipdl/PPluginModuleChild.cpp:622
7 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2141
8 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2071
9 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1917

=============================================================

crashes with this signature are newly appearing in firefox 61 on windows, around half of them in the plugin process.
Flags: needinfo?(aklotz)
The main issue that I am seeing in these dumps is that the interceptor's AddHook method is failing, but BasicFunctionHook doesn't distinguish between "we haven't hooked this yet" and "we tried to hook this but failed."

Every time a new plugin instance is created, we re-attempt to set the hook. Each attempt fails, but each attempt also consumes a trampoline, until the trampoline space is eventually exhausted.

While adding a more sophisticated allocator for interceptor trampolines could fix that, I think that would be overkill given the nature of the problem, and is better addressed at the BasicFunctionHook level.

ni? over to David to see what he thinks.
Component: General → Plug-ins
Flags: needinfo?(aklotz) → needinfo?(davidp99)
Summary: Crash in mozilla::interceptor::VMSharingPolicyUnique<T>::GetNextTrampoline → Repeated failed function hook attempts cause exhaustion of interceptor trampoline space
Blocks: 1460002
I have created bug 1460002 as a meta to track similar problems in other areas of the code. Non-plugin crashes with the same signature will be fixed there.
Crash Signature: [@ mozilla::interceptor::VMSharingPolicyUnique<T>::GetNextTrampoline]
FYI, david is on pto. He'll be back next week.
Priority: -- → P3
Whiteboard: inj+
Keywords: crash
Flags: needinfo?(davidp99)
Keywords: regression
Assignee: nobody → davidp99
Priority: P3 → P1
This just does the straight-forward thing of distinguishing between having never attempted to register vs having failed to register.

I had also wanted to move the initialization of the interceptions (the call to HookFunctions) from PluginModuleChild::AllocPPluginInstanceChild since, fundamentally, the mistake is that we keep trying to initialize every time we need a new PPluginInstance but the quirks system looks like it got mangled in the e10s move so we are doing some sketchy stuff about re-initialization that I _think_ is all moot anyway but, in the end, I decided it's not important enough to risk bothering.  Still, this implementation is kind of a cop-out.

---

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac1abcb7feece928d541cd08a9572d8a3ed92813
Attachment #8980137 - Flags: review?(aklotz)
Attachment #8980137 - Flags: review?(aklotz) → review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9400fe51c4
Distinguish between unregistered and failed DLL function interceptions. r=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f9400fe51c4
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.