Last Comment Bug 747213 - Protect the crash reporter from being unloaded
: Protect the crash reporter from being unloaded
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-19 15:40 PDT by Nick Cameron [:nrc]
Modified: 2013-03-31 04:26 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to protect the crash reporter (6.38 KB, patch)
2012-04-22 22:54 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
updated patch with review comments (6.37 KB, patch)
2012-04-23 14:21 PDT, Nick Cameron [:nrc]
ehsan: review+
ted: review+
Details | Diff | Splinter Review
updated patch with review comments (6.42 KB, patch)
2012-04-30 20:57 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-04-19 15:40:09 PDT
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] 2012-04-19 15:40:40 PDT
See Bug 733892 for an example of where this is happening
Comment 2 Nick Cameron [:nrc] 2012-04-22 22:53:27 PDT
Try run for patch: https://tbpl.mozilla.org/?tree=Try&rev=4bbb50d75444
Comment 3 Nick Cameron [:nrc] 2012-04-22 22:54:53 PDT
Created attachment 617395 [details] [diff] [review]
patch to protect the crash reporter
Comment 4 :Ehsan Akhgari 2012-04-23 07:48:32 PDT
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] 2012-04-23 13:18:45 PDT
> ::: 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] 2012-04-23 14:21:42 PDT
Created attachment 617645 [details] [diff] [review]
updated patch with review comments
Comment 7 Nick Cameron [:nrc] 2012-04-25 15:27:05 PDT
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 :Ehsan Akhgari 2012-04-25 17:47:56 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-04-26 10:44:47 PDT
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] 2012-04-26 18:07:38 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-04-30 14:01:13 PDT
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] 2012-04-30 20:40:50 PDT
(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] 2012-04-30 20:43:44 PDT
(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] 2012-04-30 20:57:36 PDT
Created attachment 619826 [details] [diff] [review]
updated patch with review comments

addressed reviewer's comments, carrying r+
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-02 22:34:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65f0c082b1b
Comment 16 Ed Morley [:emorley] 2012-05-04 02:36:21 PDT
https://hg.mozilla.org/mozilla-central/rev/d65f0c082b1b
Comment 17 Ted Mielczarek [:ted.mielczarek] 2012-09-17 11:51:38 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.