Fix CrashGenerationServer to recover from protocol errors and a test for same.

src/client/windows/crash_generation/crash_generation_server.cc

 // Copyright (c) 2008, Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 // notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
@@ skipping 16 matching lines @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include "client/windows/crash_generation/crash_generation_server.h"
 #include <windows.h>
 #include <cassert>
 #include <list>
 #include "client/windows/common/auto_critical_section.h"
 #include "processor/scoped_ptr.h"
 
+#include "client/windows/crash_generation/client_info.h"

[Erik Wright, 2010/09/16 21:06:59]

Changed to a forward-decl in crash_generation_server.h.

+
 namespace google_breakpad {
 
+// How long to wait for the server thread to begin listening or die trying
+static const DWORD kMaxStartDelayMs = INFINITE;
+
 // Output buffer size.
 static const size_t kOutBufferSize = 64;
 
 // Input buffer size.
 static const size_t kInBufferSize = 64;
 
 // Access flags for the client on the dump request event.
 static const DWORD kDumpRequestEventAccess = EVENT_MODIFY_STATE;
 
 // Access flags for the client on the dump generated event.
@@ skipping 61 matching lines @@
       dump_callback_(dump_callback),
       dump_context_(dump_context),
       exit_callback_(exit_callback),
       exit_context_(exit_context),
       generate_dumps_(generate_dumps),
       dump_generator_(NULL),
       server_state_(IPC_SERVER_STATE_INITIAL),
       shutting_down_(false),
       overlapped_(),
       client_info_(NULL),
-      cleanup_item_count_(0) {
+      cleanup_item_count_(0),
+      server_start_complete_event_(NULL) {
   InitializeCriticalSection(&clients_sync_);
 
   if (dump_path) {
     dump_generator_.reset(new MinidumpGenerator(*dump_path));
   }
 }
 
 CrashGenerationServer::~CrashGenerationServer() {
   // Indicate to existing threads that server is shutting down.
   shutting_down_ = true;
@@ skipping 62 matching lines @@
   if (server_alive_handle_) {
     // Release the mutex before closing the handle so that clients requesting
     // dumps wait for a long time for the server to generate a dump.
     ReleaseMutex(server_alive_handle_);
     CloseHandle(server_alive_handle_);
   }
 
   DeleteCriticalSection(&clients_sync_);
 }
 
 bool CrashGenerationServer::Start() {

[Erik Wright, 2010/09/16 21:06:59]

A non-negligible change in this function's behaviour, but probably meets what
clients previously expected. When it returns success, the server is started and
listening (clients may connect). Previously there was a theoretical gap, and a
possibility that it would fail after reporting success.

The actual work that is done in the worker thread is trivial (and only there,
IMO, for convenience/style), so in practice the return is probably not really
delayed and the client probably expected these things to be done before Start
returns.

Also, this way the tests don't need a sleep/poll thing for checking when the
server is started.

   server_state_ = IPC_SERVER_STATE_INITIAL;
 
+  server_start_complete_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
+  if (!server_start_complete_event_) {
+    return false;
+  }
+
   server_alive_handle_ = CreateMutex(NULL, TRUE, NULL);
   if (!server_alive_handle_) {
     return false;
   }
 
   // Event to signal the client connection and pipe reads and writes.
   overlapped_.hEvent = CreateEvent(NULL,   // Security descriptor.
                                    TRUE,   // Manual reset.
                                    FALSE,  // Initially signaled.
                                    NULL);  // Name.
@@ skipping 18 matching lines @@
                           kOutBufferSize,
                           kInBufferSize,
                           0,
                           pipe_sec_attrs_);
   if (pipe_ == INVALID_HANDLE_VALUE) {
     return false;
   }
 
   // Signal the event to start a separate thread to handle
   // client connections.
-  return SetEvent(overlapped_.hEvent) != FALSE;
+  if (SetEvent(overlapped_.hEvent) == FALSE) {
+    return false;
+  }
+
+  if (WaitForSingleObject(server_start_complete_event_, kMaxStartDelayMs) !=

[Erik Wright, 2010/09/16 21:06:59]

I don't think it is possible for the worker thread to not complete, but using
anything less than INFINITE could result in hard-to-repro failures under stress.
Hence kMaxStartDelayMs is INFINITE...

+      WAIT_OBJECT_0) {
+    return false;
+  }
+
+  // If we are in error state, it's probably because we failed to start,
+  // though it could theoretically be due to a failure between succesful start
+  // and our return from the above wait.
+  return server_state_ != IPC_SERVER_STATE_ERROR;
 }
 
 // If the server thread serving clients ever gets into the
 // ERROR state, reset the event, close the pipe and remain
 // in the error state forever. Error state means something
 // that we didn't account for has happened, and it's dangerous
 // to do anything unknowingly.
 void CrashGenerationServer::HandleErrorState() {
   assert(server_state_ == IPC_SERVER_STATE_ERROR);
 
@@ skipping 21 matching lines @@
 
 // When the server thread serving clients is in the INITIAL state,
 // try to connect to the pipe asynchronously. If the connection
 // finishes synchronously, directly go into the CONNECTED state;
 // otherwise go into the CONNECTING state. For any problems, go
 // into the ERROR state.
 void CrashGenerationServer::HandleInitialState() {
   assert(server_state_ == IPC_SERVER_STATE_INITIAL);
 
   if (!ResetEvent(overlapped_.hEvent)) {
-    server_state_ = IPC_SERVER_STATE_ERROR;
-    return;
+    EnterErrorState();
+  } else {
+    bool success = ConnectNamedPipe(pipe_, &overlapped_) != FALSE;
+
+    // From MSDN, it is not clear that when ConnectNamedPipe is used
+    // in an overlapped mode, will it ever return non-zero value, and
+    // if so, in what cases.
+    assert(!success);
+
+    DWORD error_code = GetLastError();
+    switch (error_code) {
+      case ERROR_IO_PENDING:
+        EnterStateWhenSignaled(IPC_SERVER_STATE_CONNECTING);
+        break;
+
+      case ERROR_PIPE_CONNECTED:
+        EnterStateImmediately(IPC_SERVER_STATE_CONNECTED);
+        break;
+
+      default:
+        EnterErrorState();
+        break;
+    }
   }
 
-  bool success = ConnectNamedPipe(pipe_, &overlapped_) != FALSE;
-
-  // From MSDN, it is not clear that when ConnectNamedPipe is used
-  // in an overlapped mode, will it ever return non-zero value, and
-  // if so, in what cases.
-  assert(!success);
-
-  DWORD error_code = GetLastError();
-  switch (error_code) {
-    case ERROR_IO_PENDING:
-      server_state_ = IPC_SERVER_STATE_CONNECTING;
-      break;
-
-    case ERROR_PIPE_CONNECTED:
-      if (SetEvent(overlapped_.hEvent)) {
-        server_state_ = IPC_SERVER_STATE_CONNECTED;
-      } else {
-        server_state_ = IPC_SERVER_STATE_ERROR;
-      }
-      break;
-
-    default:
-      server_state_ = IPC_SERVER_STATE_ERROR;
-      break;
+  if (!SetEvent(server_start_complete_event_)) {
+    EnterErrorState();
   }
 }
 
 // When the server thread serving the clients is in the CONNECTING state,
 // try to get the result of the asynchronous connection request using
 // the OVERLAPPED object. If the result indicates the connection is done,
 // go into the CONNECTED state. If the result indicates I/O is still
 // INCOMPLETE, remain in the CONNECTING state. For any problems,
 // go into the DISCONNECTING state.
 void CrashGenerationServer::HandleConnectingState() {
   assert(server_state_ == IPC_SERVER_STATE_CONNECTING);
 
   DWORD bytes_count = 0;
   bool success = GetOverlappedResult(pipe_,
                                      &overlapped_,
                                      &bytes_count,
                                      FALSE) != FALSE;
 
   if (success) {
-    server_state_ = IPC_SERVER_STATE_CONNECTED;
-    return;
-  }
-
-  if (GetLastError() != ERROR_IO_INCOMPLETE) {
-    server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+    EnterStateImmediately(IPC_SERVER_STATE_CONNECTED);
+  } else if (GetLastError() != ERROR_IO_INCOMPLETE) {
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
   }
 }
 
 // When the server thread serving the clients is in the CONNECTED state,
 // try to issue an asynchronous read from the pipe. If read completes
 // synchronously or if I/O is pending then go into the READING state.
 // For any problems, go into the DISCONNECTING state.
 void CrashGenerationServer::HandleConnectedState() {
   assert(server_state_ == IPC_SERVER_STATE_CONNECTED);
 
   DWORD bytes_count = 0;
   memset(&msg_, 0, sizeof(msg_));
   bool success = ReadFile(pipe_,
                           &msg_,
                           sizeof(msg_),
                           &bytes_count,
                           &overlapped_) != FALSE;
 
   // Note that the asynchronous read issued above can finish before the
   // code below executes. But, it is okay to change state after issuing
   // the asynchronous read. This is because even if the asynchronous read
   // is done, the callback for it would not be executed until the current
   // thread finishes its execution.
   if (success || GetLastError() == ERROR_IO_PENDING) {
-    server_state_ = IPC_SERVER_STATE_READING;
+    EnterStateWhenSignaled(IPC_SERVER_STATE_READING);
   } else {
-    server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
   }
 }
 
 // When the server thread serving the clients is in the READING state,
 // try to get the result of the async read. If async read is done,
 // go into the READ_DONE state. For any problems, go into the
 // DISCONNECTING state.
 void CrashGenerationServer::HandleReadingState() {
   assert(server_state_ == IPC_SERVER_STATE_READING);
 
   DWORD bytes_count = 0;
   bool success = GetOverlappedResult(pipe_,
                                      &overlapped_,
                                      &bytes_count,
                                      FALSE) != FALSE;
 
   if (success && bytes_count == sizeof(ProtocolMessage)) {
-    server_state_ = IPC_SERVER_STATE_READ_DONE;
-    return;
+    EnterStateImmediately(IPC_SERVER_STATE_READ_DONE);
+  } else {
+    // We should never get an I/O incomplete since we should not execute this
+    // unless the Read has finished and the overlapped event is signaled. If
+    // we do get INCOMPLETE, we have a bug in our code.
+    assert(GetLastError() != ERROR_IO_INCOMPLETE);
+
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
   }
-
-  DWORD error_code;
-  error_code = GetLastError();
-
-  // We should never get an I/O incomplete since we should not execute this
-  // unless the Read has finished and the overlapped event is signaled. If
-  // we do get INCOMPLETE, we have a bug in our code.
-  assert(error_code != ERROR_IO_INCOMPLETE);
-
-  server_state_ = IPC_SERVER_STATE_DISCONNECTING;
 }
 
 // When the server thread serving the client is in the READ_DONE state,
 // validate the client's request message, register the client by
 // creating appropriate objects and prepare the response.  Then try to
 // write the response to the pipe asynchronously. If that succeeds,
 // go into the WRITING state. For any problems, go into the DISCONNECTING
 // state.
 void CrashGenerationServer::HandleReadDoneState() {
   assert(server_state_ == IPC_SERVER_STATE_READ_DONE);
 
   if (!IsClientRequestValid(msg_)) {
-    server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
     return;
   }
 
   scoped_ptr<ClientInfo> client_info(
       new ClientInfo(this,
                      msg_.pid,
                      msg_.dump_type,
                      msg_.thread_id,
                      msg_.exception_pointers,
                      msg_.assert_info,
                      msg_.custom_client_info));
 
   if (!client_info->Initialize()) {
-    server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
     return;
   }
 
+  // Issues an asynchronous WriteFile call if successful.
+  // Iff successful, assigns ownership of the client_info pointer to the server
+  // instance, in which case we must be sure not to free it in this function.
   if (!RespondToClient(client_info.get())) {
-    server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
     return;
   }
 
+  client_info_ = client_info.release();
+
   // Note that the asynchronous write issued by RespondToClient function
   // can finish before  the code below executes. But it is okay to change
   // state after issuing the asynchronous write. This is because even if
   // the asynchronous write is done, the callback for it would not be
   // executed until the current thread finishes its execution.
-  server_state_ = IPC_SERVER_STATE_WRITING;
-  client_info_ = client_info.release();
+  EnterStateWhenSignaled(IPC_SERVER_STATE_WRITING);
 }
 
 // When the server thread serving the clients is in the WRITING state,
 // try to get the result of the async write. If the async write is done,
 // go into the WRITE_DONE state. For any problems, go into the
 // DISONNECTING state.
 void CrashGenerationServer::HandleWritingState() {
   assert(server_state_ == IPC_SERVER_STATE_WRITING);
 
   DWORD bytes_count = 0;
   bool success = GetOverlappedResult(pipe_,
                                      &overlapped_,
                                      &bytes_count,
                                      FALSE) != FALSE;
 
   if (success) {
-    server_state_ = IPC_SERVER_STATE_WRITE_DONE;
+    EnterStateImmediately(IPC_SERVER_STATE_WRITE_DONE);
     return;
   }
 
-  DWORD error_code;
-  error_code = GetLastError();
-
   // We should never get an I/O incomplete since we should not execute this
   // unless the Write has finished and the overlapped event is signaled. If
   // we do get INCOMPLETE, we have a bug in our code.
-  assert(error_code != ERROR_IO_INCOMPLETE);
+  assert(GetLastError() != ERROR_IO_INCOMPLETE);
 
-  server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+  EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
 }
 
 // When the server thread serving the clients is in the WRITE_DONE state,
 // try to issue an async read on the pipe. If the read completes synchronously
 // or if I/O is still pending then go into the READING_ACK state. For any
 // issues, go into the DISCONNECTING state.
 void CrashGenerationServer::HandleWriteDoneState() {
   assert(server_state_ == IPC_SERVER_STATE_WRITE_DONE);
 
-  server_state_ = IPC_SERVER_STATE_READING_ACK;
-
   DWORD bytes_count = 0;
   bool success = ReadFile(pipe_,
                           &msg_,
                           sizeof(msg_),
                           &bytes_count,
                           &overlapped_) != FALSE;
 
   if (success) {
-    return;
-  }
-
-  DWORD error_code = GetLastError();
-
-  if (error_code != ERROR_IO_PENDING) {
-    server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+    EnterStateImmediately(IPC_SERVER_STATE_READING_ACK);
+  } else if (GetLastError() == ERROR_IO_PENDING) {
+    EnterStateWhenSignaled(IPC_SERVER_STATE_READING_ACK);
+  } else {
+    EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
   }
 }
 
 // When the server thread serving the clients is in the READING_ACK state,
 // try to get result of async read. Go into the DISCONNECTING state.
 void CrashGenerationServer::HandleReadingAckState() {
   assert(server_state_ == IPC_SERVER_STATE_READING_ACK);
 
   DWORD bytes_count = 0;
   bool success = GetOverlappedResult(pipe_,
                                      &overlapped_,
                                      &bytes_count,
                                      FALSE) != FALSE;
 
   if (success) {
     // The connection handshake with the client is now complete; perform
     // the callback.
     if (connect_callback_) {
       connect_callback_(connect_context_, client_info_);
     }
   } else {
-    DWORD error_code = GetLastError();
-
     // We should never get an I/O incomplete since we should not execute this
     // unless the Read has finished and the overlapped event is signaled. If
     // we do get INCOMPLETE, we have a bug in our code.
-    assert(error_code != ERROR_IO_INCOMPLETE);
+    assert(GetLastError() != ERROR_IO_INCOMPLETE);
   }
 
-  server_state_ = IPC_SERVER_STATE_DISCONNECTING;
+  EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING);
 }
 
 // When the server thread serving the client is in the DISCONNECTING state,
 // disconnect from the pipe and reset the event. If anything fails, go into
 // the ERROR state. If it goes well, go into the INITIAL state and set the
 // event to start all over again.
 void CrashGenerationServer::HandleDisconnectingState() {
   assert(server_state_ == IPC_SERVER_STATE_DISCONNECTING);
 
   // Done serving the client.
   client_info_ = NULL;
 
   overlapped_.Internal = NULL;
   overlapped_.InternalHigh = NULL;
   overlapped_.Offset = 0;
   overlapped_.OffsetHigh = 0;
   overlapped_.Pointer = NULL;
 
   if (!ResetEvent(overlapped_.hEvent)) {
-    server_state_ = IPC_SERVER_STATE_ERROR;
+    EnterErrorState();
     return;
   }
 
   if (!DisconnectNamedPipe(pipe_)) {
-    server_state_ = IPC_SERVER_STATE_ERROR;
+    EnterErrorState();
     return;
   }
 
   // If the server is shutting down do not connect to the
   // next client.
   if (shutting_down_) {
     return;
   }
 
-  server_state_ = IPC_SERVER_STATE_INITIAL;
+  EnterStateImmediately(IPC_SERVER_STATE_INITIAL);
+}
+
+void CrashGenerationServer::EnterErrorState() {

[Erik Wright, 2010/09/16 21:06:59]

May these functions aid in readability, and prevent the accidental forgetting of
SetEvent where it is needed for immediate transition to next state.

NB that in some cases, it is true, the event is already signalled by the
overlapped IO stuff. But it doesn't hurt to signal it redundantly and it was
clearly easy for previous authors to not remember where it was required.

+  SetEvent(overlapped_.hEvent);
+  server_state_ = IPC_SERVER_STATE_ERROR;
+}
+
+void CrashGenerationServer::EnterStateWhenSignaled(IPCServerState state) {
+  server_state_ = state;
+}
+
+void CrashGenerationServer::EnterStateImmediately(IPCServerState state) {
+  server_state_ = state;
+
   if (!SetEvent(overlapped_.hEvent)) {
     server_state_ = IPC_SERVER_STATE_ERROR;
   }
 }
 
 bool CrashGenerationServer::PrepareReply(const ClientInfo& client_info,
                                          ProtocolMessage* reply) const {
   reply->tag = MESSAGE_TAG_REGISTRATION_RESPONSE;
   reply->pid = GetCurrentProcessId();
 
@@ skipping 45 matching lines @@
                        &reply->server_alive_handle,
                        kMutexAccess,
                        FALSE,
                        0)) {
     return false;
   }
 
   return true;
 }
 
 bool CrashGenerationServer::RespondToClient(ClientInfo* client_info) {

[Erik Wright, 2010/09/16 21:06:59]

There was a bug in this function where it could take ownership of the pointer
but then return failure. The client function then freed the pointer.

   ProtocolMessage reply;
   if (!PrepareReply(*client_info, &reply)) {
     return false;
   }
 
-  if (!AddClient(client_info)) {
-    return false;
-  }
-
   DWORD bytes_count = 0;
   bool success = WriteFile(pipe_,
                            &reply,
                            sizeof(reply),
                            &bytes_count,
                            &overlapped_) != FALSE;
 
-  return success || GetLastError() == ERROR_IO_PENDING;
+  if (!success && GetLastError() != ERROR_IO_PENDING) {
+    return false;
+  }
+
+  // Takes over ownership of client_info. We MUST return true if AddClient
+  // succeeds.
+  if (!AddClient(client_info)) {
+    return false;
+  }
+
+  return true;
 }
 
 // The server thread servicing the clients runs this method. The method
 // implements the state machine described in ReadMe.txt along with the
 // helper methods HandleXXXState.
 void CrashGenerationServer::HandleConnectionRequest() {
   // If we are shutting doen then get into ERROR state, reset the event so more
   // workers don't run and return immediately.
   if (shutting_down_) {
     server_state_ = IPC_SERVER_STATE_ERROR;
@@ skipping 81 matching lines @@
   {
     AutoCriticalSection lock(&clients_sync_);
     clients_.push_back(client_info);
   }
 
   return true;
 }
 
 // static
 void CALLBACK CrashGenerationServer::OnPipeConnected(void* context, BOOLEAN) {
-  assert (context);
+  assert(context);
 
   CrashGenerationServer* obj =
       reinterpret_cast<CrashGenerationServer*>(context);
   obj->HandleConnectionRequest();
 }
 
 // static
 void CALLBACK CrashGenerationServer::OnDumpRequest(void* context, BOOLEAN) {
   assert(context);
   ClientInfo* client_info = reinterpret_cast<ClientInfo*>(context);
@@ skipping 92 matching lines @@
                                         client_thread_id,
                                         GetCurrentThreadId(),
                                         client_ex_info,
                                         client.assert_info(),
                                         client.dump_type(),
                                         true,
                                         dump_path);
 }
 
 }  // namespace google_breakpad