From 0ded3d718f4b090d106014bb0ea9dbb7af4e1d81 Mon Sep 17 00:00:00 2001 From: doshimun Date: Mon, 5 May 2008 20:03:56 +0000 Subject: [PATCH] Add a way for the client apps to specify custom information in case of out-of-process scenarios that the OOP server can use in whatever way it wants to. Fix a bug in CrashGenerationserver where CreateNamedPipe failure was not checked correctly. TODO in near future: Add a custom stream to minidump files for the custom information. git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@267 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/windows/common/ipc_protocol.h | 50 +++++++++++++++++++ .../windows/crash_generation/client_info.cc | 36 +++++++++++-- .../windows/crash_generation/client_info.h | 17 ++++++- .../crash_generation_client.cc | 28 +++++++---- .../crash_generation_client.h | 10 +++- .../crash_generation_server.cc | 6 ++- .../windows/handler/exception_handler.cc | 15 ++++-- .../windows/handler/exception_handler.h | 7 ++- .../crash_generation_app.cc | 42 +++++++++++++++- .../tests/crash_generation_app/precompile.h | 1 + 10 files changed, 187 insertions(+), 25 deletions(-) diff --git a/src/client/windows/common/ipc_protocol.h b/src/client/windows/common/ipc_protocol.h index 831be6cb..35b99016 100644 --- a/src/client/windows/common/ipc_protocol.h +++ b/src/client/windows/common/ipc_protocol.h @@ -32,10 +32,49 @@ #include #include +#include +#include #include "google_breakpad/common/minidump_format.h" namespace google_breakpad { +// Name/value pair for custom client information. +struct CustomInfoEntry { + // Maximum length for name and value for client custom info. + static const int kNameMaxLength = 64; + static const int kValueMaxLength = 64; + + CustomInfoEntry() { + // Putting name and value in initializer list makes VC++ show warning 4351. + set_name(NULL); + set_value(NULL); + } + + CustomInfoEntry(const wchar_t* name_arg, const wchar_t* value_arg) { + set_name(name_arg); + set_value(value_arg); + } + + void set_name(const wchar_t* name_arg) { + if (!name_arg) { + name[0] = L'\0'; + return; + } + wcscpy_s(name, kNameMaxLength, name_arg); + } + + void set_value(const wchar_t* value_arg) { + if (!value_arg) { + value[0] = L'\0'; + return; + } + wcscpy_s(value, kValueMaxLength, value_arg); + } + + wchar_t name[kNameMaxLength]; + wchar_t value[kValueMaxLength]; +}; + // Constants for the protocol between client and the server. // Tags sent with each message indicating the purpose of @@ -47,6 +86,11 @@ enum MessageTag { MESSAGE_TAG_REGISTRATION_ACK = 3 }; +struct CustomClientInfo { + const CustomInfoEntry* entries; + int count; +}; + // Message structure for IPC between crash client and crash server. struct ProtocolMessage { ProtocolMessage() @@ -56,6 +100,7 @@ struct ProtocolMessage { thread_id(0), exception_pointers(NULL), assert_info(NULL), + custom_client_info(), dump_request_handle(NULL), dump_generated_handle(NULL), server_alive_handle(NULL) { @@ -68,6 +113,7 @@ struct ProtocolMessage { DWORD* arg_thread_id, EXCEPTION_POINTERS** arg_exception_pointers, MDRawAssertionInfo* arg_assert_info, + const CustomClientInfo& custom_info, HANDLE arg_dump_request_handle, HANDLE arg_dump_generated_handle, HANDLE arg_server_alive) @@ -77,6 +123,7 @@ struct ProtocolMessage { thread_id(arg_thread_id), exception_pointers(arg_exception_pointers), assert_info(arg_assert_info), + custom_client_info(custom_info), dump_request_handle(arg_dump_request_handle), dump_generated_handle(arg_dump_generated_handle), server_alive_handle(arg_server_alive) { @@ -101,6 +148,9 @@ struct ProtocolMessage { // pure call failure. MDRawAssertionInfo* assert_info; + // Custom client information. + CustomClientInfo custom_client_info; + // Handle to signal the crash event. HANDLE dump_request_handle; diff --git a/src/client/windows/crash_generation/client_info.cc b/src/client/windows/crash_generation/client_info.cc index 35dd27b0..48886d2a 100644 --- a/src/client/windows/crash_generation/client_info.cc +++ b/src/client/windows/crash_generation/client_info.cc @@ -28,6 +28,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "client/windows/crash_generation/client_info.h" +#include "client/windows/common/ipc_protocol.h" namespace google_breakpad { @@ -36,12 +37,14 @@ ClientInfo::ClientInfo(CrashGenerationServer* crash_server, MINIDUMP_TYPE dump_type, DWORD* thread_id, EXCEPTION_POINTERS** ex_info, - MDRawAssertionInfo* assert_info) + MDRawAssertionInfo* assert_info, + const CustomClientInfo& custom_client_info) : crash_server_(crash_server), pid_(pid), dump_type_(dump_type), ex_info_(ex_info), assert_info_(assert_info), + custom_client_info_(custom_client_info), thread_id_(thread_id), process_handle_(NULL), dump_requested_handle_(NULL), @@ -117,8 +120,7 @@ bool ClientInfo::UnregisterWaits() { return success; } -bool ClientInfo::GetClientExceptionInfo( - EXCEPTION_POINTERS** ex_info) const { +bool ClientInfo::GetClientExceptionInfo(EXCEPTION_POINTERS** ex_info) const { SIZE_T bytes_count = 0; if (!ReadProcessMemory(process_handle_, ex_info_, @@ -144,4 +146,32 @@ bool ClientInfo::GetClientThreadId(DWORD* thread_id) const { return bytes_count == sizeof(*thread_id); } +bool ClientInfo::PopulateCustomInfo() { + SIZE_T bytes_count = 0; + SIZE_T read_count = sizeof(CustomInfoEntry) * custom_client_info_.count; + + // If the scoped array for custom info already has an array, it will be + // the same size as what we need. This is because the number of custom info + // entries is always the same. So allocate memory only if scoped array has + // a NULL pointer. + if (!custom_info_entries_.get()) { + custom_info_entries_.reset(new CustomInfoEntry[custom_client_info_.count]); + } + + if (!ReadProcessMemory(process_handle_, + custom_client_info_.entries, + custom_info_entries_.get(), + read_count, + &bytes_count)) { + return false; + } + + return bytes_count == read_count; +} + +int ClientInfo::GetCustomInfo(CustomInfoEntry const** custom_info) const { + *custom_info = custom_info_entries_.get(); + return custom_client_info_.count; +} + } // namespace google_breakpad diff --git a/src/client/windows/crash_generation/client_info.h b/src/client/windows/crash_generation/client_info.h index 9956c47a..db3f05e9 100644 --- a/src/client/windows/crash_generation/client_info.h +++ b/src/client/windows/crash_generation/client_info.h @@ -32,7 +32,9 @@ #include #include +#include "client/windows/common/ipc_protocol.h" #include "google_breakpad/common/minidump_format.h" +#include "processor/scoped_ptr.h" namespace google_breakpad { @@ -49,7 +51,8 @@ class ClientInfo { MINIDUMP_TYPE dump_type, DWORD* thread_id, EXCEPTION_POINTERS** ex_info, - MDRawAssertionInfo* assert_info); + MDRawAssertionInfo* assert_info, + const CustomClientInfo& custom_client_info); ~ClientInfo(); @@ -85,6 +88,10 @@ class ClientInfo { bool Initialize(); bool GetClientExceptionInfo(EXCEPTION_POINTERS** ex_info) const; bool GetClientThreadId(DWORD* thread_id) const; + // Reads the custom information from the client process address space. + bool PopulateCustomInfo(); + // Returns the client custom information. + int GetCustomInfo(CustomInfoEntry const** custom_info) const; private: // Crash generation server. @@ -113,6 +120,14 @@ class ClientInfo { // in the address space of another process. MDRawAssertionInfo* assert_info_; + // Custom information about the client. + CustomClientInfo custom_client_info_; + + // Contains the custom client info entries read from the client process + // memory. This will be populated only if the method GetClientCustomInfo + // is called. + scoped_array custom_info_entries_; + // Address of a variable in the client process address space that // will contain the thread id of the crashing client thread. // diff --git a/src/client/windows/crash_generation/crash_generation_client.cc b/src/client/windows/crash_generation/crash_generation_client.cc index 4ec932c3..197807ab 100644 --- a/src/client/windows/crash_generation/crash_generation_client.cc +++ b/src/client/windows/crash_generation/crash_generation_client.cc @@ -29,6 +29,7 @@ #include "client/windows/crash_generation/crash_generation_client.h" #include +#include #include "client/windows/common/ipc_protocol.h" namespace google_breakpad { @@ -89,17 +90,23 @@ static bool TransactNamedPipeDebugHelper(HANDLE pipe, } **/ -CrashGenerationClient::CrashGenerationClient(const wchar_t* pipe_name, - MINIDUMP_TYPE dump_type) - : pipe_name_(pipe_name), - dump_type_(dump_type), - thread_id_(0), - server_process_id_(0), - crash_event_(NULL), - crash_generated_(NULL), - server_alive_(NULL), - exception_pointers_(NULL) { +CrashGenerationClient::CrashGenerationClient( + const wchar_t* pipe_name, + MINIDUMP_TYPE dump_type, + const CustomClientInfo* custom_info) + : pipe_name_(pipe_name), + dump_type_(dump_type), + thread_id_(0), + server_process_id_(0), + crash_event_(NULL), + crash_generated_(NULL), + server_alive_(NULL), + exception_pointers_(NULL), + custom_info_() { memset(&assert_info_, 0, sizeof(assert_info_)); + if (custom_info) { + custom_info_ = *custom_info; + } } CrashGenerationClient::~CrashGenerationClient() { @@ -184,6 +191,7 @@ bool CrashGenerationClient::RegisterClient(HANDLE pipe) { &thread_id_, &exception_pointers_, &assert_info_, + custom_info_, NULL, NULL, NULL); diff --git a/src/client/windows/crash_generation/crash_generation_client.h b/src/client/windows/crash_generation/crash_generation_client.h index fdec1c26..81b0e6ca 100644 --- a/src/client/windows/crash_generation/crash_generation_client.h +++ b/src/client/windows/crash_generation/crash_generation_client.h @@ -33,10 +33,14 @@ #include #include #include +#include #include "client/windows/common/ipc_protocol.h" +#include "processor/scoped_ptr.h" namespace google_breakpad { +struct CustomClientInfo; + // Abstraction of client-side implementation of out of process // crash generation. // @@ -59,7 +63,8 @@ namespace google_breakpad { class CrashGenerationClient { public: CrashGenerationClient(const wchar_t* pipe_name, - MINIDUMP_TYPE dump_type); + MINIDUMP_TYPE dump_type, + const CustomClientInfo* custom_info); ~CrashGenerationClient(); @@ -115,6 +120,9 @@ class CrashGenerationClient { // Pipe name to use to talk to server. std::wstring pipe_name_; + // Custom client information + CustomClientInfo custom_info_; + // Type of dump to generate. MINIDUMP_TYPE dump_type_; diff --git a/src/client/windows/crash_generation/crash_generation_server.cc b/src/client/windows/crash_generation/crash_generation_server.cc index 9be11ee7..b13db9db 100644 --- a/src/client/windows/crash_generation/crash_generation_server.cc +++ b/src/client/windows/crash_generation/crash_generation_server.cc @@ -220,7 +220,7 @@ bool CrashGenerationServer::Start() { kInBufferSize, 0, pipe_sec_attrs_); - if (!pipe_) { + if (pipe_ == INVALID_HANDLE_VALUE) { return false; } @@ -400,7 +400,8 @@ void CrashGenerationServer::HandleReadDoneState() { msg_.dump_type, msg_.thread_id, msg_.exception_pointers, - msg_.assert_info)); + msg_.assert_info, + msg_.custom_client_info)); if (!client_info->Initialize()) { server_state_ = IPC_SERVER_STATE_DISCONNECTING; @@ -726,6 +727,7 @@ void CALLBACK CrashGenerationServer::OnPipeConnected(void* context, BOOLEAN) { void CALLBACK CrashGenerationServer::OnDumpRequest(void* context, BOOLEAN) { assert(context); ClientInfo* client_info = reinterpret_cast(context); + client_info->PopulateCustomInfo(); CrashGenerationServer* crash_server = client_info->crash_server(); assert(crash_server); diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc index fc678a91..f6469d78 100644 --- a/src/client/windows/handler/exception_handler.cc +++ b/src/client/windows/handler/exception_handler.cc @@ -34,6 +34,7 @@ #include "common/windows/string_utils-inl.h" +#include "client/windows/common/ipc_protocol.h" #include "client/windows/handler/exception_handler.h" #include "common/windows/guid_string.h" @@ -52,14 +53,16 @@ ExceptionHandler::ExceptionHandler(const wstring& dump_path, void* callback_context, int handler_types, MINIDUMP_TYPE dump_type, - const wchar_t* pipe_name) { + const wchar_t* pipe_name, + const CustomClientInfo* custom_info) { Initialize(dump_path, filter, callback, callback_context, handler_types, dump_type, - pipe_name); + pipe_name, + custom_info); } ExceptionHandler::ExceptionHandler(const wstring &dump_path, @@ -73,6 +76,7 @@ ExceptionHandler::ExceptionHandler(const wstring &dump_path, callback_context, handler_types, MiniDumpNormal, + NULL, NULL); } @@ -82,7 +86,8 @@ void ExceptionHandler::Initialize(const wstring& dump_path, void* callback_context, int handler_types, MINIDUMP_TYPE dump_type, - const wchar_t* pipe_name) { + const wchar_t* pipe_name, + const CustomClientInfo* custom_info) { filter_ = filter; callback_ = callback; callback_context_ = callback_context; @@ -112,7 +117,9 @@ void ExceptionHandler::Initialize(const wstring& dump_path, // Attempt to use out-of-process if user has specified pipe name. if (pipe_name != NULL) { scoped_ptr client( - new CrashGenerationClient(pipe_name, dump_type_)); + new CrashGenerationClient(pipe_name, + dump_type_, + custom_info)); // If successful in registering with the monitoring process, // there is no need to setup in-process crash generation. diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h index 870025fb..d02f7080 100644 --- a/src/client/windows/handler/exception_handler.h +++ b/src/client/windows/handler/exception_handler.h @@ -68,6 +68,7 @@ #include #include +#include "client/windows/common/ipc_protocol.h" #include "client/windows/crash_generation/crash_generation_client.h" #include "google_breakpad/common/minidump_format.h" #include "processor/scoped_ptr.h" @@ -163,7 +164,8 @@ class ExceptionHandler { void* callback_context, int handler_types, MINIDUMP_TYPE dump_type, - const wchar_t* pipe_name); + const wchar_t* pipe_name, + const CustomClientInfo* custom_info); ~ExceptionHandler(); @@ -213,7 +215,8 @@ class ExceptionHandler { void* callback_context, int handler_types, MINIDUMP_TYPE dump_type, - const wchar_t* pipe_name); + const wchar_t* pipe_name, + const CustomClientInfo* custom_info); // Function pointer type for MiniDumpWriteDump, which is looked up // dynamically. diff --git a/src/client/windows/tests/crash_generation_app/crash_generation_app.cc b/src/client/windows/tests/crash_generation_app/crash_generation_app.cc index 924d734a..13d86216 100644 --- a/src/client/windows/tests/crash_generation_app/crash_generation_app.cc +++ b/src/client/windows/tests/crash_generation_app/crash_generation_app.cc @@ -35,7 +35,7 @@ namespace google_breakpad { const int kMaxLoadString = 100; -const wchar_t kPipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\S-1-5-21-39260824-743453154-142223018-195347"; +const wchar_t kPipeName[] = L"\\\\.\\pipe\\GoogleCrashServices-Testing"; const DWORD kEditBoxStyles = WS_CHILD | WS_VISIBLE | @@ -63,6 +63,13 @@ BOOL InitInstance(HINSTANCE, int); LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM); INT_PTR CALLBACK About(HWND, UINT, WPARAM, LPARAM); +static int k_custom_info_count = 2; +static CustomInfoEntry k_custom_info_entries[] = + { + CustomInfoEntry(L"prod", L"CrashTestApp"), + CustomInfoEntry(L"ver", L"1.0"), + }; + static ExceptionHandler* handler = NULL; static CrashGenerationServer* crash_server = NULL; @@ -211,6 +218,33 @@ static void _cdecl ShowClientCrashed(void* context, } QueueUserWorkItem(AppendTextWorker, line, WT_EXECUTEDEFAULT); + + const CustomInfoEntry* custom_info = NULL; + int custom_info_count = client_info->GetCustomInfo(&custom_info); + if (custom_info_count <= 0) { + return; + } + + wstring str_line; + for (int i = 0; i < custom_info_count; ++i) { + if (i > 0) { + str_line += L", "; + } + str_line += custom_info[i].name; + str_line += L": "; + str_line += custom_info[i].value; + } + + line = new TCHAR[kMaximumLineLength]; + result = swprintf_s(line, + kMaximumLineLength, + L"%s\n", + str_line.c_str()); + if (result == -1) { + delete[] line; + return; + } + QueueUserWorkItem(AppendTextWorker, line, WT_EXECUTEDEFAULT); } static void _cdecl ShowClientExited(void* context, @@ -276,6 +310,7 @@ void RequestDump() { if (!handler->WriteMinidump()) { MessageBoxW(NULL, L"Dump request failed", L"Dumper", MB_OK); } + k_custom_info_entries[1].set_value(L"1.1"); } void CleanUp() { @@ -427,6 +462,8 @@ int APIENTRY _tWinMain(HINSTANCE instance, cs_edit = new CRITICAL_SECTION(); InitializeCriticalSection(cs_edit); + CustomClientInfo custom_info = {k_custom_info_entries, k_custom_info_count}; + // This is needed for CRT to not show dialog for invalid param // failures and instead let the code handle it. _CrtSetReportMode(_CRT_ASSERT, 0); @@ -436,7 +473,8 @@ int APIENTRY _tWinMain(HINSTANCE instance, NULL, ExceptionHandler::HANDLER_ALL, MiniDumpNormal, - kPipeName); + kPipeName, + &custom_info); // Initialize global strings. LoadString(instance, IDS_APP_TITLE, title, kMaxLoadString); diff --git a/src/client/windows/tests/crash_generation_app/precompile.h b/src/client/windows/tests/crash_generation_app/precompile.h index 7dab1281..1ade4451 100644 --- a/src/client/windows/tests/crash_generation_app/precompile.h +++ b/src/client/windows/tests/crash_generation_app/precompile.h @@ -77,6 +77,7 @@ #include #include +#include "client/windows/common/ipc_protocol.h" #include "client/windows/crash_generation/client_info.h" #include "client/windows/crash_generation/crash_generation_client.h" #include "client/windows/crash_generation/crash_generation_server.h"