747213 - Protect the crash reporter from being unloaded

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Nick Cameron [:nrc]]

It is currently possible for other DLLs running in our process to override the FF crash reporter with their own or to revert to using the Windows default (which gives the Windows debug or kill dialog box). In either case, the user sees a Firefox crash, but it is not reported to us (bad).

Once we set the crash reported as the UnhandledExceptionFilter, we should not allow the UnhandledExceptionFilter to be set to NULL (which reverts to the default filter). Other software might want to run their own crash reporters whilst in their code, and this is probably legitimate, but preventing this is also an option.

Comment 1

[Nick Cameron [:nrc]]

See Bug 733892 for an example of where this is happening

Comment 2

[Nick Cameron [:nrc]]

Try run for patch: https://tbpl.mozilla.org/?tree=Try&rev=4bbb50d75444

Comment 3

[Nick Cameron [:nrc]]

Attachment: patch to protect the crash reporter

Comment 4

[(no longer active)]

Comment on attachment 617395 [details] [diff] [review]
patch to protect the crash reporter

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

I'd like to take a look at another version of this patch which addresses the comments below.

Also, since this is changing a breakpad header, please ask Ted for review as well.

Thanks!

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +660,5 @@
> +{
> +  if (gBlockUnhandledExceptionFilter) {
> +    // intercept attempts to change the filter
> +    return stub_SetUnhandledExceptionFilter
> +      (google_breakpad::ExceptionHandler::HandleException);

You don't need to set the exception handler multiple times.  I'd check lpTopLevelExceptionFilter against ExceptionHandler::HandleException, and if they're not equal, I'd just return NULL without calling SetUnhandledExceptionFilter.

@@ +854,5 @@
> +  // protect the crash reporter from being unloaded
> +  gKernel32Intercept.Init("kernel32.dll");
> +  bool ok = gKernel32Intercept.AddHook("SetUnhandledExceptionFilter",
> +         reinterpret_cast<intptr_t>(patched_SetUnhandledExceptionFilter),
> +         (void**) &stub_SetUnhandledExceptionFilter);

You need to check gBlockUnhandledExceptionFilter (or stub_SetUnhandledExceptionFilter) here to avoid hooking SetUnhandledExceptionFilter multiple times.

Comment 5

[Nick Cameron [:nrc]]

> ::: toolkit/crashreporter/nsExceptionHandler.cpp
> @@ +660,5 @@
> > +{
> > +  if (gBlockUnhandledExceptionFilter) {
> > +    // intercept attempts to change the filter
> > +    return stub_SetUnhandledExceptionFilter
> > +      (google_breakpad::ExceptionHandler::HandleException);
> 
> You don't need to set the exception handler multiple times.  I'd check
> lpTopLevelExceptionFilter against ExceptionHandler::HandleException, and if
> they're not equal, I'd just return NULL without calling
> SetUnhandledExceptionFilter.
> 

done

> @@ +854,5 @@
> > +  // protect the crash reporter from being unloaded
> > +  gKernel32Intercept.Init("kernel32.dll");
> > +  bool ok = gKernel32Intercept.AddHook("SetUnhandledExceptionFilter",
> > +         reinterpret_cast<intptr_t>(patched_SetUnhandledExceptionFilter),
> > +         (void**) &stub_SetUnhandledExceptionFilter);
> 
> You need to check gBlockUnhandledExceptionFilter (or
> stub_SetUnhandledExceptionFilter) here to avoid hooking
> SetUnhandledExceptionFilter multiple times.

The whole method only gets run once, and this is ensured at line 680, even if this does get run more than once, the attempt to set the hook will fail (you only hook a function once with our dll interceptor), and nothing bad will happen (well you'ld get a message if you're debugging, but that's all).

Comment 6

[Nick Cameron [:nrc]]

Attachment: updated patch with review comments

Comment 7

[Nick Cameron [:nrc]]

Received an email from Avast, who have been investigating the crash described in Bug 733892. Apparently the call to set the exception filter to NULL, comes from Windows, not them. This means that any DLL injected into the FF process could be causing crashes we don't know about because the crash reporter is disabled.

Comment 8

[(no longer active)]

(In reply to Nick Cameron [:nrc] from comment #7)
> Received an email from Avast, who have been investigating the crash
> described in Bug 733892. Apparently the call to set the exception filter to
> NULL, comes from Windows, not them. This means that any DLL injected into
> the FF process could be causing crashes we don't know about because the
> crash reporter is disabled.

Why is Windows doing that?

Comment 9

[(not currently active) Ted Mielczarek]

I haven't had a chance to look at this yet, but we need to be careful that our floating point exception filter (which we install after Breakpad, IIRC) still works:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsSigHandlers.cpp#368

Comment 10

[Nick Cameron [:nrc]]

(In reply to Ted Mielczarek [:ted] from comment #9)
> I haven't had a chance to look at this yet, but we need to be careful that
> our floating point exception filter (which we install after Breakpad, IIRC)
> still works:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsSigHandlers.
> cpp#368

I've just tested this, and the floating point exception filter (and hopefully any other exception handlers) are all installed before the crash reporter, and so work with this patch.

exception handlers:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2826

crash reporter and protection:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2967

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 617645 [details] [diff] [review]
updated patch with review comments

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

::: toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.h
@@ +239,5 @@
>    void UnregisterAppMemory(void *ptr);
>  
> +  // Called on the exception thread when an unhandled exception occurs.
> +  // Signals the exception handler thread to handle the exception.
> +  static LONG WINAPI HandleException(EXCEPTION_POINTERS* exinfo);

This is upstream Breakpad code. :-/ I can probably get this landed upstream, it's pretty inoffensive.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +652,5 @@
> +typedef LPTOP_LEVEL_EXCEPTION_FILTER (WINAPI *SetUnhandledExceptionFilter_func)
> +  (LPTOP_LEVEL_EXCEPTION_FILTER lpTopLevelExceptionFilter);
> +static SetUnhandledExceptionFilter_func stub_SetUnhandledExceptionFilter = 0;
> +WindowsDllInterceptor gKernel32Intercept;
> +bool gBlockUnhandledExceptionFilter = true;

All the file-static variables in this file are defined up top, please put these here as well. Also, you should declare these as static.

@@ +854,5 @@
> +  // protect the crash reporter from being unloaded
> +  gKernel32Intercept.Init("kernel32.dll");
> +  bool ok = gKernel32Intercept.AddHook("SetUnhandledExceptionFilter",
> +          reinterpret_cast<intptr_t>(patched_SetUnhandledExceptionFilter),
> +          (void**) &stub_SetUnhandledExceptionFilter);

Does the AddHook code know not to re-hook something that it has previously hooked? The API allows calling Set/UnsetExceptionHandler multiple times.

Comment 12

[Nick Cameron [:nrc]]

(In reply to Ted Mielczarek [:ted] from comment #11)
> Comment on attachment 617645 [details] [diff] [review]
> updated patch with review comments
> 
> Review of attachment 617645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/crashreporter/google-breakpad/src/client/windows/handler/
> exception_handler.h
> @@ +239,5 @@
> >    void UnregisterAppMemory(void *ptr);
> >  
> > +  // Called on the exception thread when an unhandled exception occurs.
> > +  // Signals the exception handler thread to handle the exception.
> > +  static LONG WINAPI HandleException(EXCEPTION_POINTERS* exinfo);
> 
> This is upstream Breakpad code. :-/ I can probably get this landed upstream,
> it's pretty inoffensive.
> 
Thanks!

> ::: toolkit/crashreporter/nsExceptionHandler.cpp
> @@ +652,5 @@
> > +typedef LPTOP_LEVEL_EXCEPTION_FILTER (WINAPI *SetUnhandledExceptionFilter_func)
> > +  (LPTOP_LEVEL_EXCEPTION_FILTER lpTopLevelExceptionFilter);
> > +static SetUnhandledExceptionFilter_func stub_SetUnhandledExceptionFilter = 0;
> > +WindowsDllInterceptor gKernel32Intercept;
> > +bool gBlockUnhandledExceptionFilter = true;
> 
> All the file-static variables in this file are defined up top, please put
> these here as well. Also, you should declare these as static.
> 
done

> @@ +854,5 @@
> > +  // protect the crash reporter from being unloaded
> > +  gKernel32Intercept.Init("kernel32.dll");
> > +  bool ok = gKernel32Intercept.AddHook("SetUnhandledExceptionFilter",
> > +          reinterpret_cast<intptr_t>(patched_SetUnhandledExceptionFilter),
> > +          (void**) &stub_SetUnhandledExceptionFilter);
> 
> Does the AddHook code know not to re-hook something that it has previously
> hooked? The API allows calling Set/UnsetExceptionHandler multiple times.

I'm not sure what you mean here. AddHook is meant to be able to handle multiple calls, but in practice, I found that it led to crashes, which is why I use a bool to enable blocking - I can't unload the patched version of SetUnhandledExceptionHandler. But, the call to AddHook will only be made once, so it is not a problem (ensured at line 680). Calling SetUnhandledExceptionFilter multiple times is supported just fine.

Comment 13

[Nick Cameron [:nrc]]

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> (In reply to Nick Cameron [:nrc] from comment #7)
> > Received an email from Avast, who have been investigating the crash
> > described in Bug 733892. Apparently the call to set the exception filter to
> > NULL, comes from Windows, not them. This means that any DLL injected into
> > the FF process could be causing crashes we don't know about because the
> > crash reporter is disabled.
> 
> Why is Windows doing that?

I have no idea, it seems weird. I didn't see this in my debug runs, but I was only debugging the user side, I don't have a remote debug setup for debugging the kernel side. Nor do I have any idea what is happening inside the Avast dll, other than some pretty opaque disassembly.

Comment 14

[Nick Cameron [:nrc]]

Attachment: updated patch with review comments

addressed reviewer's comments, carrying r+

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d65f0c082b1b

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d65f0c082b1b

Comment 17

[(not currently active) Ted Mielczarek]

FYI, upstream wasn't wild about making that API public, so I'm taking a different tack:
https://breakpad.appspot.com/456002/

I've rolled this into my patch on bug 791775.