1488806 - Crash in std::_Func_class<T>::operator()

Status: RESOLVED DUPLICATE of bug 1488827

(Toolkit :: Crash Reporting, enhancement)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-089f74d4-5f8b-4b6f-87a7-88df60180905.
=============================================================

Seen while looking at nightly crash stats - several crashes in the 20180905123750  build: https://bit.ly/2CngM8D. This particular one appears to be a UAF.

Outside of 64 there are a handful of crashes on 63, 62 and 61. No useful comments.

Top 10 frames of crashing thread:

0 xul.dll std::_Func_class<void, bool>::operator 
1 xul.dll void std::_Func_impl_no_alloc<`lambda at z:/build/build/src/toolkit/crashreporter/nsExceptionHandler.cpp:3868:36', void>::_Do_call 
2 xul.dll nsresult mozilla::detail::RunnableFunction<std::function<void  xpcom/threads/nsThreadUtils.h:564
3 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1161
4 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
5 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:125
6 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:318
7 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:298
8 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:158
9 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:415

=============================================================

Comment 1

[Eric Rahm [:erahm]]

The crash is in a crashreporter lambda [1]. Crash Address 0xffffffffe5e5e5e5 implies a UAF, presumably the instance that owns the function pointed to by `aCallback` has be freed prior to hitting the callback. We should audit the callbacks that get passed in to make sure any ref counted things are being held by an explicit RefPtr.

Gabriele, can you take a look?

[1] https://hg.mozilla.org/mozilla-central/annotate/26990836dc5cc3cd1b8027392b79210e71094dc3/toolkit/crashreporter/nsExceptionHandler.cpp#l3868

Comment 2

[Nathan Froyd [:froydnj]]

Is our lambda capture analysis failing to save us here?  We're doing things like:

https://hg.mozilla.org/mozilla-central/annotate/26990836dc5cc3cd1b8027392b79210e71094dc3/toolkit/crashreporter/nsExceptionHandler.cpp#l4004

which is capturing references to refcounted things and not complaining...

Comment 3

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Is our lambda capture analysis failing to save us here?  We're doing things
> like:
> 
> https://hg.mozilla.org/mozilla-central/annotate/
> 26990836dc5cc3cd1b8027392b79210e71094dc3/toolkit/crashreporter/
> nsExceptionHandler.cpp#l4004
> 
> which is capturing references to refcounted things and not complaining...

That example is `[=]`, so it's capturing by copy which should be okay (the obviously ref counted thing, `incomingDumpToPair`, is declared as a nsCOMPtr, not a raw pointer, so that should be okay). I don't know if our analysis handles nested lambda-captures which is probably the issue here.

It's also possible we don't have crash reporting enabled for whatever does this analysis so we don't compile this code?

Comment 4

[Nathan Froyd [:froydnj]]

Ah, right, I got mixed up between `incomingDumpToPair` and `aIncomingDumpToPair`, which are very different.

OK, so we're crashing in `NotifyDumpResult`'s lambda:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#3862-3870

`NotifyDumpResult` is only ever called from `CreatePairedChildMinidumpAsync`, which happens to pass in the `aCallback` parameter:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#3882-3938

which in turn is called from `CreateMinidumpsAndPair`, also passing through the callback:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#4004-4014

which in turn is only called from `CrashReporterHost::GenerateMinidumpAndPair`:

https://searchfox.org/mozilla-central/source/ipc/glue/CrashReporterHost.cpp#229-235

and the callback that's gotten threaded through all of this is:

https://searchfox.org/mozilla-central/source/ipc/glue/CrashReporterHost.cpp#212-227

which I think gets all its refcounting correct (assuming that lambda capture list copies, and doesn't take references), but what about the reference to `this`?  `CrashReporterHost` is not refcounted:

https://searchfox.org/mozilla-central/source/ipc/glue/CrashReporterHost.h

so if we manage to free CrashReporterHost before that runnable happens, we are in trouble...

Comment 5

[Gabriele Svelto [:gsvelto]]

Bug 1488827 was also filed for this and it's not marked sec-high.

The changes that were introduced in bug 1360308 are proving to be too problematic; I'll manually back out those changes and go back to the previous synchronous setup. It was done to avoid hangs, but if we get crashes instead it doesn't really help. Anyway we intend to rip out all this stuff starting in December.

Comment 6

[Gabriele Svelto [:gsvelto]]

The patch that caused this was backed out, feel free to close this bug.