1290975 - mozilla::ipc::MessageChannel is using WeakReference on more than one thread

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Honza Bambas (:mayhemer)]

This is based on using patch from bug 956338 that adds run-time thread safety checks into our weak references.

I'm getting the following assertion failure short after startup with patch v1 from bug 956338 applied:

 	xul.dll!mozilla::detail::WeakReference<mozilla::ipc::MessageListener>::get() Line 130	C++
 	xul.dll!mozilla::WeakPtr<mozilla::ipc::MessageListener>::operator->() Line 245	C++
>	xul.dll!mozilla::ipc::MessageChannel::EnteredCxxStack() Line 341	C++
 	xul.dll!mozilla::ipc::MessageChannel::CxxStackFrame::CxxStackFrame({...}, IN_MESSAGE, 0x0e8ff9a4) Line 234	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchMessageW({...}) Line 1592	C++
 	xul.dll!mozilla::ipc::MessageChannel::OnMaybeDequeueOne() Line 1568	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>(0x0e6f6038, 0x0f2522b1, {...}, {...}) Line 730	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>(0x0e6f6038, 0x0f2522b1) Line 735	C++
 	xul.dll!mozilla::detail::RunnableMethodImpl<bool (__thiscall mozilla::ipc::MessageChannel::*)(void),0,1>::Run() Line 766	C++
 	xul.dll!mozilla::ipc::MessageChannel::RefCountedTask::Run() Line 550	C++
 	xul.dll!mozilla::ipc::MessageChannel::DequeueTask::Run() Line 571	C++
 	xul.dll!nsThread::ProcessNextEvent(true, 0x0e8ffaf1) Line 1068	C++
 	xul.dll!NS_ProcessNextEvent(0x0e534840, true) Line 290	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(0x0dfbad90) Line 384	C++
 	xul.dll!MessageLoop::RunInternal() Line 233	C++
 	xul.dll!MessageLoop::RunHandler() Line 226	C++
 	xul.dll!MessageLoop::Run() Line 206	C++
 	xul.dll!nsThread::ThreadFunc(0x0e534840) Line 465	C++
 	nss3.dll!_PR_NativeRunThread(0x0e2d13a0) Line 397	C


The WeakReference object involved was created on a different thread (the main thread) than it's being read now (IPDL Background).  This is known to be unsafe, unless there is an absolute certainty the object can't die - but then why using weak ref?

Reporting rather as a sec bug, this might be exploitable.

Comment 1

[Honza Bambas (:mayhemer)]

Josh, you are the one who originally changed mListener to be weak ptr proxy in bug 857152.  This has later been merged to MessageChannel in bug 901789.  Now we are using MessageChannel on multiple threads and using the WeakPtr is then unsafe.

Can you check on this please or reach to other people that could?

Maybe bent could have some insight too.

Comment 2

[Josh Matthews [:jdm]]

This was jlebar's work; I only changed the use of chromium atomics to NSPR. https://bugzilla.mozilla.org/show_bug.cgi?id=857152#c3 is all I know about this, so I recommend billm instead.

Comment 3

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

Yeah, it's been a while since I looked at this code in detail. Bill is a much better bet here!

Comment 4

[Andrew McCreight [:mccr8]]

Sounds potentially bad, so I'm marking it sec-high. Feel free to adjust as appropriate.

Comment 5

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

I'm not totally convinced that this is actually a bug. We often create IPC objects on the main thread and then move them to the thread that they'll be used on. After that, all activity must be on that thread (including destruction). As long as get() and detach() are always called on the same thread, it seems like we're safe. Maybe I'm wrong though.

However, I'm not sure why we need a WeakPtr here. Justin said in his comment, "This allows us to keep the mListener reference around longer, whereas before we cleared it in AsyncChannel::Clear." That sort of makes sense (although I would expect mChannelState to be not connected, so Send would err out before examining mListener). But I don't see how mListener would go away before the MessageChannel. There's even a comment here:
http://searchfox.org/mozilla-central/rev/d9a04f71630ce4203ff0a5e26722723045d035b5/ipc/glue/MessageChannel.cpp#624-625

Anyway, I think it would be fine to convert this to a raw pointer. We could also consider nulling out mListener in Clear(), but that would probably be a bit more work.

Comment 6

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

Honza, do you want to convert to a raw pointer for your assertion checking patches?

Comment 7

[Andrew McCreight [:mccr8]]

I'll lower the rating, then.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Bill McCloskey (:billm) from comment #6)
> Honza, do you want to convert to a raw pointer for your assertion checking
> patches?

That is probably the way.  According the explanation, the channel's listener is ensured to live longer than the channel itself or can be easily notified about the listener's destruction - all is moved and further happens only on a single (background) thread.

Who will write the patch?  I can do that, but I don't know the code at all.

Comment 9

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

> Who will write the patch?  I can do that, but I don't know the code at all.

Do you mind writing it? If you run into any trouble, feel free to pass it to me.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Bill McCloskey (:billm) from comment #9)
> > Who will write the patch?  I can do that, but I don't know the code at all.
> 
> Do you mind writing it? If you run into any trouble, feel free to pass it to
> me.

OK, taking it.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Bill McCloskey (:billm) from comment #5)
> "This allows us to keep the mListener reference around longer,
> whereas before we cleared it in AsyncChannel::Clear." That sort of makes
> sense (although I would expect mChannelState to be not connected, so Send
> would err out before examining mListener). But I don't see how mListener
> would go away before the MessageChannel. There's even a comment here:
> http://searchfox.org/mozilla-central/rev/
> d9a04f71630ce4203ff0a5e26722723045d035b5/ipc/glue/MessageChannel.cpp#624-625

I've found this, but it's probably a leftover, explained below:

https://dxr.mozilla.org/mozilla-central/rev/763fe887c37cee5fcfe0f00e94fdffc84a41ea1c/ipc/glue/MessageChannel.cpp#260

        // mListener could have gone away if Close() was called while
        // MessageChannel code was still on the stack
        if (!mThat.mListener)
            return;

Introduced here https://hg.mozilla.org/mozilla-central/rev/dbfb36b8b381, bug 545455.  In times that bug has been added, mListener was declared in AsyncChannel (later merged) and was only a raw ptr.  WeakPtr has been introduced in https://hg.mozilla.org/mozilla-central/rev/c97f19dc7f7f, bug 857152 and this places might have been omitted from update.

Fingers crossed.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1

Roughly tested only, but the main idea is clear.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53d7f6bbd51b

Comment 13

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

Please don't make mListener Atomic. After initialization, it's always accessed on the channel's worker thread.

Comment 14

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

Also, that CxxStackFrame test is no longer useful since we no longer clear mListener in Clear(). So it can be removed.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Bill McCloskey (:billm) from comment #14)
> Also, that CxxStackFrame test is no longer useful since we no longer clear
> mListener in Clear(). So it can be removed.

I don't follow, I'm already removing it in the patch, or what do you mean?

Comment 16

[Honza Bambas (:mayhemer)]

Do you mean this hunk?

 void
 MessageChannel::ExitedCxxStack()
 {
-    mListener->OnExitedCxxStack();
+    if (mListener) {
+        mListener->OnExitedCxxStack();
+    }
     if (mSawInterruptOutMsg) {

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Comment 18

[Honza Bambas (:mayhemer)]

Daniel, can you please remove this bug from the " Security-Sensitive DOM Bug "?  I don't have the permission for that group.  

According the findings, this is not a security bug or anything exploitable.  Thanks.

Comment 19

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

(In reply to Honza Bambas (:mayhemer) from comment #15)
> (In reply to Bill McCloskey (:billm) from comment #14)
> > Also, that CxxStackFrame test is no longer useful since we no longer clear
> > mListener in Clear(). So it can be removed.
> 
> I don't follow, I'm already removing it in the patch, or what do you mean?

I was trying to answer the needinfo in comment 11. I meant the hunk that you already removed.

Comment 20

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

Comment on attachment 8779306 [details] [diff] [review]
v1.1

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

Thanks!

::: ipc/glue/MessageChannel.cpp
@@ +2079,5 @@
>  
>      // Oops, error!  Let the listener know about it.
>      mChannelState = ChannelError;
>  
> +    // After this, the channel may be deleted.  Based on the premiss that

premise

@@ +2081,5 @@
>      mChannelState = ChannelError;
>  
> +    // After this, the channel may be deleted.  Based on the premiss that
> +    // mListener owns this channel, any calls back to this class that may
> +    // work with mListner should still work on living objects.

mListener

Comment 21

[Honza Bambas (:mayhemer)]

Grr... needs an update first!

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v1.1.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfc312760e8c4c6103e05b80db1b192d64fe337b

(ignore Android 4.3 API15+ opt failures, that is a different patch)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71f49b7f44b6
turn mListener to a raw ptr, r=billm

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71f49b7f44b6