From b6ee7dcb220db5c396d12ddf12e071c8ec48dfd3 Mon Sep 17 00:00:00 2001 From: "erikwright@chromium.org" Date: Mon, 20 Sep 2010 21:35:24 +0000 Subject: Fix CrashGenerationServer to recover from protocol errors and a test for same. R=siggi at http://breakpad.appspot.com/196001/show git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@695 4c0a9323-5329-0410-9bdc-e9ce6186880e --- .../crash_generation/crash_generation_server.cc | 170 ++++++++++++--------- .../crash_generation/crash_generation_server.h | 23 ++- 2 files changed, 119 insertions(+), 74 deletions(-) (limited to 'src/client/windows/crash_generation') diff --git a/src/client/windows/crash_generation/crash_generation_server.cc b/src/client/windows/crash_generation/crash_generation_server.cc index dafa5c2a..61af1b2d 100644 --- a/src/client/windows/crash_generation/crash_generation_server.cc +++ b/src/client/windows/crash_generation/crash_generation_server.cc @@ -34,6 +34,8 @@ #include "client/windows/common/auto_critical_section.h" #include "processor/scoped_ptr.h" +#include "client/windows/crash_generation/client_info.h" + namespace google_breakpad { // Output buffer size. @@ -113,7 +115,7 @@ CrashGenerationServer::CrashGenerationServer( exit_context_(exit_context), generate_dumps_(generate_dumps), dump_generator_(NULL), - server_state_(IPC_SERVER_STATE_INITIAL), + server_state_(IPC_SERVER_STATE_UNINITIALIZED), shutting_down_(false), overlapped_(), client_info_(NULL), @@ -197,10 +199,18 @@ CrashGenerationServer::~CrashGenerationServer() { CloseHandle(server_alive_handle_); } + if (overlapped_.hEvent) { + CloseHandle(overlapped_.hEvent); + } + DeleteCriticalSection(&clients_sync_); } bool CrashGenerationServer::Start() { + if (server_state_ != IPC_SERVER_STATE_UNINITIALIZED) { + return false; + } + server_state_ = IPC_SERVER_STATE_INITIAL; server_alive_handle_ = CreateMutex(NULL, TRUE, NULL); @@ -239,9 +249,12 @@ bool CrashGenerationServer::Start() { return false; } - // Signal the event to start a separate thread to handle - // client connections. - return SetEvent(overlapped_.hEvent) != FALSE; + // Kick-start the state machine. This will initiate an asynchronous wait + // for client connections. + HandleInitialState(); + + // If we are in error state, it's because we failed to start listening. + return server_state_ != IPC_SERVER_STATE_ERROR; } // If the server thread serving clients ever gets into the @@ -283,33 +296,29 @@ void CrashGenerationServer::HandleInitialState() { assert(server_state_ == IPC_SERVER_STATE_INITIAL); if (!ResetEvent(overlapped_.hEvent)) { - server_state_ = IPC_SERVER_STATE_ERROR; + EnterErrorState(); return; } bool success = ConnectNamedPipe(pipe_, &overlapped_) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); // 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; + EnterStateWhenSignaled(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; - } + EnterStateImmediately(IPC_SERVER_STATE_CONNECTED); break; default: - server_state_ = IPC_SERVER_STATE_ERROR; + EnterErrorState(); break; } } @@ -328,14 +337,14 @@ void CrashGenerationServer::HandleConnectingState() { &overlapped_, &bytes_count, FALSE) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); 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 (error_code != ERROR_IO_INCOMPLETE) { + EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING); + } else { + // remain in CONNECTING state } } @@ -353,16 +362,17 @@ void CrashGenerationServer::HandleConnectedState() { sizeof(msg_), &bytes_count, &overlapped_) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); // 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; + if (success || error_code == ERROR_IO_PENDING) { + EnterStateWhenSignaled(IPC_SERVER_STATE_READING); } else { - server_state_ = IPC_SERVER_STATE_DISCONNECTING; + EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING); } } @@ -378,21 +388,18 @@ void CrashGenerationServer::HandleReadingState() { &overlapped_, &bytes_count, FALSE) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); if (success && bytes_count == sizeof(ProtocolMessage)) { - server_state_ = IPC_SERVER_STATE_READ_DONE; - return; - } - - 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); + 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(error_code != ERROR_IO_INCOMPLETE); - server_state_ = IPC_SERVER_STATE_DISCONNECTING; + EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING); + } } // When the server thread serving the client is in the READ_DONE state, @@ -405,7 +412,7 @@ 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; } @@ -419,22 +426,26 @@ void CrashGenerationServer::HandleReadDoneState() { 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, @@ -449,21 +460,19 @@ void CrashGenerationServer::HandleWritingState() { &overlapped_, &bytes_count, FALSE) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); 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); - server_state_ = IPC_SERVER_STATE_DISCONNECTING; + EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING); } // When the server thread serving the clients is in the WRITE_DONE state, @@ -473,23 +482,20 @@ void CrashGenerationServer::HandleWritingState() { 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; + &msg_, + sizeof(msg_), + &bytes_count, + &overlapped_) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); 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 (error_code == ERROR_IO_PENDING) { + EnterStateWhenSignaled(IPC_SERVER_STATE_READING_ACK); + } else { + EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING); } } @@ -503,6 +509,7 @@ void CrashGenerationServer::HandleReadingAckState() { &overlapped_, &bytes_count, FALSE) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); if (success) { // The connection handshake with the client is now complete; perform @@ -511,15 +518,13 @@ void CrashGenerationServer::HandleReadingAckState() { 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); } - server_state_ = IPC_SERVER_STATE_DISCONNECTING; + EnterStateImmediately(IPC_SERVER_STATE_DISCONNECTING); } // When the server thread serving the client is in the DISCONNECTING state, @@ -539,12 +544,12 @@ void CrashGenerationServer::HandleDisconnectingState() { 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; } @@ -554,7 +559,21 @@ void CrashGenerationServer::HandleDisconnectingState() { return; } - server_state_ = IPC_SERVER_STATE_INITIAL; + EnterStateImmediately(IPC_SERVER_STATE_INITIAL); +} + +void CrashGenerationServer::EnterErrorState() { + 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; } @@ -626,18 +645,25 @@ bool CrashGenerationServer::RespondToClient(ClientInfo* client_info) { return false; } - if (!AddClient(client_info)) { + DWORD bytes_count = 0; + bool success = WriteFile(pipe_, + &reply, + sizeof(reply), + &bytes_count, + &overlapped_) != FALSE; + DWORD error_code = success ? ERROR_SUCCESS : GetLastError(); + + if (!success && error_code != ERROR_IO_PENDING) { return false; } - DWORD bytes_count = 0; - bool success = WriteFile(pipe_, - &reply, - sizeof(reply), - &bytes_count, - &overlapped_) != FALSE; + // Takes over ownership of client_info. We MUST return true if AddClient + // succeeds. + if (!AddClient(client_info)) { + return false; + } - return success || GetLastError() == ERROR_IO_PENDING; + return true; } // The server thread servicing the clients runs this method. The method @@ -739,7 +765,7 @@ bool CrashGenerationServer::AddClient(ClientInfo* client_info) { // static void CALLBACK CrashGenerationServer::OnPipeConnected(void* context, BOOLEAN) { - assert (context); + assert(context); CrashGenerationServer* obj = reinterpret_cast(context); diff --git a/src/client/windows/crash_generation/crash_generation_server.h b/src/client/windows/crash_generation/crash_generation_server.h index cacb639a..31a353bf 100644 --- a/src/client/windows/crash_generation/crash_generation_server.h +++ b/src/client/windows/crash_generation/crash_generation_server.h @@ -33,11 +33,11 @@ #include #include #include "client/windows/common/ipc_protocol.h" -#include "client/windows/crash_generation/client_info.h" #include "client/windows/crash_generation/minidump_generator.h" #include "processor/scoped_ptr.h" namespace google_breakpad { +class ClientInfo; // Abstraction for server side implementation of out-of-process crash // generation protocol for Windows platform only. It generates Windows @@ -91,7 +91,8 @@ class CrashGenerationServer { ~CrashGenerationServer(); - // Performs initialization steps needed to start listening to clients. + // Performs initialization steps needed to start listening to clients. Upon + // successful return clients may connect to this server's pipe. // // Returns true if initialization is successful; false otherwise. bool Start(); @@ -100,6 +101,9 @@ class CrashGenerationServer { // Various states the client can be in during the handshake with // the server. enum IPCServerState { + // Server starts in this state. + IPC_SERVER_STATE_UNINITIALIZED, + // Server is in error state and it cannot serve any clients. IPC_SERVER_STATE_ERROR, @@ -192,6 +196,21 @@ class CrashGenerationServer { // Generates dump for the given client. bool GenerateDump(const ClientInfo& client, std::wstring* dump_path); + // Puts the server in a permanent error state and sets a signal such that + // the state will be immediately entered after the current state transition + // is complete. + void EnterErrorState(); + + // Puts the server in the specified state and sets a signal such that the + // state is immediately entered after the current state transition is + // complete. + void EnterStateImmediately(IPCServerState state); + + // Puts the server in the specified state. No signal will be set, so the state + // transition will only occur when signaled manually or by completion of an + // asynchronous IO operation. + void EnterStateWhenSignaled(IPCServerState state); + // Sync object for thread-safe access to the shared list of clients. CRITICAL_SECTION clients_sync_; -- cgit v1.2.1