win: Add signal handler for SIGFPE and SIGABRT [crashpad/crashpad : master]
Scott Graham (Gerrit)
Scott Graham posted comments on win: Add signal handler for SIGFPE and SIGABRT.
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on win: Add signal handler for SIGFPE and SIGABRT.
Scott Graham (Gerrit)
Scott Graham posted comments on win: Add signal handler for SIGFPE and SIGABRT.
Scott Graham (Gerrit)
Scott Graham posted comments on win: Add signal handler for SIGFPE and SIGABRT.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on win: Add signal handler for SIGFPE and SIGABRT.
File client/crashpad_client_win.cc:
Patch Set #2, Line 168:
// VS's signal.h lists:The CRT’s, since it’s basically unbundled from VS now.
Patch Set #2, Line 177:
register forThe comment about registering belongs in the function that does the registering, which is way later in the file.
SIGBREAK is // undocumented
But we can see what’s going on with BREAK in the CRT source. It’s dealt with the same as INT. Good to capture that information, at least.
No “we,” “our” (a couple of lines down).
Patch Set #2, Line 178:
// SIGFPE and SIGABRT, and use _pxcptinfoptrs from signal.h to send theTERM and INT wouldn’t be dump-worthy signals in any event. BREAK is non-standard but I wouldn’t expect it to be either. But you never mentioned SEGV. If that can show up here, it ought to be dump-worthy. Problem is, things that would show up as SEGV would normally arrive as exceptions. We wouldn’t want to take them as signals. It ought to be the same situation for FPE. So my question is: why do you want to take FPE through the signal interface instead of catching the underlying exception?
Patch Set #2, Line 180:
// primarily for SIGABRT which in non-Debug builds without a SIGABRT handlerYour last sentence here basically sums up what I’m saying: if we can receive something as an exception instead of a signal, we should do that. If someone steals away our ability to receive those exceptions because they’ve installed a signal handler, then sure, that’s too bad, but they could steal it away from us regardless of whether we’re trying to catch it as an exception or a signal. I see the utility here for ABRT. But presumably you did it for FPE for a reason too, and I don’t see it yet.
Patch Set #2, Line 183:
void HandleSIG(int signum) {HandleSignal
Patch Set #2, Line 184:
PEXCEPTION_POINTERS exception_pointers_pointer =EXCEPTION_POINTERS* (like you said on the next line)
Patch Set #2, Line 191:
// only for exception-like signals), fabricate one here.(it doesn’t for SIGABRT because that never originates as an exception)
Patch Set #2, Line 195:
record.ExceptionCode = signum;Sticking a non-exception into a hole that everyone expects will hold exceptions. Is that the right call? Is there a more appropriate single exception that we can use to say “this was a signal”, and then stick the signal number in ExceptionInformation?
Patch Set #2, Line 201:
exception_pointers.ExceptionRecord = &record;This pointer goes bad as soon as we fall out of scope. Move record out of this scope too.
Patch Set #2, Line 546:
signal(SIGFPE, HandleSIG);ucrt source tells me that signal() is effective per-thread for FPE, not per-process. That…stinks. But like I was saying above, I can’t see why we’d want FPE as a signal anyway, so it might not matter.
File handler/win/crashy_signal.cc:
Patch Set #2, Line 50:
_controlfp_s(It’s still not a bad idea to have a test for floating-point exceptions, since they’re a little weird in that they require unmasking and people might not otherwise notice quickly if we lost coverage for them. But it shouldn’t have to be a signal-based test here.
Patch Set #2, Line 57:
abort();Makes this program much more straightforward, though.
To view, visit this change. To unsubscribe, visit settings.
Scott Graham (Gerrit)
Scott Graham posted comments on win: Add signal handler for SIGFPE and SIGABRT.
File client/crashpad_client_win.cc:
Patch Set #2, Line 168:
void HandleAbortSignal(int signum) {
The CRT’s, since it’s basically unbundled from VS now.
Done
Patch Set #2, Line 177:
EXCEPTION_RECORD record = {};
No “we,” “our” (a couple of lines down).
Done
Patch Set #2, Line 177:
EXCEPTION_RECORD record = {};
But we can see what’s going on with BREAK in the CRT source. It’s dealt wit
Done
Patch Set #2, Line 177:
EXCEPTION_RECORD record = {};
The comment about registering belongs in the function that does the registe
Done
Patch Set #2, Line 178:
record.ExceptionCode = STATUS_FATAL_APP_EXIT;
TERM and INT wouldn’t be dump-worthy signals in any event. BREAK is non-sta
Done
Patch Set #2, Line 180:
record.ExceptionAddress = reinterpret_cast<void*>(context.Rip);
Your last sentence here basically sums up what I’m saying: if we can receiv
Agreed, removed FPE.
Patch Set #2, Line 195:
static PPROC_THREAD_ATTRIBUTE_LIST InvalidValue() { return nullptr; }
Sticking a non-exception into a hole that everyone expects will hold except
Probably not the right call. The particular value for SIGABRT probably isn't that useful either. Now that it's only for SIGABRT, the best existing status code I can see is STATUS_FATAL_APP_EXIT, so I'll use that.
Patch Set #2, Line 201:
static const auto delete_proc_thread_attribute_list =
This pointer goes bad as soon as we fall out of scope. Move record out of t
Oof! Done.
Patch Set #2, Line 546:
signal(SIGABRT, HandleAbortSignal);
ucrt source tells me that signal() is effective per-thread for FPE, not per
Hmm, yeah. :( That would be a horrible mess to set up API-wise. I tried signalling from a background thread and it does seem to work, though I'm not sure why given what you found. ... Oh, right. If there's a signal handler then that's run, but if there's not, then it does end up in the unhandled exception filter. (It looked like it worked locally because the signal handler just called the UEF). I agree that I probably shouldn't have added the FPE handler then, so I'm going to simplify all this and only add SIGABRT in this CL.
File handler/win/crashy_signal.cc:
It’s still not a bad idea to have a test for floating-point exceptions, sin
Right. I'll do this elsewhere in a followup.
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on win: Add signal handler for SIGABRT to handle abort() calls.
File client/crashpad_client_win.cc:
Patch Set #5, Line 171:
EXCEPTION_POINTERS exception_pointers = {};You’re unconditionally setting both fields of this, so no need to {}. Maybe it’d be clearer if you held off on declaring this until you had both of the fields ready to put in it.
Patch Set #5, Line 178:
record.ExceptionCode = STATUS_FATAL_APP_EXIT;Put EXCEPTION_NONCONTINUABLE in ExceptionFlags. That’s what abort() would give to ReportFault() if it decided to call it.
Patch Set #5, Line 546:
signal(SIGABRT, HandleAbortSignal);Save off the return value and DCHECK_NE(rv, SIG_ERR).
Patch Set #5, Line 155:
'../third_party/mini_chromium/mini_chromium.gyp:base',Not used by crashy_signal.cc.
Patch Set #5, Line 156:
'../util/util.gyp:crashpad_util',Neither is this one.
File handler/win/crashy_signal.cc:
Patch Set #5, Line 39:
void DoSignal(int sig) {Unused, remove.
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on win: Add signal handler for SIGABRT to handle abort() calls.
File client/crashpad_client_win.cc:
Patch Set #5, Line 545:
// expect it to cause a crash dump.Maybe add something here saying that this will only catch abort()s called in the same CRT that’s in use here.
Scott Graham (Gerrit)
Scott Graham posted comments on win: Add signal handler for SIGABRT to handle abort() calls.
File client/crashpad_client_win.cc:
Patch Set #5, Line 171:
EXCEPTION_POINTERS exception_pointers = {};
You’re unconditionally setting both fields of this, so no need to {}. Maybe
Done
Patch Set #5, Line 178:
record.ExceptionCode = STATUS_FATAL_APP_EXIT;Put EXCEPTION_NONCONTINUABLE in ExceptionFlags. That’s what abort() would g
Done
Patch Set #5, Line 545:
// expect it to cause a crash dump.Maybe add something here saying that this will only catch abort()s called i
Done
Patch Set #5, Line 546:
signal(SIGABRT, HandleAbortSignal);Save off the return value and DCHECK_NE(rv, SIG_ERR).
Patch Set #5, Line 155:
'../third_party/mini_chromium/mini_chromium.gyp:base',Not used by crashy_signal.cc.
Hmm. I think crashpad_client needs a xyz_dependent, as crashpad_client.h includes files from base. I'll tidy that up separately.
Patch Set #5, Line 156:
'../util/util.gyp:crashpad_util',Neither is this one.
Done
Mark Mentovai
Scott Graham
Gerrit’s broken! Old-school:crashy_signal.cc:70:Since you are depending on base anyway, there’s no reason you can’t PLOG() this instead. Same on line 74.
--
You received this message because you are subscribed to the Google Groups "Crashpad-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to crashpad-dev+unsubscribe@chromium.org.
To post to this group, send email to crashp...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/crashpad-dev/CAHk1GDyGkHGvV7dvK3bmqW8vTkwJ%3Dj-wXCiW%2Bsfz6KC8_pb83g%40mail.gmail.com.
Scott Graham (Gerrit)
Scott Graham merged win: Add signal handler for SIGABRT to handle abort() calls.
win: Add signal handler for SIGABRT to handle abort() calls R=ma...@chromium.org BUG=crashpad:57 Change-Id: Ib7141f00e74e3db9e5be427cc990847331e09912 Reviewed-on: https://chromium-review.googlesource.com/412058 Reviewed-by: Mark Mentovai <ma...@chromium.org> --- M client/crashpad_client_win.cc M handler/handler.gyp A handler/win/crashy_signal.cc M snapshot/win/end_to_end_test.py 4 files changed, 182 insertions(+), 2 deletions(-)
Approvals: Mark Mentovai: Looks good to me
diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc
index 235ef18..6f93f7a 100644
--- a/client/crashpad_client_win.cc
+++ b/client/crashpad_client_win.cc
@@ -15,6 +15,7 @@
#include "client/crashpad_client.h"
#include <windows.h>
+#include <signal.h>
#include <stdint.h>
#include <string.h>
@@ -31,6 +32,7 @@
#include "util/file/file_io.h"
#include "util/misc/random_string.h"
#include "util/win/address_types.h"
+#include "util/win/capture_context.h"
#include "util/win/command_line.h"
#include "util/win/critical_section_with_debug_info.h"
#include "util/win/get_function.h"
@@ -161,6 +163,28 @@
TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump);
return EXCEPTION_CONTINUE_SEARCH;
+}
+
+void HandleAbortSignal(int signum) {
+ DCHECK_EQ(signum, SIGABRT);
+
+ CONTEXT context;
+ CaptureContext(&context);
+
+ EXCEPTION_RECORD record = {};
+ record.ExceptionCode = STATUS_FATAL_APP_EXIT;
+ record.ExceptionFlags = EXCEPTION_NONCONTINUABLE;
+#if defined(ARCH_CPU_64_BITS)
+ record.ExceptionAddress = reinterpret_cast<void*>(context.Rip);
+#else
+ record.ExceptionAddress = reinterpret_cast<void*>(context.Eip);
+#endif // ARCH_CPU_64_BITS
+
+ EXCEPTION_POINTERS exception_pointers;
+ exception_pointers.ContextRecord = &context;
+ exception_pointers.ExceptionRecord = &record;
+
+ UnhandledExceptionHandler(&exception_pointers);
}
std::wstring FormatArgumentString(const std::string& name,
@@ -500,6 +524,32 @@
g_non_crash_dump_lock = new base::Lock();
}
+void RegisterHandlers() {
+ SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
+
+ // The Windows CRT's signal.h lists:
+ // - SIGINT
+ // - SIGILL
+ // - SIGFPE
+ // - SIGSEGV
+ // - SIGTERM
+ // - SIGBREAK
+ // - SIGABRT
+ // SIGILL and SIGTERM are documented as not being generated. SIGBREAK and
+ // SIGINT are for Ctrl-Break and Ctrl-C, and aren't something for which
+ // capturing a dump is warranted. SIGFPE and SIGSEGV are captured as regular
+ // exceptions through the unhandled exception filter. This leaves SIGABRT. In
+ // the standard CRT, abort() is implemented as a synchronous call to the
+ // SIGABRT signal handler if installed, but after doing so, the unhandled
+ // exception filter is not triggered (it instead __fastfail()s). So, register
+ // to handle SIGABRT to catch abort() calls, as client code might use this and
+ // expect it to cause a crash dump. This will only work when the abort()
+ // that's called in client code is the same (or has the same behavior) as the
+ // one in use here.
+ _crt_signal_t rv = signal(SIGABRT, HandleAbortSignal);
+ DCHECK_NE(rv, SIG_ERR);
+}
+
} // namespace
CrashpadClient::CrashpadClient() : ipc_pipe_(), handler_start_thread_() {}
@@ -536,7 +586,7 @@
CommonInProcessInitialization();
- SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
+ RegisterHandlers();
auto data = new BackgroundHandlerStartThreadData(handler,
database,
@@ -609,7 +659,8 @@
}
SetHandlerStartupState(StartupState::kSucceeded);
- SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
+
+ RegisterHandlers();
// The server returns these already duplicated to be valid in this process.
g_signal_exception =
diff --git a/handler/handler.gyp b/handler/handler.gyp
index d690053..e2d993d 100644
--- a/handler/handler.gyp
+++ b/handler/handler.gyp
@@ -148,6 +148,20 @@
],
},
{
+ 'target_name': 'crashy_signal',
+ 'type': 'executable',
+ 'dependencies': [
+ '../client/client.gyp:crashpad_client',
+ '../third_party/mini_chromium/mini_chromium.gyp:base',
+ ],
+ 'include_dirs': [
+ '..',
+ ],
+ 'sources': [
+ 'win/crashy_signal.cc',
+ ],
+ },
+ {
'target_name': 'crash_other_program',
'type': 'executable',
'dependencies': [
diff --git a/handler/win/crashy_signal.cc b/handler/win/crashy_signal.cc
new file mode 100644
index 0000000..634bce8
--- /dev/null
+++ b/handler/win/crashy_signal.cc
@@ -0,0 +1,91 @@
+// Copyright 2016 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <stdlib.h>
+#include <string.h>
+#include <windows.h>
+
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
+#include "client/crashpad_client.h"
+
+namespace crashpad {
+namespace {
+
+enum WhereToSignalFrom {
+ kUnknown = -1,
+ kMain = 0,
+ kBackground = 1,
+};
+
+WhereToSignalFrom MainOrBackground(wchar_t* name) {
+ if (wcscmp(name, L"main") == 0)
+ return kMain;
+ if (wcscmp(name, L"background") == 0)
+ return kBackground;
+ return kUnknown;
+}
+
+DWORD WINAPI BackgroundThread(void* arg) {
+ abort();
+ return 0;
+}
+
+int CrashySignalMain(int argc, wchar_t* argv[]) {
+ CrashpadClient client;
+
+ WhereToSignalFrom from;
+ if (argc == 3 && (from = MainOrBackground(argv[2])) != kUnknown) {
+ if (!client.SetHandlerIPCPipe(argv[1])) {
+ LOG(ERROR) << "SetHandler";
+ return EXIT_FAILURE;
+ }
+ } else {
+ LOG(ERROR) << "Usage: " << base::UTF16ToUTF8(argv[0])
+ << " <server_pipe_name> main|background";
+ return EXIT_FAILURE;
+ }
+
+ // In debug builds part of abort() is to open a dialog. We don't want tests to
+ // block at that dialog, so disable it.
+ _set_abort_behavior(0, _WRITE_ABORT_MSG);
+
+ if (from == kBackground) {
+ HANDLE thread = CreateThread(nullptr,
+ 0,
+ &BackgroundThread,
+ nullptr,
+ 0,
+ nullptr);
+ if (!thread) {
+ PLOG(ERROR) << "CreateThread";
+ return EXIT_FAILURE;
+ }
+ if (WaitForSingleObject(thread, INFINITE) != WAIT_OBJECT_0) {
+ PLOG(ERROR) << "WaitForSingleObject";
+ return EXIT_FAILURE;
+ }
+ } else {
+ abort();
+ }
+
+ return EXIT_SUCCESS;
+}
+
+} // namespace
+} // namespace crashpad
+
+int wmain(int argc, wchar_t* argv[]) {
+ return crashpad::CrashySignalMain(argc, argv);
+}
diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py
index f469bcf..42999df 100755
--- a/snapshot/win/end_to_end_test.py
+++ b/snapshot/win/end_to_end_test.py
@@ -143,6 +143,10 @@
*args)
+def GetDumpFromSignal(out_dir, pipe_name, *args):
+ return GetDumpFromProgram(out_dir, pipe_name, 'crashy_signal.exe', *args)
+
+
def GetDumpFromSelfDestroyingProgram(out_dir, pipe_name):
return GetDumpFromProgram(out_dir, pipe_name, 'self_destroying_program.exe')
@@ -201,6 +205,8 @@
z7_dump_path,
other_program_path,
other_program_no_exception_path,
+ sigabrt_main_path,
+ sigabrt_background_path,
pipe_name):
"""Runs various tests in sequence. Runs a new cdb instance on the dump for
each block of tests to reduce the chances that output from one command is
@@ -361,6 +367,13 @@
'other program with no exception given')
out.Check('!RaiseException', 'other program in RaiseException()')
+ out = CdbRun(cdb_path, sigabrt_main_path, '.ecxr')
+ out.Check('code 40000015', 'got sigabrt signal')
+ out.Check('::HandleAbortSignal', ' stack in expected location')
+
+ out = CdbRun(cdb_path, sigabrt_background_path, '.ecxr')
+ out.Check('code 40000015', 'got sigabrt signal from background thread')
+
def main(args):
try:
@@ -411,6 +424,15 @@
if not other_program_no_exception_path:
return 1
+ sigabrt_main_path = GetDumpFromSignal(args[0], pipe_name, 'main')
+ if not sigabrt_main_path:
+ return 1
+
+ sigabrt_background_path = GetDumpFromSignal(
+ args[0], pipe_name, 'background')
+ if not sigabrt_background_path:
+ return 1
+
RunTests(cdb_path,
crashy_dump_path,
start_handler_dump_path,
@@ -418,6 +440,8 @@
z7_dump_path,
other_program_path,
other_program_no_exception_path,
+ sigabrt_main_path,
+ sigabrt_background_path,
pipe_name)
return 1 if g_had_failures else 0
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on win: Add signal handler for SIGABRT to handle abort() calls.
File handler/win/crashy_signal.cc:
Patch Set #8, Line 55:
LOG(ERROR) << "Usage: " << base::UTF16ToUTF8(argv[0])Doesn’t really matter now, but we usually use fprintf(stderr, …) directly for usage messages, since the extra “log” goop isn’t really useful for them.
To view, visit this change. To unsubscribe, visit settings.
Scott Graham (Gerrit)
Scott Graham posted comments on win: Add signal handler for SIGABRT to handle abort() calls.
Patch Set #8, Line 55:
LOG(ERROR) << "Usage: " << base::UTF16ToUTF8(argv[0])Doesn’t really matter now, but we usually use fprintf(stderr, …) directly f
Will do. Apparently I broke VS2013 again, so I'll get this in the followup.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on win: Add signal handler for SIGABRT to handle abort() calls.
Patch Set #8, Line 55:
LOG(ERROR) << "Usage: " << base::UTF16ToUTF8(argv[0])
Will do. Apparently I broke VS2013 again, so I'll get this in the followup.
> Will do. Apparently I broke VS2013 again, so I'll get this in the followup. Every Windows change needs a follow-up these days! Mine sure have. I was (happily!) surprised that they even provided a typedef for signal handlers, because POSIX certainly doesn’t specify a standard one. Oh well.