Sandbox: remove raw handles from SharedMemIPCServer::ServerControl

sandbox/win/src/sharedmem_ipc_server.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/stl_util.h"
 #include "sandbox/win/src/sharedmem_ipc_server.h"
 #include "sandbox/win/src/sharedmem_ipc_client.h"
 #include "sandbox/win/src/sandbox.h"
 #include "sandbox/win/src/sandbox_types.h"
 #include "sandbox/win/src/crosscall_params.h"
 #include "sandbox/win/src/crosscall_server.h"
 
 namespace {
 // This handle must not be closed.
 volatile HANDLE g_alive_mutex = NULL;
@@ skipping 26 matching lines @@
     }
   }
 }
 
 SharedMemIPCServer::~SharedMemIPCServer() {
   // Free the wait handles associated with the thread pool.
   if (!thread_provider_->UnRegisterWaits(this)) {
     // Better to leak than to crash.
     return;
   }
-  // Free the IPC signal events.
-  ServerContexts::iterator it;
-  for (it = server_contexts_.begin(); it != server_contexts_.end(); ++it) {
-    ServerControl* context = (*it);
-    ::CloseHandle(context->ping_event);
-    ::CloseHandle(context->pong_event);
-    delete context;
-  }
+  STLDeleteElements(&server_contexts_);
 
   if (client_control_)
     ::UnmapViewOfFile(client_control_);
 }
 
 bool SharedMemIPCServer::Init(void* shared_mem, uint32 shared_size,
                               uint32 channel_size) {
   // The shared memory needs to be at least as big as a channel.
   if (shared_size < channel_size) {
     return false;
@@ skipping 47 matching lines @@
     service_context->channel = client_context;
     service_context->channel_buffer = service_context->shared_base +
                                       client_context->channel_base;
     service_context->dispatcher = call_dispatcher_;
     service_context->target_info.process = target_process_;
     service_context->target_info.process_id = target_process_id_;
     service_context->target_info.job_object = target_job_object_;
     // Advance to the next channel.
     base_start += channel_size;
     // Register the ping event with the threadpool.
-    thread_provider_->RegisterWait(this, service_context->ping_event,
+    thread_provider_->RegisterWait(this, service_context->ping_event.Get(),
                                    ThreadPingEventReady, service_context);
   }
   if (!::DuplicateHandle(::GetCurrentProcess(), g_alive_mutex,
                          target_process_, &client_control_->server_alive,
                          SYNCHRONIZE | EVENT_MODIFY_STATE, FALSE, 0)) {
     return false;
   }
   // This last setting indicates to the client all is setup.
   client_control_->channels_count = channel_count;
   return true;
@@ skipping 250 matching lines @@
   CrossCallReturn call_result = {0};
   void* buffer = service_context->channel_buffer;
 
   InvokeCallback(service_context, buffer, &call_result);
 
   // Copy the answer back into the channel and signal the pong event. This
   // should wake up the client so he can finish the the ipc cycle.
   CrossCallParams* call_params = reinterpret_cast<CrossCallParams*>(buffer);
   memcpy(call_params->GetCallReturn(), &call_result, sizeof(call_result));
   ::InterlockedExchange(&service_context->channel->state, kAckChannel);
-  ::SetEvent(service_context->pong_event);
+  ::SetEvent(service_context->pong_event.Get());
 }

[cpu_(ooo_6.6-7.5), 2015/07/09 16:51:53]

maybe check() the return of set event?

[rvargas (doing something else), 2015/07/09 18:14:44]

I'd prefer to keep this CL strictly as a refactor without functionality changes.

I can add the check on another CL if you want me to. Are you thinking that this
should only fail if the handle is being messed with, hence we should crash?

 
-bool SharedMemIPCServer::MakeEvents(HANDLE* server_ping, HANDLE* server_pong,
+bool SharedMemIPCServer::MakeEvents(base::win::ScopedHandle* server_ping,
+                                    base::win::ScopedHandle* server_pong,
                                     HANDLE* client_ping, HANDLE* client_pong) {
   // Note that the IPC client has no right to delete the events. That would
   // cause problems. The server *owns* the events.
   const DWORD kDesiredAccess = SYNCHRONIZE | EVENT_MODIFY_STATE;
 
   // The events are auto reset, and start not signaled.
-  *server_ping = ::CreateEventW(NULL, FALSE, FALSE, NULL);
-  if (!::DuplicateHandle(::GetCurrentProcess(), *server_ping, target_process_,
-                         client_ping, kDesiredAccess, FALSE, 0)) {
+  server_ping->Set(::CreateEventW(NULL, FALSE, FALSE, NULL));
+  if (!::DuplicateHandle(::GetCurrentProcess(), server_ping->Get(),
+                         target_process_, client_ping, kDesiredAccess, FALSE,
+                         0)) {
     return false;
   }
-  *server_pong = ::CreateEventW(NULL, FALSE, FALSE, NULL);
-  if (!::DuplicateHandle(::GetCurrentProcess(), *server_pong, target_process_,
-                         client_pong, kDesiredAccess, FALSE, 0)) {
+
+  server_pong->Set(::CreateEventW(NULL, FALSE, FALSE, NULL));
+  if (!::DuplicateHandle(::GetCurrentProcess(), server_pong->Get(),
+                         target_process_, client_pong, kDesiredAccess, FALSE,
+                         0)) {
     return false;
   }
   return true;
 }
 
 }  // namespace sandbox