976838 - Deleting ProcessLink before ::Open may access uninitialized mTransport/mIOLoop

Status: RESOLVED FIXED

(Core :: IPC, defect, P3)

Description

[John Schoenick [:johns]]

In bug 974933, we're crashing deleting a PluginModuleParent -> PPluginModuleParent -> MessageChannel -> ProcessLink. I believe I've tracked this down to ProcessLink::Open not being called due to an abort [1], which means mTransport and mIOLoop are never initialized, resulting in a bogus |mTransport->set_listener(0)| [2]

[1] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#101

[2] http://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageLink.cpp#69

Comment 1

[John Schoenick [:johns]]

Looks like this was introduced in bug 901789. Unsure if this code gets used in b2g

Comment 2

[John Schoenick [:johns]]

Attachment: Initialize mTransport/mIOLoop in constructor

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8381782 [details] [diff] [review]
Initialize mTransport/mIOLoop in constructor

Review of attachment 8381782 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Let's backport this as far as it needs to go.

Comment 5

[Andrew McCreight [:mccr8]]

I'm going to say sec-high.  Feel free to adjust.

Comment 6

[John Schoenick [:johns]]

@bent Do you know if this code is only used by plugin-IPC or should it apply to b2g as well?

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Well, now that I'm looking at this more closely I'm not sure this is such a big deal after all. The issue isn't that PluginModuleParent is created/destroyed and Open never called, it's that ProcessLink could be created and destroyed with Open never being called.

I don't see anywhere where that could happen given this:

  http://mxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#296

We should still fix this though.

Am I missing anything here?

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8381782 [details] [diff] [review]
Initialize mTransport/mIOLoop in constructor

Review of attachment 8381782 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/MessageLink.cpp
@@ +59,5 @@
>  ProcessLink::ProcessLink(MessageChannel *aChan)
>    : MessageLink(aChan),
> +    mExistingListener(nullptr),
> +    mTransport(nullptr),
> +    mIOLoop(nullptr)

Oh, please make sure these are in the correct order.

Comment 9

[Benjamin Smedberg]

The crash at https://crash-stats.mozilla.com/report/index/eb39173b-4faa-4dcc-b47e-ca1862140219 clearly indicates that we're operating on poisoned memory.

Uninitialized is a possibility although bent's right that this seems unlikely. It's also possible that we're just operating on a dead object and have a dangling pointer. Here are the possibilities I can think of:

* double-deleting the PluginModuleParent
* the channel is a member of PluginModuleParent so that's not a problem
* in MessageChannel::Clear the pointer mLink
* in ProcessLink::~ProcessLink the pointer to mTransport
* in Channel::set_listener the pointer to channel_impl_

My intuition is that we're somehow deleting the PluginModuleParent nested inside its own destructor somehow.

Comment 10

[John Schoenick [:johns]]

You're right, I was under the impression ::Open() was called directly from ModuleParent::Open. If that's not happening, that can't be the cause. Argh.

Comment 11

[Andrew McCreight [:mccr8]]

John, what sort of security rating should this get?  Is this not actually an issue after all?  Or maybe something like sec-moderate?  Thanks.

Comment 12

[John Schoenick [:johns]]

I think per comment 7 this isn't a security issue, though it should probably stay protected until the memory corruption issue it's talking about is resolved (or, the relevant comments all hidden, I guess)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this still relevant?

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Probably still relevant, but there have been some changes to the IPC core over the past few years.  :billm might know.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The original crash seems to have gone away. There's a patch that initializes some stuff, which is nice. But I don't think this stuff should ever be accessed before Open, when it's currently initialized. The patch still seems nice to guard against unspecified badness, but I don't think we need to spend too much time wondering how the crash might have happened or got fixed.

Perhaps we should just take the patch and close the bug. Does that sound okay to you Jed?

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Seems reasonable.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This was actually fixed by the patch for bug 1044322.