NaCl: Pass the process handle to the broker rather than reopening it

Previously, we would open the NaCl loader's Windows process handle by
its PID in the NaCl broker (when attaching a Windows debug exception
handler).  But there is a potential race condition here: if the NaCl
loader dies, and its PID is reused, we could be opening the wrong
process.

Fix this by opening the process handle in the browser process and
passing it to the 64-bit NaCl broker.

This requires fixing a bug in ipc_message_utils.h.  This code could
cope with sending a handle from a 64-bit process to a 32-bit process
(NaCl broker to browser), but the DCHECK would fail when sending a
handle from a 32-bit process to a 64-bit process (browser to NaCl
broker).

This change is in preparation for changing NaCl's Windows debug
exception handler to attach on demand, which would allow untrusted
NaCl code to cause the NaCl process to exit before
OnDebugExceptionHandlerLaunched() runs.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2618
TEST=run_inbrowser_exception_test in nacl_integration

Review URL: https://chromiumcodereview.appspot.com/10174031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@134189 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 76 insertions, 40 deletions

diff --git a/chrome/browser/nacl_host/nacl_broker_host_win.cc b/chrome/browser/nacl_host/nacl_broker_host_win.cc
index 50f3706..d2a7aad 100644
--- a/chrome/browser/nacl_host/nacl_broker_host_win.cc
+++ b/chrome/browser/nacl_host/nacl_broker_host_win.cc
@@ -15,6 +15,7 @@
 #include "chrome/common/nacl_cmd_line.h"
 #include "chrome/common/nacl_messages.h"
 #include "content/public/browser/browser_child_process_host.h"
+#include "content/public/browser/child_process_data.h"
 #include "content/public/common/child_process_host.h"
 
 NaClBrokerHost::NaClBrokerHost() {
@@ -71,8 +72,16 @@ void NaClBrokerHost::OnLoaderLaunched(const std::string& loader_channel_id,
   NaClBrokerService::GetInstance()->OnLoaderLaunched(loader_channel_id, handle);
 }
 
-bool NaClBrokerHost::LaunchDebugExceptionHandler(int32 pid) {
-  return process_->Send(new NaClProcessMsg_LaunchDebugExceptionHandler(pid));
+bool NaClBrokerHost::LaunchDebugExceptionHandler(
+    int32 pid, base::ProcessHandle process_handle) {
+  base::ProcessHandle broker_process = process_->GetData().handle;
+  base::ProcessHandle handle_in_broker_process;
+  if (!DuplicateHandle(::GetCurrentProcess(), process_handle,
+                       broker_process, &handle_in_broker_process,
+                       0, /* bInheritHandle= */ FALSE, DUPLICATE_SAME_ACCESS))
+    return false;
+  return process_->Send(new NaClProcessMsg_LaunchDebugExceptionHandler(
+      pid, handle_in_broker_process));
 }
 
 void NaClBrokerHost::OnDebugExceptionHandlerLaunched(int32 pid) {
diff --git a/chrome/browser/nacl_host/nacl_broker_host_win.h b/chrome/browser/nacl_host/nacl_broker_host_win.h
index b44a094..2ed061c 100644
--- a/chrome/browser/nacl_host/nacl_broker_host_win.h
+++ b/chrome/browser/nacl_host/nacl_broker_host_win.h
@@ -28,7 +28,8 @@ class NaClBrokerHost : public content::BrowserChildProcessHostDelegate {
   // a Native Client loader process.
   bool LaunchLoader(const std::string& loader_channel_id);
 
-  bool LaunchDebugExceptionHandler(int32 pid);
+  bool LaunchDebugExceptionHandler(int32 pid,
+                                   base::ProcessHandle process_handle);
 
   // Stop the broker process.
   void StopBroker();
diff --git a/chrome/browser/nacl_host/nacl_broker_service_win.cc b/chrome/browser/nacl_host/nacl_broker_service_win.cc
index da91ef9..9b1a748 100644
--- a/chrome/browser/nacl_host/nacl_broker_service_win.cc
+++ b/chrome/browser/nacl_host/nacl_broker_service_win.cc
@@ -67,12 +67,13 @@ void NaClBrokerService::OnLoaderDied() {
 }
 
 bool NaClBrokerService::LaunchDebugExceptionHandler(
-    base::WeakPtr<NaClProcessHost> nacl_process_host, int32 pid) {
+    base::WeakPtr<NaClProcessHost> nacl_process_host, int32 pid,
+    base::ProcessHandle process_handle) {
   pending_debuggers_[pid] = nacl_process_host;
   NaClBrokerHost* broker_host = GetBrokerHost();
   if (!broker_host)
     return false;
-  return broker_host->LaunchDebugExceptionHandler(pid);
+  return broker_host->LaunchDebugExceptionHandler(pid, process_handle);
 }
 
 void NaClBrokerService::OnDebugExceptionHandlerLaunched(int32 pid) {
diff --git a/chrome/browser/nacl_host/nacl_broker_service_win.h b/chrome/browser/nacl_host/nacl_broker_service_win.h
index 78a891d..e6794a8 100644
--- a/chrome/browser/nacl_host/nacl_broker_service_win.h
+++ b/chrome/browser/nacl_host/nacl_broker_service_win.h
@@ -36,7 +36,8 @@ class NaClBrokerService {
   void OnLoaderDied();
 
   bool LaunchDebugExceptionHandler(base::WeakPtr<NaClProcessHost> client,
-                                   int32 pid);
+                                   int32 pid,
+                                   base::ProcessHandle process_handle);
 
   // Called by NaClBrokerHost to notify the service that a debug
   // exception handler was started.
diff --git a/chrome/browser/nacl_host/nacl_process_host.cc b/chrome/browser/nacl_host/nacl_process_host.cc
index 122c8db..e58d747 100644
--- a/chrome/browser/nacl_host/nacl_process_host.cc
+++ b/chrome/browser/nacl_host/nacl_process_host.cc
@@ -49,6 +49,7 @@
 
 #include "base/threading/thread.h"
 #include "base/process_util.h"
+#include "base/win/scoped_handle.h"
 #include "chrome/browser/nacl_host/nacl_broker_service_win.h"
 #include "content/public/common/sandbox_init.h"
 #include "native_client/src/trusted/service_runtime/win/debug_exception_handler.h"
@@ -572,9 +573,33 @@ void NaClProcessHost::OnChannelConnected(int32 peer_pid) {
   }
   debug_context_->SetNaClProcessHost(weak_factory_.GetWeakPtr());
   if (RunningOnWOW64()) {
-    if (!NaClBrokerService::GetInstance()->LaunchDebugExceptionHandler(
-             weak_factory_.GetWeakPtr(), peer_pid)) {
+    base::win::ScopedHandle process_handle;
+    // We cannot use process_->GetData().handle because it does not
+    // have the necessary access rights.  We open the new handle here
+    // rather than in the NaCl broker process in case the NaCl loader
+    // process dies before the NaCl broker process receives the
+    // message we send.  The debug exception handler uses
+    // DebugActiveProcess() to attach, but this takes a PID.  We need
+    // to prevent the NaCl loader's PID from being reused before
+    // DebugActiveProcess() is called, and holding a process handle
+    // open achieves this.
+    if (!base::OpenProcessHandleWithAccess(
+             peer_pid,
+             base::kProcessAccessQueryInformation |
+             base::kProcessAccessSuspendResume |
+             base::kProcessAccessTerminate |
+             base::kProcessAccessVMOperation |
+             base::kProcessAccessVMRead |
+             base::kProcessAccessVMWrite |
+             base::kProcessAccessWaitForTermination,
+             process_handle.Receive())) {
+      LOG(ERROR) << "Failed to get process handle";
       debug_context_->AllowAndSendStartMessage();
+    } else {
+      if (!NaClBrokerService::GetInstance()->LaunchDebugExceptionHandler(
+             weak_factory_.GetWeakPtr(), peer_pid, process_handle)) {
+        debug_context_->AllowAndSendStartMessage();
+      }
     }
   } else {
     // Start new thread for debug loop
diff --git a/chrome/common/nacl_debug_exception_handler_win.cc b/chrome/common/nacl_debug_exception_handler_win.cc
index 5a37955..4bb0e4f 100644
--- a/chrome/common/nacl_debug_exception_handler_win.cc
+++ b/chrome/common/nacl_debug_exception_handler_win.cc
@@ -6,16 +6,19 @@
 
 #include "base/process_util.h"
 #include "base/threading/platform_thread.h"
+#include "base/win/scoped_handle.h"
 #include "native_client/src/trusted/service_runtime/win/debug_exception_handler.h"
 
 namespace {
 
 class DebugExceptionHandler : public base::PlatformThread::Delegate {
  public:
-  DebugExceptionHandler(int32 pid,
+  DebugExceptionHandler(base::ProcessHandle nacl_process,
                         base::MessageLoopProxy* message_loop,
                         const base::Closure& on_connected)
-      : pid_(pid), message_loop_(message_loop), on_connected_(on_connected) {
+      : nacl_process_(nacl_process),
+        message_loop_(message_loop),
+        on_connected_(on_connected) {
   }
 
   virtual void ThreadMain() OVERRIDE {
@@ -24,20 +27,11 @@ class DebugExceptionHandler : public base::PlatformThread::Delegate {
     // DebugActiveProcess()) on the same thread on which
     // NaClDebugLoop() receives debug events for the process.
     BOOL attached = false;
-    base::ProcessHandle process_handle = base::kNullProcessHandle;
-    if (!base::OpenProcessHandleWithAccess(
-             pid_,
-             base::kProcessAccessQueryInformation |
-             base::kProcessAccessSuspendResume |
-             base::kProcessAccessTerminate |
-             base::kProcessAccessVMOperation |
-             base::kProcessAccessVMRead |
-             base::kProcessAccessVMWrite |
-             base::kProcessAccessWaitForTermination,
-             &process_handle)) {
-      LOG(ERROR) << "Failed to get process handle";
+    int pid = GetProcessId(nacl_process_);
+    if (pid == 0) {
+      LOG(ERROR) << "Invalid process handle";
     } else {
-      attached = DebugActiveProcess(pid_);
+      attached = DebugActiveProcess(pid);
       if (!attached) {
         LOG(ERROR) << "Failed to connect to the process";
       }
@@ -50,16 +44,13 @@ class DebugExceptionHandler : public base::PlatformThread::Delegate {
 
     if (attached) {
       DWORD exit_code;
-      NaClDebugLoop(process_handle, &exit_code);
-    }
-    if (process_handle != base::kNullProcessHandle) {
-      base::CloseProcessHandle(process_handle);
+      NaClDebugLoop(nacl_process_, &exit_code);
     }
     delete this;
   }
 
  private:
-  int32 pid_;
+  base::win::ScopedHandle nacl_process_;
   base::MessageLoopProxy* message_loop_;
   base::Closure on_connected_;
 
@@ -68,13 +59,13 @@ class DebugExceptionHandler : public base::PlatformThread::Delegate {
 
 }  // namespace
 
-void NaClStartDebugExceptionHandlerThread(int32 nacl_process_id,
+void NaClStartDebugExceptionHandlerThread(base::ProcessHandle nacl_process,
                                           base::MessageLoopProxy* message_loop,
                                           const base::Closure& on_connected) {
   // The new PlatformThread will take ownership of the
   // DebugExceptionHandler object, which will delete itself on exit.
   DebugExceptionHandler* handler = new DebugExceptionHandler(
-      nacl_process_id, message_loop, on_connected);
+      nacl_process, message_loop, on_connected);
   if (!base::PlatformThread::CreateNonJoinable(0, handler)) {
     on_connected.Run();
     delete handler;
diff --git a/chrome/common/nacl_debug_exception_handler_win.h b/chrome/common/nacl_debug_exception_handler_win.h
index 4965f93..99f769e 100644
--- a/chrome/common/nacl_debug_exception_handler_win.h
+++ b/chrome/common/nacl_debug_exception_handler_win.h
@@ -8,8 +8,9 @@
 
 #include "base/callback.h"
 #include "base/message_loop.h"
+#include "base/process.h"
 
-void NaClStartDebugExceptionHandlerThread(int32 nacl_process_id,
+void NaClStartDebugExceptionHandlerThread(base::ProcessHandle nacl_process,
                                           base::MessageLoopProxy* message_loop,
                                           const base::Closure& on_connected);
 
diff --git a/chrome/common/nacl_messages.h b/chrome/common/nacl_messages.h
index 67dab31..b244509 100644
--- a/chrome/common/nacl_messages.h
+++ b/chrome/common/nacl_messages.h
@@ -36,8 +36,9 @@ IPC_MESSAGE_CONTROL2(NaClProcessMsg_LoaderLaunched,
 
 // Tells the NaCl broker to attach a debug exception handler to the
 // given NaCl loader process.
-IPC_MESSAGE_CONTROL1(NaClProcessMsg_LaunchDebugExceptionHandler,
-                     int32 /* pid */)
+IPC_MESSAGE_CONTROL2(NaClProcessMsg_LaunchDebugExceptionHandler,
+                     int32 /* pid of the NaCl process */,
+                     base::ProcessHandle /* handle of the NaCl process */)
 
 // Notify the browser process that the broker process finished
 // attaching a debug exception handler to the given NaCl loader
diff --git a/chrome/nacl/nacl_broker_listener.cc b/chrome/nacl/nacl_broker_listener.cc
index 086f2a6..4782e1e 100644
--- a/chrome/nacl/nacl_broker_listener.cc
+++ b/chrome/nacl/nacl_broker_listener.cc
@@ -98,9 +98,11 @@ void NaClBrokerListener::OnLaunchLoaderThroughBroker(
                                                    loader_handle_in_browser));
 }
 
-void NaClBrokerListener::OnLaunchDebugExceptionHandler(int32 pid) {
+void NaClBrokerListener::OnLaunchDebugExceptionHandler(
+    int32 pid, base::ProcessHandle process_handle) {
   base::Closure reply_sender(base::Bind(SendReply, channel_.get(), pid));
-  NaClStartDebugExceptionHandlerThread(pid, base::MessageLoopProxy::current(),
+  NaClStartDebugExceptionHandlerThread(process_handle,
+                                       base::MessageLoopProxy::current(),
                                        reply_sender);
 }
 
diff --git a/chrome/nacl/nacl_broker_listener.h b/chrome/nacl/nacl_broker_listener.h
index f30e548..94f97c9 100644
--- a/chrome/nacl/nacl_broker_listener.h
+++ b/chrome/nacl/nacl_broker_listener.h
@@ -27,7 +27,8 @@ class NaClBrokerListener : public IPC::Channel::Listener {
 
  private:
   void OnLaunchLoaderThroughBroker(const std::string& loader_channel_id);
-  void OnLaunchDebugExceptionHandler(int32 pid);
+  void OnLaunchDebugExceptionHandler(int32 pid,
+                                     base::ProcessHandle process_handle);
   void OnStopBroker();
 
   base::ProcessHandle browser_handle_;
diff --git a/ipc/ipc_message_utils.h b/ipc/ipc_message_utils.h
index 8772b67..9e259f1 100644
--- a/ipc/ipc_message_utils.h
+++ b/ipc/ipc_message_utils.h
@@ -706,15 +706,18 @@ struct ParamTraits<string16> {
 template <>
 struct ParamTraits<HANDLE> {
   typedef HANDLE param_type;
+  // Note that HWNDs/HANDLE/HCURSOR/HACCEL etc are always 32 bits, even on 64
+  // bit systems.
   static void Write(Message* m, const param_type& p) {
-    // Note that HWNDs/HANDLE/HCURSOR/HACCEL etc are always 32 bits, even on 64
-    // bit systems.
     m->WriteUInt32(reinterpret_cast<uint32>(p));
   }
   static bool Read(const Message* m, PickleIterator* iter,
                    param_type* r) {
-    DCHECK_EQ(sizeof(param_type), sizeof(uint32));
-    return m->ReadUInt32(iter, reinterpret_cast<uint32*>(r));
+    uint32 temp;
+    if (!m->ReadUInt32(iter, &temp))
+      return false;
+    *r = reinterpret_cast<HANDLE>(temp);
+    return true;
   }
   static void Log(const param_type& p, std::string* l) {
     l->append(StringPrintf("0x%X", p));