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

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 <Windows.h>
 #include <DbgHelp.h>
+#include <string>
+#include <utility>
 #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 <Windows.h>
 #include <DbgHelp.h>
+#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<CustomInfoEntry> 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 <cassert>
+#include <utility>
 #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 <windows.h>
 #include <dbghelp.h>
 #include <string>
+#include <utility>
 #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<ClientInfo*>(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<CrashGenerationClient> 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 <string>
 #include <vector>
 
+#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 <cassert>
 #include <list>
 
+#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"