Protect the crash reporter from being unloaded

RESOLVED FIXED in mozilla15

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

unspecified
mozilla15
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
See Bug 733892 for an example of where this is happening
(Assignee)

Comment 2

5 years ago
Try run for patch: https://tbpl.mozilla.org/?tree=Try&rev=4bbb50d75444
(Assignee)

Comment 3

5 years ago
Created attachment 617395 [details] [diff] [review]
patch to protect the crash reporter
Attachment #617395 - Flags: review?(ehsan)
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.
Attachment #617395 - Flags: review?(ehsan)
(Assignee)

Comment 5

5 years ago
> ::: 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).
(Assignee)

Comment 6

5 years ago
Created attachment 617645 [details] [diff] [review]
updated patch with review comments
Attachment #617395 - Attachment is obsolete: true
Attachment #617645 - Flags: review?(ted.mielczarek)
Attachment #617645 - Flags: review?(ehsan)
Attachment #617645 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

5 years ago
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.
(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 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
(Assignee)

Comment 10

5 years ago
(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 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.
Attachment #617645 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 619826 [details] [diff] [review]
updated patch with review comments

addressed reviewer's comments, carrying r+
Attachment #617645 - Attachment is obsolete: true
Attachment #619826 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65f0c082b1b
https://hg.mozilla.org/mozilla-central/rev/d65f0c082b1b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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.
You need to log in before you can comment on or make changes to this bug.