Closed Bug 747213 Opened 9 years ago Closed 9 years ago

Protect the crash reporter from being unloaded

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: nrc, Assigned: nrc)

Details

Attachments

(1 file, 2 obsolete files)

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.
See Bug 733892 for an example of where this is happening
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)
> ::: 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).
Attachment #617395 - Attachment is obsolete: true
Attachment #617645 - Flags: review?(ted.mielczarek)
Attachment #617645 - Flags: review?(ehsan)
Attachment #617645 - Flags: review?(ehsan) → review+
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
(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+
(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.
(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.
addressed reviewer's comments, carrying r+
Attachment #617645 - Attachment is obsolete: true
Attachment #619826 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d65f0c082b1b
Status: NEW → RESOLVED
Closed: 9 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.