1099414 - memory management issues in nICEr if e10s is used

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

Courtesy of external friends running some fuzzing for us:

==5504==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200005fc70 at pc 0x7f4a534b09a8 bp 0x7fffd838e6e0 sp 0x7fffd838e6d8
READ of size 8 at 0x61200005fc70 thread T0 (Web Content)
    #0 0x7f4a534b09a7 in nsCOMPtr_base::assign_with_AddRef(nsISupports*) objdir-ff-asan/dist/include/nsCOMPtr.h:467:7
    #1 0x7f4a577c64ec in mozilla::dom::UDPSocketChildBase::ReleaseIPDLReference() objdir-ff-asan/dist/include/nsCOMPtr.h:677:5
    #2 0x7f4a53ac15bc in mozilla::net::NeckoChild::DeallocPUDPSocketChild(mozilla::net::PUDPSocketChild*) netwerk/ipc/NeckoChild.cpp:251:3
    #3 0x7f4a54386537 in mozilla::net::PUDPSocketChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PUDPSocketChild.cpp:414:13
    #4 0x7f4a53f32bf6 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PContentChild.cpp:4518:16
    #5 0x7f4a53d017da in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp:1126:22
    #6 0x7f4a53cf7376 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ipc/glue/MessageChannel.cpp:1053:9
    #7 0x7f4a53cbb848 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) ipc/chromium/src/base/message_loop.cc:361:3
    #8 0x7f4a53cbc32c in MessageLoop::DoWork() ipc/chromium/src/base/message_loop.cc:447:13
    #9 0x7f4a53d085f2 in mozilla::ipc::DoWorkRunnable::Run() ipc/glue/MessagePump.cpp:233:3
    #10 0x7f4a53473106 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:830:7
    #11 0x7f4a534c9536 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #12 0x7f4a53d07d8e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:140:5
    #13 0x7f4a53cba761 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #14 0x7f4a57ecaa8f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:164:3
    #15 0x7f4a59933762 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:713:12
    #16 0x7f4a53cba761 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #17 0x7f4a59932c72 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:550:7
    #18 0x4ba94f in main ipc/contentproc/plugin-container.cpp:158:19
    #19 0x7f4a50a3dde4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260

0x61200005fc70 is located 176 bytes inside of 320-byte region [0x61200005fbc0,0x61200005fd00)
freed by thread T8 (Socket Thread) here:
    #0 0x498da1 in __interceptor_free _asan_rtl_
    #1 0x7f4a5494d13c in nr_socket_local_create media/mtransport/nr_socket_prsock.cpp:1139:5
    #2 0x7f4a5914fc42 in nr_ice_component_initialize media/mtransport/third_party/nICEr/src/ice/ice_component.c:198:12
    #3 0x7f4a59159d7a in nr_ice_media_stream_initialize media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:133:12
    #4 0x7f4a59158d52 in nr_ice_initialize media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:560:12
    #5 0x7f4a54957633 in mozilla::NrIceCtx::StartGathering() media/mtransport/nricectx.cpp:629:11
    #6 0x7f4a54888819 in mozilla::runnable_args_m_0<mozilla::RefPtr<mozilla::NrIceCtx>, tag_nsresult (mozilla::NrIceCtx::*)()>::Run() media/mtransport/runnable_utils_generated.h:48:7
    #7 0x7f4a53473106 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:830:7
    #8 0x7f4a534c9536 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #9 0x7f4a53659eea in nsSocketTransportService::Run() netwerk/base/src/nsSocketTransportService2.cpp:740:17
    #10 0x7f4a5365b99c in non-virtual thunk to nsSocketTransportService::Run() netwerk/base/src/nsSocketTransportService2.cpp:777:1
    #11 0x7f4a53473106 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:830:7
    #12 0x7f4a534c9536 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #13 0x7f4a53d08cfe in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:368:5
    #14 0x7f4a53cba761 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #15 0x7f4a5346feb6 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:350:5
    #16 0x7f4a5f34f150 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:212:5
    #17 0x7f4a5fb9bf6d in start_thread /build/buildd/eglibc-2.17/nptl/pthread_create.c:311

previously allocated by thread T8 (Socket Thread) here:
    #0 0x499079 in __interceptor_malloc _asan_rtl_
    #1 0x7f4a5f98cb1d in moz_xmalloc memory/mozalloc/mozalloc.cpp:52:17
    #2 0x7f4a5494cdca in nr_socket_local_create objdir-ff-asan/dist/include/mozilla/mozalloc.h:208:12
    #3 0x7f4a5914fc42 in nr_ice_component_initialize media/mtransport/third_party/nICEr/src/ice/ice_component.c:198:12
    #4 0x7f4a59159d7a in nr_ice_media_stream_initialize media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:133:12
    #5 0x7f4a59158d52 in nr_ice_initialize media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:560:12
    #6 0x7f4a54957633 in mozilla::NrIceCtx::StartGathering() media/mtransport/nricectx.cpp:629:11
    #7 0x7f4a54888819 in mozilla::runnable_args_m_0<mozilla::RefPtr<mozilla::NrIceCtx>, tag_nsresult (mozilla::NrIceCtx::*)()>::Run() media/mtransport/runnable_utils_generated.h:48:7
    #8 0x7f4a53473106 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:830:7
    #9 0x7f4a534c9536 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #10 0x7f4a53659eea in nsSocketTransportService::Run() netwerk/base/src/nsSocketTransportService2.cpp:740:17
    #11 0x7f4a5365b99c in non-virtual thunk to nsSocketTransportService::Run() netwerk/base/src/nsSocketTransportService2.cpp:777:1
    #12 0x7f4a53473106 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:830:7
    #13 0x7f4a534c9536 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #14 0x7f4a53d08cfe in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:368:5
    #15 0x7f4a53cba761 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #16 0x7f4a5346feb6 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:350:5
    #17 0x7f4a5f34f150 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:212:5
    #18 0x7f4a5fb9bf6d in start_thread /build/buildd/eglibc-2.17/nptl/pthread_create.c:311

Thread T8 (Socket Thread) created by T0 (Web Content) here:
    #0 0x4357be in pthread_create _asan_rtl_
    #1 0x7f4a5f34bcb0 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:453:14
    #2 0x7f4a5f34b8da in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:544:12
    #3 0x7f4a53471215 in nsThread::Init() xpcom/threads/nsThread.cpp:455:19
    #4 0x7f4a53476794 in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) xpcom/threads/nsThreadManager.cpp:269:17
    #5 0x7f4a534c8c7c in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) xpcom/glue/nsThreadUtils.cpp:68:5
    #6 0x7f4a53657dac in nsSocketTransportService::Init() netwerk/base/src/nsSocketTransportService2.cpp:468:19
    #7 0x7f4a53c61721 in nsSocketTransportServiceConstructor(nsISupports*, nsID const&, void**) netwerk/build/nsNetModule.cpp:72:1
    #8 0x7f4a534502d3 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1199:10
    #9 0x7f4a534476eb in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1560:10
    #10 0x7f4a534b1489 in nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) xpcom/glue/nsComponentManagerUtils.cpp:67:10
    #11 0x7f4a535ef654 in nsIOService::SetOffline(bool) objdir-ff-asan/dist/include/nsCOMPtr.h:744:5
    #12 0x7f4a535ee41f in nsIOService::InitializeNetworkLinkService() netwerk/base/src/nsIOService.cpp:290:9
    #13 0x7f4a535ed7d2 in nsIOService::Init() netwerk/base/src/nsIOService.cpp:226:5
    #14 0x7f4a535f01f0 in nsIOService::GetInstance() netwerk/base/src/nsIOService.cpp:303:23
    #15 0x7f4a53c61485 in nsIOServiceConstructor(nsISupports*, nsID const&, void**) netwerk/build/nsNetModule.cpp:57:1
    #16 0x7f4a534502d3 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1199:10
    #17 0x7f4a534476eb in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1560:10
    #18 0x7f4a54a3a196 in nsScriptSecurityManager::Init() objdir-ff-asan/dist/include/nsServiceManagerUtils.h:88:10
    #19 0x7f4a54a3ae2b in nsScriptSecurityManager::InitStatics() caps/nsScriptSecurityManager.cpp:1328:19
    #20 0x7f4a54573822 in nsXPConnect::InitStatics() js/xpconnect/src/nsXPConnect.cpp:132:5
    #21 0x7f4a545080c8 in xpcModuleCtor() js/xpconnect/src/XPCModule.cpp:13:5
    #22 0x7f4a58d255d4 in Initialize() layout/build/nsLayoutModule.cpp:395:8
    #23 0x7f4a5344efb1 in nsFactoryEntry::GetFactory() xpcom/components/nsComponentManager.cpp:858:21
    #24 0x7f4a53450263 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1196:34
    #25 0x7f4a534476eb in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1560:10
    #26 0x7f4a534b12af in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) xpcom/glue/nsComponentManagerUtils.cpp:67:10
    #27 0x7f4a534dc425 in NS_InitXPCOM2 objdir-ff-asan/dist/include/nsCOMPtr.h:950:5
    #28 0x7f4a599321dd in XRE_InitEmbedding2 toolkit/xre/nsEmbedFunctions.cpp:164:8
    #29 0x7f4a53d0abd5 in mozilla::ipc::ScopedXREEmbed::Start() ipc/glue/ScopedXREEmbed.cpp:104:10
    #30 0x7f4a579dfd0e in mozilla::dom::ContentProcess::Init() dom/ipc/ContentProcess.cpp:28:5
    #31 0x7f4a59932c64 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:537:12
    #32 0x4ba94f in main ipc/contentproc/plugin-container.cpp:158:19
    #33 0x7f4a50a3dde4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260

SUMMARY: AddressSanitizer: heap-use-after-free ??:0 ??
Shadow bytes around the buggy address:
  0x0c2480003f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2480003f40: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c2480003f50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2480003f60: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa
  0x0c2480003f70: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
=>0x0c2480003f80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd
  0x0c2480003f90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2480003fa0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c2480003fb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2480003fc0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa
  0x0c2480003fd0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  ASan internal:           fe
==5504==ABORTING

Comment 1

[Byron Campen [:bwc]]

My suspicion here is that |sock| has already been addref'ed when we delete it here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#1139

We should probably just use a smart pointer here.

Comment 2

[Eric Rescorla (:ekr)]

Do we know what they did to make this happen? Some notes:

1. This looks like it's in the IPC version. Do you know what platform?
2. My first though about what's happening was that we are failing somewhere in
the IPC create function:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#828

But after we have spawned off some task that uses the socket, which then comes back
and freaks out. I don't immediately see how that happens, so the next step is probably
to study this function.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: fuzz-http-4.html

This is the test case they ran on Linux 64bit ASAN. I'm not sure which exact version they ran, or if they used E10S or not. I'll ask.

Comment 4

[Byron Campen [:bwc]]

Attachment: Use Release() instead of delete when nr_socket_local_create fails.

Comment 5

[Nils Ohlmeier [:drno]]

It seems that I can execute the test case on ASAN without E10S for long periods of time without any problems.
But with E10S turned on I got a tab crash after a few minutes. Although looked different then the one in this report.
So right now it looks to me like this test exposes multiple problems with E10S under ASAN.

Comment 6

[Byron Campen [:bwc]]

Now, there is likely another bug lurking here; if main is acquiring references to NrSocketIpc, that means we need to verify that these references on main are never the last ones standing, because that would result in NrSocketIpc being destroyed on the wrong thread.

Comment 7

[Eric Rescorla (:ekr)]

Byron, I don't have a problem with the patch you attached in c4
as a clarity improvement but
if sock->create() or nr_socket_create_int() fails, then the socket
is no longer valid and the other code shouldn't be holding onto a
reference. That's the code that needs to be fixed.

Comment 8

[Nils Ohlmeier [:drno]]

Here is the same problem, but accesing the freed memory from a different area this time:

==31281==ERROR: AddressSanitizer: heap-use-after-free on address 0x6130000a04b0 at pc 0x7f35c3bd1ea6 bp 0x7fff56dfa070 sp 0x7fff56dfa068
READ of size 8 at 0x6130000a04b0 thread T0 (Web Content)
--DOCSHELL 0x6190004a6f80 == 4 [pid = 31216] [id = 3]
    #0 0x7f35c3bd1ea5 in NS_LogCOMPtrRelease /home/nohlmeier/src/mozilla-central-asan/xpcom/base/nsTraceRefcnt.cpp:1327
    #1 0x7f35c73dda35 in nsCOMPtr<nsIUDPSocketInternal>::assign_assuming_AddRef(nsIUDPSocketInternal*) /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dom/network/../../dist/include/nsCOMPtr.h:504
    #2 0x7f35c73dc0ed in nsCOMPtr<nsIUDPSocketInternal>::operator=(nsIUDPSocketInternal*) /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dom/network/../../dist/include/nsCOMPtr.h:677
    #3 0x7f35c73d596e in mozilla::dom::UDPSocketChildBase::ReleaseIPDLReference() /home/nohlmeier/src/mozilla-central-asan/dom/network/UDPSocketChild.cpp:31
    #4 0x7f35c41e234c in mozilla::net::NeckoChild::DeallocPUDPSocketChild(mozilla::net::PUDPSocketChild*) /home/nohlmeier/src/mozilla-central-asan/netwerk/ipc/NeckoChild.cpp:251
    #5 0x7f35c491e07a in mozilla::net::PNeckoChild::DeallocSubtree() /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/ipc/ipdl/./PNeckoChild.cpp:1902
    #6 0x7f35c46cc3b6 in mozilla::dom::PContentChild::DeallocSubtree() /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/ipc/ipdl/./PContentChild.cpp:6714
    #7 0x7f35c46ca2da in mozilla::dom::PContentChild::OnChannelClose() /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/ipc/ipdl/./PContentChild.cpp:6227
    #8 0x7f35c44a85de in mozilla::ipc::MessageChannel::NotifyChannelClosed() /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessageChannel.cpp:1684
    #9 0x7f35c44a8791 in mozilla::ipc::MessageChannel::OnNotifyMaybeChannelError() /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessageChannel.cpp:1566
    #10 0x7f35c4426fe1 in MessageLoop::RunTask(Task*) /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:361
    #11 0x7f35c44278ef in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:369
    #12 0x7f35c4427c46 in MessageLoop::DoWork() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:447
    #13 0x7f35c44abae7 in mozilla::ipc::DoWorkRunnable::Run() /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:233
    #14 0x7f35c3c7b879 in nsThread::ProcessNextEvent(bool, bool*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:830
    #15 0x7f35c3cd58be in NS_ProcessNextEvent(nsIThread*, bool) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:265
    #16 0x7f35c44ab219 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:140
    #17 0x7f35c44abf10 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:302
    #18 0x7f35c4426d41 in MessageLoop::RunInternal() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:233
    #19 0x7f35c4426a98 in MessageLoop::Run() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:200
    #20 0x7f35c793d9c6 in nsBaseAppShell::Run() /home/nohlmeier/src/mozilla-central-asan/widget/nsBaseAppShell.cpp:164
    #21 0x7f35c8d44e26 in XRE_RunAppShell /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsEmbedFunctions.cpp:713
    #22 0x7f35c44abdb3 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:272
    #23 0x7f35c4426d41 in MessageLoop::RunInternal() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:233
    #24 0x7f35c4426a98 in MessageLoop::Run() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:200
    #25 0x7f35c8d4462b in XRE_InitChildProcess /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsEmbedFunctions.cpp:550
    #26 0x4b842f in content_process_main(int, char**) /home/nohlmeier/src/mozilla-central-asan/ipc/app/../contentproc/plugin-container.cpp:158
    #27 0x7f35c0275ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #28 0x4b832a in _start (/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container+0x4b832a)

0x6130000a04b0 is located 176 bytes inside of 352-byte region [0x6130000a0400,0x6130000a0560)
freed by thread T17 (Socket Thread) here:
    #0 0x4999a1 in free /home/nohlmeier/checkouts/llvm-20140708/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:69
    #1 0x7f35c50a241c in nr_socket_local_create /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nr_socket_prsock.cpp:1139
    #2 0x7f35c874ba6c in nr_ice_component_initialize_udp /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_component.c:198
    #3 0x7f35c874b3ad in nr_ice_component_initialize /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_component.c:404
    #4 0x7f35c875693a in nr_ice_media_stream_initialize /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:133
    #5 0x7f35c8755694 in nr_ice_initialize /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:560
    #6 0x7f35c50abb91 in mozilla::NrIceCtx::StartGathering() /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nricectx.cpp:629
    #7 0x7f35c4dec234 in mozilla::runnable_args_m_0<mozilla::RefPtr<mozilla::NrIceCtx>, tag_nsresult (mozilla::NrIceCtx::*)()>::Run() /home/nohlmeier/src/mozilla-central-asan/media/webrtc/signaling/../../../media/mtransport/runnable_utils_generated.h:48
    #8 0x7f35c3c7b879 in nsThread::ProcessNextEvent(bool, bool*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:830
    #9 0x7f35c3cd58be in NS_ProcessNextEvent(nsIThread*, bool) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:265
    #10 0x7f35c3e3ba30 in nsSocketTransportService::Run() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsSocketTransportService2.cpp:740
    #11 0x7f35c3e3cddc in non-virtual thunk to nsSocketTransportService::Run() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsSocketTransportService2.cpp:777
    #12 0x7f35c3c7b879 in nsThread::ProcessNextEvent(bool, bool*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:830
    #13 0x7f35c3cd58be in NS_ProcessNextEvent(nsIThread*, bool) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:265
    #14 0x7f35c44ac2b8 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:339
    #15 0x7f35c4426d41 in MessageLoop::RunInternal() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:233
    #16 0x7f35c4426a98 in MessageLoop::Run() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:200
    #17 0x7f35c3c792de in nsThread::ThreadFunc(void*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:350
    #18 0x7f35cf51b863 in _pt_root /home/nohlmeier/src/mozilla-central-asan/nsprpub/pr/src/pthreads/ptthread.c:212
    #19 0x7f35cfb6d181 in start_thread /build/buildd/eglibc-2.19/nptl/pthread_create.c:312

previously allocated by thread T17 (Socket Thread) here:
    #0 0x499c79 in malloc /home/nohlmeier/checkouts/llvm-20140708/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:79
    #1 0x7f35cff7ed6d in moz_xmalloc /home/nohlmeier/src/mozilla-central-asan/memory/mozalloc/mozalloc.cpp:52
    #2 0x7f35c50a2311 in operator new(unsigned long) /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/media/mtransport/build/../../../dist/include/mozilla/mozalloc.h:208
    #3 0x7f35c50a2311 in nr_socket_local_create /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nr_socket_prsock.cpp:1118
    #4 0x7f35c874ba6c in nr_ice_component_initialize_udp /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_component.c:198
    #5 0x7f35c874b3ad in nr_ice_component_initialize /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_component.c:404
    #6 0x7f35c875693a in nr_ice_media_stream_initialize /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:133
    #7 0x7f35c8755694 in nr_ice_initialize /home/nohlmeier/src/mozilla-central-asan/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:560
    #8 0x7f35c50abb91 in mozilla::NrIceCtx::StartGathering() /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nricectx.cpp:629
    #9 0x7f35c4dec234 in mozilla::runnable_args_m_0<mozilla::RefPtr<mozilla::NrIceCtx>, tag_nsresult (mozilla::NrIceCtx::*)()>::Run() /home/nohlmeier/src/mozilla-central-asan/media/webrtc/signaling/../../../media/mtransport/runnable_utils_generated.h:48
    #10 0x7f35c3c7b879 in nsThread::ProcessNextEvent(bool, bool*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:830
    #11 0x7f35c3cd58be in NS_ProcessNextEvent(nsIThread*, bool) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:265
    #12 0x7f35c3e3ba30 in nsSocketTransportService::Run() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsSocketTransportService2.cpp:740
    #13 0x7f35c3e3cddc in non-virtual thunk to nsSocketTransportService::Run() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsSocketTransportService2.cpp:777
    #14 0x7f35c3c7b879 in nsThread::ProcessNextEvent(bool, bool*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:830
    #15 0x7f35c3cd58be in NS_ProcessNextEvent(nsIThread*, bool) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:265
    #16 0x7f35c44ac2b8 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:339
    #17 0x7f35c4426d41 in MessageLoop::RunInternal() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:233
    #18 0x7f35c4426a98 in MessageLoop::Run() /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:200
    #19 0x7f35c3c792de in nsThread::ThreadFunc(void*) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:350
    #20 0x7f35cf51b863 in _pt_root /home/nohlmeier/src/mozilla-central-asan/nsprpub/pr/src/pthreads/ptthread.c:212
    #21 0x7f35cfb6d181 in start_thread /build/buildd/eglibc-2.19/nptl/pthread_create.c:312

Thread T17 (Socket Thread) created by T0 (Web Content) here:
    #0 0x43941e in __interceptor_pthread_create /home/nohlmeier/checkouts/llvm-20140708/projects/compiler-rt/lib/asan/asan_interceptors.cc:180
    #1 0x7f35cf517be9 in _PR_CreateThread /home/nohlmeier/src/mozilla-central-asan/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7f35cf5176fa in PR_CreateThread /home/nohlmeier/src/mozilla-central-asan/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7f35c3c79fdf in nsThread::Init() /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:455
    #4 0x7f35c3c7db94 in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThreadManager.cpp:269
    #5 0x7f35c3cd50ef in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:68
    #6 0x7f35c3e39d5d in nsSocketTransportService::Init() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsSocketTransportService2.cpp:468
    #7 0x7f35c436d0a1 in nsSocketTransportServiceConstructor(nsISupports*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/netwerk/build/nsNetModule.cpp:72
    #8 0x7f35c3c59689 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1199
    #9 0x7f35c3c54050 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1560
    #10 0x7f35c3cc314e in nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsComponentManagerUtils.cpp:292
    #11 0x7f35c3e036a6 in nsCOMPtr<nsPISocketTransportService>::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/netwerk/base/src/../../../dist/include/nsCOMPtr.h:1228
    #12 0x7f35c3dfbc14 in nsCOMPtr<nsPISocketTransportService>::operator=(nsGetServiceByContractIDWithError const&) /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/netwerk/base/src/../../../dist/include/nsCOMPtr.h:744
    #13 0x7f35c3de126d in nsIOService::InitializeSocketTransportService() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsIOService.cpp:243
    #14 0x7f35c3de2054 in nsIOService::SetOffline(bool) /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsIOService.cpp:817
    #15 0x7f35c3de0f2c in nsIOService::InitializeNetworkLinkService() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsIOService.cpp:290
    #16 0x7f35c3de03e7 in nsIOService::Init() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsIOService.cpp:226
    #17 0x7f35c3de2666 in nsIOService::GetInstance() /home/nohlmeier/src/mozilla-central-asan/netwerk/base/src/nsIOService.cpp:303
    #18 0x7f35c436cf03 in nsIOServiceConstructor(nsISupports*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/netwerk/build/nsNetModule.cpp:57
    #19 0x7f35c3c59689 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1199
    #20 0x7f35c3c54050 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1560
    #21 0x7f35c515d7cd in nsScriptSecurityManager::Init() /home/nohlmeier/src/mozilla-central-asan/caps/nsScriptSecurityManager.cpp:1257:19
    #22 0x7f35c515dec2 in nsScriptSecurityManager::InitStatics() /home/nohlmeier/src/mozilla-central-asan/caps/nsScriptSecurityManager.cpp:1328
    #23 0x7f35c4c80de9 in nsXPConnect::InitStatics() /home/nohlmeier/src/mozilla-central-asan/js/xpconnect/src/nsXPConnect.cpp:132
    #24 0x7f35c4c1eb68 in xpcModuleCtor() /home/nohlmeier/src/mozilla-central-asan/js/xpconnect/src/XPCModule.cpp:13
    #25 0x7f35c831f584 in Initialize() /home/nohlmeier/src/mozilla-central-asan/layout/build/nsLayoutModule.cpp:395
    #26 0x7f35c3c57c4b in nsComponentManagerImpl::KnownModule::Load() /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:858
    #27 0x7f35c3c5887e in nsFactoryEntry::GetFactory() /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1915
    #28 0x7f35c3c59613 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1196
    #29 0x7f35c3c54050 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /home/nohlmeier/src/mozilla-central-asan/xpcom/components/nsComponentManager.cpp:1560
    #30 0x7f35c3cc2fb4 in nsGetServiceByContractID::operator()(nsID const&, void**) const /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsComponentManagerUtils.cpp:280
    #31 0x7f35c3cc2ed9 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsCOMPtr.cpp:103
    #32 0x7f35c3ce6e3f in NS_InitXPCOM2 /home/nohlmeier/src/mozilla-central-asan/xpcom/build/XPCOMInit.cpp:706
    #33 0x7f35c8d43a63 in XRE_InitEmbedding2 /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsEmbedFunctions.cpp:164
    #34 0x7f35c44ad786 in mozilla::ipc::ScopedXREEmbed::Start() /home/nohlmeier/src/mozilla-central-asan/ipc/glue/ScopedXREEmbed.cpp:104
    #35 0x7f35c759ae99 in mozilla::dom::ContentProcess::Init() /home/nohlmeier/src/mozilla-central-asan/dom/ipc/ContentProcess.cpp:28
    #36 0x7f35c8d4461d in XRE_InitChildProcess /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsEmbedFunctions.cpp:537
    #37 0x4b842f in content_process_main(int, char**) /home/nohlmeier/src/mozilla-central-asan/ipc/app/../contentproc/plugin-container.cpp:158
    #38 0x7f35c0275ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

SUMMARY: AddressSanitizer: heap-use-after-free /home/nohlmeier/src/mozilla-central-asan/xpcom/base/nsTraceRefcnt.cpp:1327 NS_LogCOMPtrRelease
Shadow bytes around the buggy address:
  0x0c268000c040: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c268000c050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c268000c060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c268000c070: 00 00 00 00 fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c268000c080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c268000c090: fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd
  0x0c268000c0a0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c268000c0b0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c268000c0c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c268000c0d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c268000c0e0: 00 00 00 00 fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  ASan internal:           fe
==31281==ABORTING

Comment 9

[Nils Ohlmeier [:drno]]

Here is an assert which I have hit twice now while executing different fuzz test cases:

Assertion failure: state_ == NR_CONNECTED || state_ == NR_CLOSING, at /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nr_socket_prsock.cpp:821
#01: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3e59fd0]
#02: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x6191099]
#03: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3890dee]
#04: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x347ae6f]
#05: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x32613a4]
#06: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x325bd07]
#07: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0fe2]
#08: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e18f0]
#09: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e1c47]
#10: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3265ae8]
#11: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x2a3587a]
#12: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x2a8f8bf]
#13: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x326523b]
#14: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3265f11]
#15: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0d42]
#16: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0a99]
#17: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x66f79c7]
#18: XRE_RunAppShell[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x7afee27]
#19: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3265db4]
#20: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0d42]
#21: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0a99]
#22: XRE_InitChildProcess[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x7afe62c]
#23: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container +0xb8430]
#24: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x21ec5]
#25: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container +0xb832b]
#26: ??? (???:???)

Program /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container (pid = 7097) received signal 11.
Stack:
#01: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x10340]
#02: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3e59f9a]

###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv

#03: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3e59fd0]
#04: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x6191099]
#05: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3890dee]
#06: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x347ae6f]
#07: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x32613a4]
#08: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x325bd07]
#09: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0fe2]
#10: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e18f0]
#11: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e1c47]
#12: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3265ae8]
#13: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x2a3587a]
#14: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x2a8f8bf]
#15: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x326523b]
#16: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3265f11]
#17: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0d42]
#18: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0a99]
#19: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x66f79c7]
#20: XRE_RunAppShell[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x7afee27]
#21: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x3265db4]
#22: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0d42]
#23: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x31e0a99]
#24: XRE_InitChildProcess[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/libxul.so +0x7afe62c]
#25: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container +0xb8430]
#26: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x21ec5]
#27: ???[/home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container +0xb832b]
#28: ??? (???:???)
Sleeping for 300 seconds.
Type 'gdb /home/nohlmeier/src/mozilla-central-asan/objdir-ff-asan/dist/bin/plugin-container 7097' to attach your debugger to this thread.

(gdb) bt
#0  0x00007f2c397429bd in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f2c39742854 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007f2c42179f3f in ah_crap_handler (signum=11)
    at /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsSigHandlers.cpp:101
#3  0x00007f2c4217a17f in child_ah_crap_handler (signum=11)
    at /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsSigHandlers.cpp:113
#4  <signal handler called>
#5  mozilla::NrSocketIpc::CallListenerClosed (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nr_socket_prsock.cpp:817
#6  0x00007f2c3e4ccfd0 in non-virtual thunk to mozilla::NrSocketIpc::CallListenerClosed() ()
    at /home/nohlmeier/src/mozilla-central-asan/media/mtransport/nr_socket_prsock.cpp:825
#7  0x00007f2c40804099 in mozilla::dom::UDPSocketChild::RecvCallbackClosed (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/dom/network/UDPSocketChild.cpp:230
#8  0x00007f2c3df03dee in mozilla::net::PUDPSocketChild::OnMessageReceived (this=<optimized out>, __msg=...)
    at ./PUDPSocketChild.cpp:326
#9  0x00007f2c3daede6f in mozilla::dom::PContentChild::OnMessageReceived (this=<optimized out>, __msg=...)
    at ./PContentChild.cpp:4692
#10 0x00007f2c3d8d43a4 in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0x61a00001ed10, aMsg=...)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessageChannel.cpp:1126
#11 0x00007f2c3d8ced07 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (this=0x61a00001ed10)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessageChannel.cpp:1053
#12 0x00007f2c3d853fe2 in MessageLoop::RunTask (this=<optimized out>, task=0x6030003e99b0)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:361
#13 0x00007f2c3d8548f0 in MessageLoop::DeferOrRunPendingTask (this=0x7f2c3107c640, pending_task=...)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:369
#14 0x00007f2c3d854c47 in MessageLoop::DoWork (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:447
#15 0x00007f2c3d8d8ae8 in mozilla::ipc::DoWorkRunnable::Run (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:233
#16 0x00007f2c3d0a887a in nsThread::ProcessNextEvent (this=<optimized out>, aMayWait=<optimized out>, 
    aResult=<optimized out>) at /home/nohlmeier/src/mozilla-central-asan/xpcom/threads/nsThread.cpp:830
#17 0x00007f2c3d1028bf in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=false)
    at /home/nohlmeier/src/mozilla-central-asan/xpcom/glue/nsThreadUtils.cpp:265
#18 0x00007f2c3d8d823b in mozilla::ipc::MessagePump::Run (this=<optimized out>, aDelegate=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:99
#19 0x00007f2c3d8d8f11 in mozilla::ipc::MessagePumpForChildProcess::Run (this=<optimized out>, 
    aDelegate=<optimized out>) at /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:302
#20 0x00007f2c3d853d42 in MessageLoop::RunInternal (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:233
#21 0x00007f2c3d853a99 in MessageLoop::Run (this=0x7fff8f1ed6e0)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:200
#22 0x00007f2c40d6a9c7 in nsBaseAppShell::Run (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/widget/nsBaseAppShell.cpp:164
#23 0x00007f2c42171e27 in XRE_RunAppShell ()
    at /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsEmbedFunctions.cpp:713
#24 0x00007f2c3d8d8db4 in mozilla::ipc::MessagePumpForChildProcess::Run (this=<optimized out>, 
    aDelegate=<optimized out>) at /home/nohlmeier/src/mozilla-central-asan/ipc/glue/MessagePump.cpp:272
#25 0x00007f2c3d853d42 in MessageLoop::RunInternal (this=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:233
#26 0x00007f2c3d853a99 in MessageLoop::Run (this=0x7fff8f1ed6e0)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/chromium/src/base/message_loop.cc:200
#27 0x00007f2c4217162c in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>)
    at /home/nohlmeier/src/mozilla-central-asan/toolkit/xre/nsEmbedFunctions.cpp:550
---Type <return> to continue, or q <return> to quit---
#28 0x00000000004b8430 in content_process_main (argc=4, argv=0x7fff8f1eda88)
    at /home/nohlmeier/src/mozilla-central-asan/ipc/app/../contentproc/plugin-container.cpp:158
#29 0x00007f2c396a2ec5 in __libc_start_main (main=0x4b84a0 <main(int, char**)>, argc=5, argv=0x7fff8f1eda88, 
    init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fff8f1eda78)
    at libc-start.c:287
#30 0x00000000004b832b in _start ()

Comment 10

[Nils Ohlmeier [:drno]]

Shih-Chiang as described in c#9 the fuzz test regularly hits this assertion:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#821

I think that assertion might be too agressive. I'm assuming that we get this callback only if we request closing of the socket. If that is the case I think we should allow closing of the socket in any given state, maybe with the execption that closing a socket which is already in the closed state should not happen.
Shih-Chiang can we relax that assert, or are really bad things going to happen if we do that?

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

This assert means UDPSocket in Parent side is either closed by child or closed by Parent while their is one before. Do we hit this assert because we are trying to send unexpected IPC message in fuzz? My understanding is that MOZ_ASSERT is for guaranteeing the program state in debug build, which means normal procedure can never break this assert hence make sure we doesn't write erroneous code. And fuzz test is all about introducing abnormal procedure in runtime. Why do we run fuzz test on debug build instead of release build?

Comment 12

[Eric Rescorla (:ekr)]

My understanding is that we are fuzzing from JS, so this should never be hit

Comment 13

[Nils Ohlmeier [:drno]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #11)
> This assert means UDPSocket in Parent side is either closed by child or
> closed by Parent while their is one before. Do we hit this assert because we
> are trying to send unexpected IPC message in fuzz?

As the attached test page shows this fuzz test is only playing around with timers in JS land. So we are not fuzzing the IPC channel at all here.
In other words this test does not introduce any IPC messages which are not generated by our code.

> My understanding is that
> MOZ_ASSERT is for guaranteeing the program state in debug build, which means
> normal procedure can never break this assert hence make sure we doesn't
> write erroneous code. And fuzz test is all about introducing abnormal
> procedure in runtime.

Fuzz tests are not only about introducing abnormal things. As this test tried lots of different timers it probably closes connections before they got established. Which is a totally valid use case. Not very normal or usual, but valid.
So far the assert has kicked in when the state_ is 1, so still connecting and not connected yet. But I think we should be able/allowed to closed a socket before it connected.

> Why do we run fuzz test on debug build instead of
> release build?

I'm actually running this on an ASAN build. Because only that way I can tell in a reasonable time what is going wrong here, which would take hours on a release build.

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Nils Ohlmeier [:drno] from comment #13)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #11)
> > This assert means UDPSocket in Parent side is either closed by child or
> > closed by Parent while their is one before. Do we hit this assert because we
> > are trying to send unexpected IPC message in fuzz?
> 
> As the attached test page shows this fuzz test is only playing around with
> timers in JS land. So we are not fuzzing the IPC channel at all here.
> In other words this test does not introduce any IPC messages which are not
> generated by our code.
> 
> > My understanding is that
> > MOZ_ASSERT is for guaranteeing the program state in debug build, which means
> > normal procedure can never break this assert hence make sure we doesn't
> > write erroneous code. And fuzz test is all about introducing abnormal
> > procedure in runtime.
> 
> Fuzz tests are not only about introducing abnormal things. As this test
> tried lots of different timers it probably closes connections before they
> got established. Which is a totally valid use case. Not very normal or
> usual, but valid.
> So far the assert has kicked in when the state_ is 1, so still connecting
> and not connected yet. But I think we should be able/allowed to closed a
> socket before it connected.
> 
> > Why do we run fuzz test on debug build instead of
> > release build?
> 
> I'm actually running this on an ASAN build. Because only that way I can tell
> in a reasonable time what is going wrong here, which would take hours on a
> release build.

Ok, got it.
In my code the state_ is always switching to NR_CLOSING while someone invoke socket.close(), so it shouldn't hit this assertion. The only scenario I can think of is that the socket in parent side is closed right after socket is opened. Therefore, the close callback might be executed before we change the state_ from NR_CONNECTING to NR_CONNECTED [1]. In this case, I agree that we can remove this assertion and add state protection in NrSocketIpc::create().

[1] http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#878

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: allow-close-in-all-state.patch

If we hit MOZ_ASSERT in line 877 after applying this patch, it can prove the socket creation is interrupted by socket close. @drno how can I verify this patch?

Comment 16

[Nils Ohlmeier [:drno]]

I ran the test for a long period of time without the assertion here:
http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#821
at it works fine.

I also ran the same test with your patch, but then your new assertion kicks in because the state has changed NR_CLOSED already.
I'm going to wrap the state changes in setState() function to try to figure out who is changing the state. Although the solution might simply be to only set state_ to NR_CONNECTED if it is still in the NR_CONNECTING state (and return an error if the state has changed to CLOSING or CLOSED.

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks @drno! The solution I have in mind is make NrSocketIpc::create() also return error as if creation failed [1].

[1] http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#874

Comment 18

[Byron Campen [:bwc]]

So, is the contract of nsIUDPSocketInternal intended to permit invocation of the d'tor on threads other than STS?

Comment 19

[Nils Ohlmeier [:drno]]

So changing the create() function to this:

mon.Wait()

if (state_ == NR_CONNECTING) {
  state_ = NR_CONNECTED;
} else {
  err_ = true;
}

if (err_) {
  ABORT(R_INTERNAL);
}

seems to work, except that the test run then pretty quickly runs into bug 1100686. So I think we need to fix 1100686 first.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Byron Campen [:bwc] from comment #18)
> So, is the contract of nsIUDPSocketInternal intended to permit invocation of
> the d'tor on threads other than STS?

Yes, it should be. NrSocketIpc object lives on both STS thread and main thread; and the real socket might be closed from content process (STS) or chrome process (Main). The listener strong reference will be broke during socket closing, which means dtor might be invoked if ref count is down to 0.

Comment 21

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Nils Ohlmeier [:drno] from comment #19)
> So changing the create() function to this:
> 
> mon.Wait()
> 
> if (state_ == NR_CONNECTING) {
>   state_ = NR_CONNECTED;
> } else {
>   err_ = true;
> }
> 
> if (err_) {
>   ABORT(R_INTERNAL);
> }
> 
> seems to work, except that the test run then pretty quickly runs into bug
> 1100686. So I think we need to fix 1100686 first.

The code should be:

mon.Wait()

if (err_ || state_ != CONNECTING) {
  ABORT(R_INTERNAL);
}

state_ = NR_CONNECTED;

Comment 22

[Andrew McCreight [:mccr8]]

(In reply to Byron Campen [:bwc] from comment #4)
> Created attachment 8523269 [details] [diff] [review]
> Use Release() instead of delete when nr_socket_local_create fails.

You should also make the dtor of NrSocketBase() protected so that this error would be caught at compile time.

Comment 23

[Andrew McCreight [:mccr8]]

Attachment: Have nr_socket_local_create use smart pointers.

In fact, I think a more C++-y way to write the patch in attachment 8523269 [details] [diff] [review] would be something like this.

We always addref socket immediately upon creation, thanks to the nsRefPtr.  Then, if nr_socket_create_int succeeds, we clear out our local reference to sock, signifying that it is now owned by the nr_socket.  If it fails, then we'll skip this, and it will just get naturally released when we return.

Comment 24

[Byron Campen [:bwc]]

Won't the already_AddRefed returned by |forget| assert when it is destroyed? I think you'd need to do something like (void)sock.forget().take(), which is a mouthful.

Comment 25

[Andrew McCreight [:mccr8]]

Ah, good point!  Yeah, that is kind of awful, but there's going to be some kind of headache or another when trying to refcount C code that doesn't know it is refcounting.  I guess it is arguable whether your patch or mine is better.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Bryon, Andrew: ping.  Can we review this patch and land it?  This sec-high has been sitting for a month now with a patch.

Comment 27

[Byron Campen [:bwc]]

So, the reason this isn't quite ready yet is I have not had time to determine whether NrSocketIpc is safe to destroy both on main and STS, since either could happen.

Comment 28

[Byron Campen [:bwc]]

Comment on attachment 8523269 [details] [diff] [review]
Use Release() instead of delete when nr_socket_local_create fails.

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

I've looked into it, and it appears that NrSocketIpc doesn't hold onto any resources that care which thread they are destroyed on, except an nsIUDPSocketChild that is wrapped in an nsMainThreadPtrHandle. So, it should be safe to destroy on either main or STS.

Comment 29

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #28)
> Comment on attachment 8523269 [details] [diff] [review]
> Use Release() instead of delete when nr_socket_local_create fails.
> 
> Review of attachment 8523269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've looked into it, and it appears that NrSocketIpc doesn't hold onto any
> resources that care which thread they are destroyed on, except an
> nsIUDPSocketChild that is wrapped in an nsMainThreadPtrHandle. So, it should
> be safe to destroy on either main or STS.

This doesn't seem like a good path to go down. There's a basic assumption in
the rest of the system that nICEr and NrSocket stuff is only destroyed on
STS. What's the argument against providing a patch that maintains this
invariant, other than expedience?

Comment 30

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #29)
> (In reply to Byron Campen [:bwc] from comment #28)
> > Comment on attachment 8523269 [details] [diff] [review]
> > Use Release() instead of delete when nr_socket_local_create fails.
> > 
> > Review of attachment 8523269 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I've looked into it, and it appears that NrSocketIpc doesn't hold onto any
> > resources that care which thread they are destroyed on, except an
> > nsIUDPSocketChild that is wrapped in an nsMainThreadPtrHandle. So, it should
> > be safe to destroy on either main or STS.
> 
> This doesn't seem like a good path to go down. There's a basic assumption in
> the rest of the system that nICEr and NrSocket stuff is only destroyed on
> STS. What's the argument against providing a patch that maintains this
> invariant, other than expedience?

   Because we have just as basic an assumption that IPC client stuff is expected to be destroyed on main.

Comment 31

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #30)
> (In reply to Eric Rescorla (:ekr) from comment #29)
> > (In reply to Byron Campen [:bwc] from comment #28)
> > > Comment on attachment 8523269 [details] [diff] [review]
> > > Use Release() instead of delete when nr_socket_local_create fails.
> > > 
> > > Review of attachment 8523269 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I've looked into it, and it appears that NrSocketIpc doesn't hold onto any
> > > resources that care which thread they are destroyed on, except an
> > > nsIUDPSocketChild that is wrapped in an nsMainThreadPtrHandle. So, it should
> > > be safe to destroy on either main or STS.
> > 
> > This doesn't seem like a good path to go down. There's a basic assumption in
> > the rest of the system that nICEr and NrSocket stuff is only destroyed on
> > STS. What's the argument against providing a patch that maintains this
> > invariant, other than expedience?
> 
>    Because we have just as basic an assumption that IPC client stuff is
> expected to be destroyed on main.

Then what's needed is a proxy and a thread dispatch.

Comment 32

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #31)
> (In reply to Byron Campen [:bwc] from comment #30)
> > (In reply to Eric Rescorla (:ekr) from comment #29)
> > > (In reply to Byron Campen [:bwc] from comment #28)
> > > > Comment on attachment 8523269 [details] [diff] [review]
> > > > Use Release() instead of delete when nr_socket_local_create fails.
> > > > 
> > > > Review of attachment 8523269 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > I've looked into it, and it appears that NrSocketIpc doesn't hold onto any
> > > > resources that care which thread they are destroyed on, except an
> > > > nsIUDPSocketChild that is wrapped in an nsMainThreadPtrHandle. So, it should
> > > > be safe to destroy on either main or STS.
> > > 
> > > This doesn't seem like a good path to go down. There's a basic assumption in
> > > the rest of the system that nICEr and NrSocket stuff is only destroyed on
> > > STS. What's the argument against providing a patch that maintains this
> > > invariant, other than expedience?
> > 
> >    Because we have just as basic an assumption that IPC client stuff is
> > expected to be destroyed on main.
> 
> Then what's needed is a proxy and a thread dispatch.

   We can do that, but it is extra code.

Comment 33

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #32)
> (In reply to Eric Rescorla (:ekr) from comment #31)
> > (In reply to Byron Campen [:bwc] from comment #30)
> > > (In reply to Eric Rescorla (:ekr) from comment #29)
> > > > (In reply to Byron Campen [:bwc] from comment #28)
> > > > > Comment on attachment 8523269 [details] [diff] [review]
> > > > > Use Release() instead of delete when nr_socket_local_create fails.
> > > > > 
> > > > > Review of attachment 8523269 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > I've looked into it, and it appears that NrSocketIpc doesn't hold onto any
> > > > > resources that care which thread they are destroyed on, except an
> > > > > nsIUDPSocketChild that is wrapped in an nsMainThreadPtrHandle. So, it should
> > > > > be safe to destroy on either main or STS.
> > > > 
> > > > This doesn't seem like a good path to go down. There's a basic assumption in
> > > > the rest of the system that nICEr and NrSocket stuff is only destroyed on
> > > > STS. What's the argument against providing a patch that maintains this
> > > > invariant, other than expedience?
> > > 
> > >    Because we have just as basic an assumption that IPC client stuff is
> > > expected to be destroyed on main.
> > 
> > Then what's needed is a proxy and a thread dispatch.
> 
>    We can do that, but it is extra code.

Understood, but I think it's the right thing.

Comment 34

[Byron Campen [:bwc]]

Attachment: Ensure that NrSocketIpc is destroyed on STS, for consistency

Ensure that NrSocketIpc is not destroyed on main when nsIUDPSocketInternal is destroyed.

Comment 35

[Byron Campen [:bwc]]

Comment on attachment 8540337 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

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

reviewboard:
https://reviewboard.mozilla.org/r/1651/

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3cf711cc6c02

non-unified try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=582bb70a0fca

Comment 36

[Eric Rescorla (:ekr)]

Comment on attachment 8540337 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

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

Can you walk me through why this patch fixes the initially reported
problem where something goes wrong in NrSocketIpc::create(), e.g.,
in Bind()?

::: media/mtransport/nr_socket_prsock.h
@@ +200,5 @@
>    };
>  
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrSocketIpc)
> +
> +  NS_IMETHODIMP CallListenerError(const nsACString &message,

I'm not immediately seeing why you can't use the macro here. Can you explain?

@@ +254,5 @@
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSIUDPSOCKETINTERNAL
> +
> +  nsresult Init(nsRefPtr<NrSocketIpc> socket);

Any reason this can't be a const reference?

@@ +258,5 @@
> +  nsresult Init(nsRefPtr<NrSocketIpc> socket);
> +
> +private:
> +  virtual ~NrSocketIpcProxy();
> +  nsRefPtr<NrSocketIpc> socket_;

Whitespace between dtor and member variables please.

::: media/mtransport/runnable_utils.h
@@ +95,5 @@
>  
> +template <class T>
> +class DispatchedRelease : public detail::runnable_args_base<detail::NoResult>
> +{
> +public:

This code has the opening brace on the same line as the class name.

@@ +97,5 @@
> +class DispatchedRelease : public detail::runnable_args_base<detail::NoResult>
> +{
> +public:
> +  DispatchedRelease(already_AddRefed<T>& ref) : ref_(ref) {}
> +  NS_IMETHOD Run() {

Whitespace before this method, please.

Comment 37

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #36)
> Comment on attachment 8540337 [details] [diff] [review]
> Ensure that NrSocketIpc is destroyed on STS, for consistency
> 
> Review of attachment 8540337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you walk me through why this patch fixes the initially reported
> problem where something goes wrong in NrSocketIpc::create(), e.g.,
> in Bind()?
> 
> ::: media/mtransport/nr_socket_prsock.h
> @@ +200,5 @@
> >    };
> >  
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrSocketIpc)
> > +
> > +  NS_IMETHODIMP CallListenerError(const nsACString &message,
> 
> I'm not immediately seeing why you can't use the macro here. Can you explain?
> 

   Because the macro tacks MOZ_OVERRIDE on the function decls, which would mean that we'd need to inherit the interface, which would perhaps invite misuse. I'm also not a huge fan of using macros to declare API unless necessary.

> @@ +254,5 @@
> > +public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> > +  NS_DECL_NSIUDPSOCKETINTERNAL
> > +
> > +  nsresult Init(nsRefPtr<NrSocketIpc> socket);
> 
> Any reason this can't be a const reference?
> 

   I don't think so.

Comment 38

[Byron Campen [:bwc]]

Attachment: Ensure that NrSocketIpc is destroyed on STS, for consistency

Interdiff at https://reviewboard.mozilla.org/r/1651/diff/1-2/

Comment 39

[Byron Campen [:bwc]]

Comment on attachment 8541015 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

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

Interdiff here: https://reviewboard.mozilla.org/r/1651/diff/1-2/

Comment 40

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #37)
> (In reply to Eric Rescorla (:ekr) from comment #36)
> > Comment on attachment 8540337 [details] [diff] [review]
> > Ensure that NrSocketIpc is destroyed on STS, for consistency
> > 
> > Review of attachment 8540337 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can you walk me through why this patch fixes the initially reported
> > problem where something goes wrong in NrSocketIpc::create(), e.g.,
> > in Bind()?

Can you walk me through this?


> > ::: media/mtransport/nr_socket_prsock.h
> > @@ +200,5 @@
> > >    };
> > >  
> > > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrSocketIpc)
> > > +
> > > +  NS_IMETHODIMP CallListenerError(const nsACString &message,
> > 
> > I'm not immediately seeing why you can't use the macro here. Can you explain?
> > 
> 
>    Because the macro tacks MOZ_OVERRIDE on the function decls, which would
> mean that we'd need to inherit the interface, which would perhaps invite
> misuse.

OK.

> I'm also not a huge fan of using macros to declare API unless
> necessary.


Yeah, I can understand that, though it does seem to be the convention.

 
> > @@ +254,5 @@
> > > +public:
> > > +  NS_DECL_THREADSAFE_ISUPPORTS
> > > +  NS_DECL_NSIUDPSOCKETINTERNAL
> > > +
> > > +  nsresult Init(nsRefPtr<NrSocketIpc> socket);
> > 
> > Any reason this can't be a const reference?
> > 
> 
>    I don't think so.

Comment 41

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #40)
> (In reply to Byron Campen [:bwc] from comment #37)
> > (In reply to Eric Rescorla (:ekr) from comment #36)
> > > Comment on attachment 8540337 [details] [diff] [review]
> > > Ensure that NrSocketIpc is destroyed on STS, for consistency
> > > 
> > > Review of attachment 8540337 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Can you walk me through why this patch fixes the initially reported
> > > problem where something goes wrong in NrSocketIpc::create(), e.g.,
> > > in Bind()?
> 
> Can you walk me through this?

   So, the way that the code off on main was getting a ref to the NrSocketIpc was via the socket_child_->Bind() call here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#1025

   With this patch, we instead pass |proxy|, which still holds onto a ref to the NrSocketIpc, but knows to release that ref on STS. The proxy holds no other resources besides that, so it doesn't really matter what thread it goes away on.

Comment 42

[Eric Rescorla (:ekr)]

Comment on attachment 8523269 [details] [diff] [review]
Use Release() instead of delete when nr_socket_local_create fails.

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

r+ but please file a bug to find out the source of the failure.

::: media/mtransport/nr_socket_prsock.cpp
@@ +1117,5 @@
>      NS_GetMainThread(getter_AddRefs(main_thread));
>      sock = new NrSocketIpc(main_thread.get());
>    }
>  
> +  // Add a reference so that we can delete it in destroy()

Please add a comment that this is not the ownership model
that is ordinarily assumed for this object.

@@ +1118,5 @@
>      sock = new NrSocketIpc(main_thread.get());
>    }
>  
> +  // Add a reference so that we can delete it in destroy()
> +  sock->AddRef();

Let's instead attach this to a RefPtr and then forget about
before line 1135. IIRC with RefPtr, it's .forget().drop().

Then you can remove the if clause in line 1138.

Comment 44

[Byron Campen [:bwc]]

Attachment: Use RefPtr logic instead of delete when nr_socket_local_create fails

Fix nits.

Comment 45

[Byron Campen [:bwc]]

Comment on attachment 8543351 [details] [diff] [review]
Use RefPtr logic instead of delete when nr_socket_local_create fails

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

Carry forward r=ekr.

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

Comment 46

[Byron Campen [:bwc]]

Check back on try.

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

You may want to recubmit the try; looks like it hit infra problems

Comment 48

[Byron Campen [:bwc]]

If at first you don't succeed...

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4aba1d323040

Comment 49

[Byron Campen [:bwc]]

Attachment: Ensure that NrSocketIpc is destroyed on STS, for consistency

Try to fix the static analysis build.

Comment 50

[Byron Campen [:bwc]]

Comment on attachment 8544661 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

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

Carry forward r=ekr

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=552000692dd7

Comment 51

[Byron Campen [:bwc]]

Comment on attachment 8544661 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

Approval Request Comment
[Feature/regressing bug #]:

   bug 869869

[User impact if declined]:

   See below.

[Describe test coverage new/current, TBPL]:

   The basic use of NrSocketIpc is covered by mochitests.

[Risks and why]: 
   See below.

[String/UUID change made/needed]:

   None.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   The bug was hit with a fuzzer, and it is a UAF (double delete) on an object with a vtable, so it seems pretty exploitable to me as an arbitrary code execution. This is only in the child process, FWIW.

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

   Not exactly; careful inspection of the patch would reveal the underlying bug being fixed, but would not reveal how to exploit very easily (in fact, we are still not sure what specific error condition led to the crash).

Which older supported branches are affected by this flaw?

   This goes back to 28, so all supported branches.

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

   Bug 869869

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

   The backports should be easy.

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

   Not too likely, I think.

Comment 52

[Byron Campen [:bwc]]

Did we want to try for a release or esr approval for this?

Comment 53

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

It is far too late for this to make 35. We have release candidates building and no more betas. 

This is getting sec-approval+ for trunk but needs to have checkin held until 1/26, which is two weeks into the next release cycle.

Comment 54

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

Comment on attachment 8544661 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

sec-approval+ for checkin on 1/26.

Since this involves e10s, does it *really* affect ESR31? Can this be triggered without e10s?

Comment 55

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

[Tracking Requested - why for this release]:

Comment 56

[Byron Campen [:bwc]]

(In reply to Al Billings [:abillings] from comment #54)
> Comment on attachment 8544661 [details] [diff] [review]
> Ensure that NrSocketIpc is destroyed on STS, for consistency
> 
> sec-approval+ for checkin on 1/26.
> 
> Since this involves e10s, does it *really* affect ESR31? Can this be
> triggered without e10s?

   No. You would have to pref e10s on.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8544661 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

Doesn't need extra approval for B2G.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeac5917dda7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd74efe0b44a

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f313202b1e25

https://treeherder.mozilla.org/logviewer.html#?job_id=5867490&repo=mozilla-inbound

Comment 60

[Byron Campen [:bwc]]

Looks like the API moved since the last try push. Can fix up on Monday.

Comment 61

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8544661 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

Based on comment 56 I understand that there is no need to take this fix in Beta 36 or Aurora 37 (e10s is preffed off in both). I am flagging for b2g approval again as this fix may very well be needed on that platform.

Comment 62

[Byron Campen [:bwc]]

Have a new patch, push to try since it has been a while.

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

Comment 63

[Byron Campen [:bwc]]

Ok, trying a little harder to force the ref to be dropped without getting compiler warnings:

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

Comment 64

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9028edd5e752
https://hg.mozilla.org/integration/mozilla-inbound/rev/d67a6612282c

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8544661 [details] [diff] [review]
Ensure that NrSocketIpc is destroyed on STS, for consistency

B@G has auto-approval for affected branches since it's sec-high

Comment 66

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

https://hg.mozilla.org/mozilla-central/rev/9028edd5e752
https://hg.mozilla.org/mozilla-central/rev/d67a6612282c

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d3404125da64
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af0fdf4046d9

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/5b7def1d1b1b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9677031f90e2

This looks non-trivial to backport to b2g32/b2g30. Is it worth even trying, Byron?

Comment 68

[Byron Campen [:bwc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #67)
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d3404125da64
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af0fdf4046d9
> 
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/5b7def1d1b1b
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9677031f90e2
> 
> This looks non-trivial to backport to b2g32/b2g30. Is it worth even trying,
> Byron?

I think that we can at least prevent this particular UAF by backporting the "Use RefPtr logic" patch. The other, more complicated, patch is just to enforce the invariant that NrSocket should only be destroyed on STS, but I'm pretty sure that NrSocketIpc can be destroyed on any thread with no ill effect. I can try to backport the smaller patch.

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5f14fa8d51ec
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/06f61809e6f7

Comment 70

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

Isn't e10s preffed off for 38 as shipped?

Comment 71

[Byron Campen [:bwc]]

I believe so?

Comment 72

[Daniel Veditz [:dveditz]]

(In reply to Nils Ohlmeier [:drno] from comment #0)
> Courtesy of external friends running some fuzzing for us:

Nils: looks like we may be writing a b2g advisory for this. Do your "external friends" have names/handles we should put in it?

Comment 73

[Nils Ohlmeier [:drno]]

(In reply to Daniel Veditz [:dveditz] from comment #72)
> Nils: looks like we may be writing a b2g advisory for this. Do your
> "external friends" have names/handles we should put in it?

I asked via email, but it looks like I won't get an answer before mid August. I'm happy to report back then. Let me know if we need to publish sooner then that.

Comment 74

[Nils Ohlmeier [:drno]]

So Patrick Höglund from Google (who is on CC on this bug) reported this bug to me. Feel free to add his name to the advisory.
Google requested to keep the bug closed to not disclose the fuzz test case which was used to discover this.