1256336 - Crash in mozilla::net::InterceptStreamListener::Cleanup

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The top of the stack is service worker related, but the Cleanup() method simply nulls two variables.  If its accessing 0xffffffff then I think the underlying object is garbage which suggests neck problem.

Note, there was a similar issue previously in bug 1089749.

Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0xffffffffffffffff

0 	xul.dll 	mozilla::net::InterceptStreamListener::Cleanup() 	netwerk/protocol/http/HttpChannelChild.cpp
1 	xul.dll 	mozilla::net::HttpChannelChild::DoNotifyListenerCleanup() 	netwerk/protocol/http/HttpChannelChild.cpp
2 	xul.dll 	mozilla::net::HttpBaseChannel::DoNotifyListener() 	netwerk/protocol/http/HttpBaseChannel.cpp
3 	xul.dll 	mozilla::net::HttpAsyncAborter<mozilla::net::HttpChannelChild>::HandleAsyncAbort() 	netwerk/protocol/http/HttpBaseChannel.h
4 	xul.dll 	mozilla::net::HttpChannelChild::RecvFailedAsyncOpen(nsresult const&) 	netwerk/protocol/http/HttpChannelChild.cpp
5 	xul.dll 	mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PHttpChannelChild.cpp
6 	xul.dll 	mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PContentChild.cpp
7 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
8 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
9 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
10 	xul.dll 	RunnableMethod<mozilla::ipc::MessageChannel, void ( mozilla::ipc::MessageChannel::*)(void), mozilla::Tuple<> >::Run() 	ipc/chromium/src/base/task.h
11 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
12 	xul.dll 	mozilla::ipc::DoWorkRunnable::Run() 	ipc/glue/MessagePump.cpp
13 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
14 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
15 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
16 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
17 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
18 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
19 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
20 	xul.dll 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp
21 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
22 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
23 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
24 	xul.dll 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp
25 	plugin-container.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
26 	plugin-container.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255

Comment 1

[Josh Matthews [:jdm]]

This is weird, because the InterceptStreamListener in HttpChannelChild is a strong reference.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #1)
> This is weird, because the InterceptStreamListener in HttpChannelChild is a
> strong reference.

Right.  That's why I think the entire HttpChannelChild is bogus.

Comment 3

[Patrick McManus [:mcmanus]]

more e10s eyes

Comment 4

[Mats Palmgren (inactive)]

Attachment: stack

I got this crash on shutdown in a local Linux64 debug trunk build.
It looks like the FailedAsyncOpenEvent::mChild raw pointer is stale.

Comment 5

[Josh Matthews [:jdm]]

Eep. I wonder why we chose to use raw pointers for those members :(

Comment 6

[Dragana Damjanovic [:dragana]]

I am taking this to try to figure out what is happening.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixing crash signature link, tracking since this is sec-critical and affects 47 and 48.

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Josh Matthews [:jdm] from comment #5)
> Eep. I wonder why we chose to use raw pointers for those members :(

It is fine to have raw pointers here. Calls are queues only if a divert to parents is in progress. At the end of a diversion the channelparent sends DeleteSelf, which deletes httpChannelChild, that is queued as well and called the last.

In the stack from comment #4 the channel is not in a diversion. AsyncOpen of a httpchannelchild is called which constructs a parent
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1946
The parent init fails which sends  SendFailedAsyncOpen.
At the moment when the RecvFailedAsyncOpen is called the httpchannelChild still exists it is not freed. A FailedAsyncOpenEvent is constructed and not queues just Run() is call which at some point calls DoNotifyListenerCleanup.
This function calls  PHttpChannelChild::Send__delete__(this) which frees httpChannelChild :) and then mInterceptListener->Cleanup() crashes :)

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1256336_v1.patch

Comment 10

[Dragana Damjanovic [:dragana]]

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

Comment 11

[Patrick McManus [:mcmanus]]

Thanks!

Comment 12

[Jason Duell]

Comment on attachment 8732809 [details] [diff] [review]
bug_1256336_v1.patch

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

Yes, Send_delete() is treacherous.  Thanks for the catch.

Comment 13

[Dragana Damjanovic [:dragana]]

Comment on attachment 8732809 [details] [diff] [review]
bug_1256336_v1.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If some check during channel asyncOpen fails (e.g. during shutdown it will fail, if host name cannot be parsed from url, in some cases if do not track blocks a uri) this code path will be triggered.
2 function calls are rearrange and one has a name Send_delete it is a bit obvious, IMO. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?
All. The change is in since ff38.

If not all supported branches, which bug introduced the flaw? 1065216

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply to all affected branches or it would be very easy to create without risk.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:
Since this problem was introduced in bug 1065216 it exists in all versions containing service workers.

Comment 16

[Patrick McManus [:mcmanus]]

it does exist in them all, but it also requires e10s right? So that might limit the uplifts (e.g. not to esr).

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:

Good point.  We probably don't need uplift to 45.  I think we are doing experiments with e10s in 46, though, right?

Also, I marked ESR45 disabled because service workers are disabled, there, but I think this bug can happen even in that case.  Right?

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46. It would probably be good to uplift this to beta 46 for the e10s experiments, as Ben points out.  
Service workers isn't enabled for 45esr so we likely don't need this fix for 45esr.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> Service workers isn't enabled for 45esr so we likely don't need this fix for
> 45esr.

Its true service workers are disabled in esr45, but I think this crash can trigger even on sites not using service workers when e10s is enabled.

Comment 20

[Dragana Damjanovic [:dragana]]

(In reply to Ben Kelly [:bkelly] from comment #19)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> > Service workers isn't enabled for 45esr so we likely don't need this fix for
> > 45esr.
> 
> Its true service workers are disabled in esr45, but I think this crash can
> trigger even on sites not using service workers when e10s is enabled.

This is not a service worker specific problem. If e10s is enable this can be triggered.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

e10s is also disabled at the code level on ESR45.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

sec-approval+. We'll want this on Aurora and Beta so please nominate it there as well.

Marking ESR45 as disabled based on comment 21.

Comment 23

[Dragana Damjanovic [:dragana]]

Comment on attachment 8732809 [details] [diff] [review]
bug_1256336_v1.patch

Approval Request Comment
[Feature/regressing bug #]:1065216
[User impact if declined]:crash
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: No risk, just changing order of 2 functions. In current order it will always crash(probably almost always on that code path), because the first function frees a mChild which is used in the next function.
[String/UUID change made/needed]:none

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8732809 [details] [diff] [review]
bug_1256336_v1.patch

May decrease e10s crashes, let's try uplifting this to beta 5 since
the e10s experiment is still running.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/364ea75abf26

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f567a7cba86

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/95bba9f0e660

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/364ea75abf26

Comment 29

[Mats Palmgren (inactive)]

I suspect this might not be fully fixed because I got a similar crash in bug 1262907.

Comment 30

[Dragana Damjanovic [:dragana]]

(In reply to Mats Palmgren (:mats) from comment #29)
> I suspect this might not be fully fixed because I got a similar crash in bug
> 1262907.

I cannot see bug 1262907. Can it be dup of 1261070? Or is a separate issue?

Comment 31

[Mats Palmgren (inactive)]

I don't have access to bug 1261070 so I can't tell.
The relevant part of the stack in bug 1262907 is:
...
#5  <signal handler called>
#6  nsCOMPtr<nsILoadGroup>::get (this=0xe5e5e5e5e5e5e705) at nsCOMPtr.h:721
#7  nsCOMPtr<nsILoadGroup>::operator nsILoadGroup* (this=0xe5e5e5e5e5e5e705) at nsCOMPtr.h:729
#8  mozilla::net::HttpAsyncAborter<mozilla::net::HttpChannelChild>::HandleAsyncAbort (this=0x7fce516794a8) at mozilla/net/HttpBaseChannel.h:578
#9  mozilla::net::HttpChannelChild::HandleAsyncAbort (this=0x7fce51679000) at netwerk/protocol/http/HttpChannelChild.cpp:1080
#10 mozilla::net::HttpChannelChild::FailedAsyncOpen (this=0x7fce51679000, status=@0x7fce4e8694f0: NS_ERROR_NOT_AVAILABLE) at netwerk/protocol/http/HttpChannelChild.cpp:1091
#11 mozilla::net::FailedAsyncOpenEvent::Run (this=0x7fce4e8694e0) at netwerk/protocol/http/HttpChannelChild.cpp:1060
...

Comment 32

[Jason Duell]

Dragana--re: comment 31.  Does that looks like a new bug, or still this one?

Comment 33

[Dragana Damjanovic [:dragana]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #32)
> Dragana--re: comment 31.  Does that looks like a new bug, or still this one?

This is resolved in bug 1261070.