Repeated failed function hook attempts cause exhaustion of interceptor trampoline space

RESOLVED FIXED in Firefox 62

Status

()

P1
critical
RESOLVED FIXED
10 months ago
9 months ago

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)

(Reporter)

Description

10 months ago
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.
status-firefox62: --- → affected
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

Updated

10 months ago
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]

Comment 3

10 months ago
FYI, david is on pto. He'll be back next week.
status-firefox60: unaffected → affected
Priority: -- → P3
Whiteboard: inj+
Keywords: crash

Updated

9 months ago
Flags: needinfo?(davidp99)

Updated

9 months ago
status-firefox59: unaffected → ---
status-firefox60: affected → ---
status-firefox61: affected → ---
status-firefox62: affected → ---
status-firefox-esr52: unaffected → ---
status-firefox-esr60: unaffected → ---

Updated

9 months ago
Keywords: regression
(Assignee)

Updated

9 months ago
Assignee: nobody → davidp99
Priority: P3 → P1
(Assignee)

Comment 4

9 months ago
Created attachment 8980137 [details] [diff] [review]
Distinguish between unregistered and failed DLL function interceptions

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+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 5

9 months ago
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

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f9400fe51c4
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.