Sandbox: Remove ::CloseHandle from BrokerServices.

sandbox/win/src/broker_services.cc

Index: sandbox/win/src/broker_services.cc
diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc
index 68d68d829325d4b95ce6406d6638e3198eb96761..4a3a92931af757e7223dcfa83242c38e7fbe9efc 100644
--- a/sandbox/win/src/broker_services.cc
+++ b/sandbox/win/src/broker_services.cc
@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/stl_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/scoped_process_information.h"
@@ -55,22 +56,47 @@ enum {
 // Helper structure that allows the Broker to associate a job notification
 // with a job object and with a policy.
 struct JobTracker {
-  HANDLE job;
-  sandbox::PolicyBase* policy;
-  JobTracker(HANDLE cjob, sandbox::PolicyBase* cpolicy)
-      : job(cjob), policy(cpolicy) {
+  JobTracker(base::win::ScopedHandle cjob, sandbox::PolicyBase* cpolicy)

[cpu_(ooo_6.6-7.5), 2015/07/17 19:33:24]

I don't think you need to add 'c' to the args. I believe c++ does not get
confused here.

[rvargas (doing something else), 2015/07/20 20:06:30]

I'm not adding 'c'... I'm just moving this code a little to have methods first
and members last. But sure, removed the 'c'. (even if c++ doesn't get confused
I'm not sure the result is more readable though... but I don't like cfoo
either). 

+      : job(cjob.Pass()), policy(cpolicy) {
+  }
+  ~JobTracker() {
+    FreeResources();
   }
+
+  // Releases the Job and notifies the associated Policy object to release its
+  // resources as well.
+  void FreeResources();
+
+  base::win::ScopedHandle job;
+  sandbox::PolicyBase* policy;
 };
 
+void JobTracker::FreeResources() {
+  if (policy) {
+    BOOL res = ::TerminateJobObject(job.Get(), sandbox::SBOX_ALL_OK);
+    DCHECK(res);
+    // Closing the job causes the target process to be destroyed so this needs
+    // to happen before calling OnJobEmpty().
+    HANDLE old_job_handle = job.Get();

[cpu_(ooo_6.6-7.5), 2015/07/17 19:33:24]

so old_job_handle is more really invalid_job_handle ?

[rvargas (doing something else), 2015/07/20 20:06:30]

yes, it becomes stale next line. (saying this was the old_handle doesn't really
tells me that the handle is valid :p ). Renamed to stale_ (but it is assigned a
handle that is not stale_... oh well).

+    job.Close();
+
+    // In OnJobEmpty() we don't actually use the job handle directly.
+    policy->OnJobEmpty(old_job_handle);
+    policy->Release();
+    policy = NULL;
+  }
+}
+
 // Helper structure that allows the broker to track peer processes
 struct PeerTracker {
+  PeerTracker(DWORD process_id, HANDLE broker_job_port)
+      : wait_object(NULL), id(process_id), job_port(broker_job_port) {
+  }
+
   HANDLE wait_object;
   base::win::ScopedHandle process;
   DWORD id;
   HANDLE job_port;
-  PeerTracker(DWORD process_id, HANDLE broker_job_port)
-      : wait_object(NULL), id(process_id), job_port(broker_job_port) {
-  }
 };
 
 void DeregisterPeerTracker(PeerTracker* peer) {
@@ -124,30 +150,38 @@ uint32_t GenerateTokenCacheKey(const sandbox::PolicyBase* policy) {
 
 namespace sandbox {
 
-BrokerServicesBase::BrokerServicesBase()
-    : job_port_(NULL),
-      no_targets_(NULL),
-      job_thread_(NULL),
-      thread_pool_(NULL) {
+// TODO(rvargas): Store a pair of ScopedHandles when that becomes possible.

[cpu_(ooo_6.6-7.5), 2015/07/17 19:33:24]

the 153 comment, which 'pair' are you referring to?

[rvargas (doing something else), 2015/07/20 20:06:30]

Reworded

+struct BrokerServicesBase::Tokens {

[cpu_(ooo_6.6-7.5), 2015/07/17 19:33:24]

I feel like maybe this class should be named TokenPair, but not too attached to
that.

[rvargas (doing something else), 2015/07/20 20:06:30]

Done.

+  Tokens(base::win::ScopedHandle initial_token,
+         base::win::ScopedHandle lockdown_token)
+      : initial(initial_token.Pass()),
+        lockdown(lockdown_token.Pass()) {
+  }
+
+  base::win::ScopedHandle initial;
+  base::win::ScopedHandle lockdown;
+};
+
+BrokerServicesBase::BrokerServicesBase() : thread_pool_(NULL) {
 }
 
 // The broker uses a dedicated worker thread that services the job completion
 // port to perform policy notifications and associated cleanup tasks.
 ResultCode BrokerServicesBase::Init() {
-  if ((NULL != job_port_) || (NULL != thread_pool_))
+  if (job_port_.IsValid() || (NULL != thread_pool_))
     return SBOX_ERROR_UNEXPECTED_CALL;
 
   ::InitializeCriticalSection(&lock_);
 
-  job_port_ = ::CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 0);
-  if (NULL == job_port_)
+  job_port_.Set(::CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 0));
+  if (!job_port_.IsValid())
     return SBOX_ERROR_GENERIC;
 
-  no_targets_ = ::CreateEventW(NULL, TRUE, FALSE, NULL);
+  no_targets_.Set(::CreateEventW(NULL, TRUE, FALSE, NULL));
 
-  job_thread_ = ::CreateThread(NULL, 0,  // Default security and stack.
-                               TargetEventsThread, this, NULL, NULL);
-  if (NULL == job_thread_)
+  job_thread_.Set(::CreateThread(NULL, 0,  // Default security and stack.
+                                 TargetEventsThread, this, NULL, NULL));
+  if (!job_thread_.IsValid())
     return SBOX_ERROR_GENERIC;
 
   return SBOX_ALL_OK;
@@ -160,31 +194,24 @@ ResultCode BrokerServicesBase::Init() {
 // wait for threads here.
 BrokerServicesBase::~BrokerServicesBase() {
   // If there is no port Init() was never called successfully.
-  if (!job_port_)
+  if (!job_port_.IsValid())
     return;
 
   // Closing the port causes, that no more Job notifications are delivered to
   // the worker thread and also causes the thread to exit. This is what we
   // want to do since we are going to close all outstanding Jobs and notifying
   // the policy objects ourselves.
-  ::PostQueuedCompletionStatus(job_port_, 0, THREAD_CTRL_QUIT, FALSE);
-  ::CloseHandle(job_port_);
+  ::PostQueuedCompletionStatus(job_port_.Get(), 0, THREAD_CTRL_QUIT, FALSE);
 
-  if (WAIT_TIMEOUT == ::WaitForSingleObject(job_thread_, 1000)) {
+  if (job_thread_.IsValid() &&
+      WAIT_TIMEOUT == ::WaitForSingleObject(job_thread_.Get(), 1000)) {

[cpu_(ooo_6.6-7.5), 2015/07/17 19:33:24]

but if job thread is not valid WFSO will return an error ... so the IsValid() is
not needed?

[rvargas (doing something else), 2015/07/20 20:06:30]

I don't think it's good form to pass invalid handles to APIs.

     // Cannot clean broker services.
     NOTREACHED();
     return;
   }
 
-  JobTrackerList::iterator it;
-  for (it = tracker_list_.begin(); it != tracker_list_.end(); ++it) {
-    JobTracker* tracker = (*it);
-    FreeResources(tracker);
-    delete tracker;
-  }
-  ::CloseHandle(job_thread_);
+  STLDeleteElements(&tracker_list_);

[cpu_(ooo_6.6-7.5), 2015/07/17 19:33:24]

nice!

   delete thread_pool_;
-  ::CloseHandle(no_targets_);
 
   // Cancel the wait events and delete remaining peer trackers.
   for (PeerTrackerMap::iterator it = peer_map_.begin();
@@ -192,16 +219,10 @@ BrokerServicesBase::~BrokerServicesBase() {
     DeregisterPeerTracker(it->second);
   }
 
-  // If job_port_ isn't NULL, assumes that the lock has been initialized.
-  if (job_port_)
-    ::DeleteCriticalSection(&lock_);
+  ::DeleteCriticalSection(&lock_);
 
   // Close any token in the cache.
-  for (TokenCacheMap::iterator it = token_cache_.begin();
-       it != token_cache_.end(); ++it) {
-    ::CloseHandle(it->second.first);
-    ::CloseHandle(it->second.second);
-  }
+  STLDeleteValues(&token_cache_);
 }
 
 TargetPolicy* BrokerServicesBase::CreatePolicy() {
@@ -210,21 +231,6 @@ TargetPolicy* BrokerServicesBase::CreatePolicy() {
   return new PolicyBase;
 }
 
-void BrokerServicesBase::FreeResources(JobTracker* tracker) {
-  if (NULL != tracker->policy) {
-    BOOL res = ::TerminateJobObject(tracker->job, SBOX_ALL_OK);
-    DCHECK(res);
-    // Closing the job causes the target process to be destroyed so this
-    // needs to happen before calling OnJobEmpty().
-    res = ::CloseHandle(tracker->job);
-    DCHECK(res);
-    // In OnJobEmpty() we don't actually use the job handle directly.
-    tracker->policy->OnJobEmpty(tracker->job);
-    tracker->policy->Release();
-    tracker->policy = NULL;
-  }
-}
-
 // The worker thread stays in a loop waiting for asynchronous notifications
 // from the job objects. Right now we only care about knowing when the last
 // process on a job terminates, but in general this is the place to tell
@@ -236,8 +242,8 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
   base::PlatformThread::SetName("BrokerEvent");
 
   BrokerServicesBase* broker = reinterpret_cast<BrokerServicesBase*>(param);
-  HANDLE port = broker->job_port_;
-  HANDLE no_targets = broker->no_targets_;
+  HANDLE port = broker->job_port_.Get();
+  HANDLE no_targets = broker->no_targets_.Get();
 
   int target_counter = 0;
   ::ResetEvent(no_targets);
@@ -247,11 +253,12 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
     ULONG_PTR key = 0;
     LPOVERLAPPED ovl = NULL;
 
-    if (!::GetQueuedCompletionStatus(port, &events, &key, &ovl, INFINITE))
+    if (!::GetQueuedCompletionStatus(port, &events, &key, &ovl, INFINITE)) {
       // this call fails if the port has been closed before we have a
       // chance to service the last packet which is 'exit' anyway so
       // this is not an error.
       return 1;
+    }
 
     if (key > THREAD_CTRL_LAST) {
       // The notification comes from a job object. There are nine notifications
@@ -265,7 +272,7 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
           // to appear out of thin air in this job, it safe to assume that
           // we can tell the policy to destroy the target object, and for
           // us to release our reference to the policy object.
-          FreeResources(tracker);
+          tracker->FreeResources();
           break;
         }
 
@@ -297,7 +304,7 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
         }
 
         case JOB_OBJECT_MSG_PROCESS_MEMORY_LIMIT: {
-          BOOL res = ::TerminateJobObject(tracker->job,
+          BOOL res = ::TerminateJobObject(tracker->job.Get(),
                                           SBOX_FATAL_MEMORY_EXCEEDED);
           DCHECK(res);
           break;
@@ -367,33 +374,31 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
     // process launch.
     uint32_t token_key = GenerateTokenCacheKey(policy_base);
     TokenCacheMap::iterator it = token_cache_.find(token_key);
-    HANDLE initial_token_temp;
-    HANDLE lockdown_token_temp;
+    Tokens* tokens;
     if (it != token_cache_.end()) {
-      initial_token_temp = it->second.first;
-      lockdown_token_temp = it->second.second;
+      tokens = it->second;
     } else {
       result = policy_base->MakeTokens(&initial_token, &lockdown_token);
       if (SBOX_ALL_OK != result)
         return result;
-      token_cache_[token_key] =
-          std::make_pair(initial_token.Get(), lockdown_token.Get());
-      initial_token_temp = initial_token.Take();
-      lockdown_token_temp = lockdown_token.Take();
+
+      tokens = new Tokens(initial_token.Pass(), lockdown_token.Pass());
+      token_cache_[token_key] = tokens;
     }
 
-    if (!::DuplicateToken(initial_token_temp, SecurityImpersonation,
-                          &initial_token_temp)) {
+    HANDLE temp_token;
+    if (!::DuplicateToken(tokens->initial.Get(), SecurityImpersonation,
+                          &temp_token)) {
       return SBOX_ERROR_GENERIC;
     }
-    initial_token.Set(initial_token_temp);
+    initial_token.Set(temp_token);
 
-    if (!::DuplicateTokenEx(lockdown_token_temp, TOKEN_ALL_ACCESS, 0,
+    if (!::DuplicateTokenEx(tokens->lockdown.Get(), TOKEN_ALL_ACCESS, 0,
                             SecurityIdentification, TokenPrimary,
-                            &lockdown_token_temp)) {
+                            &temp_token)) {
       return SBOX_ERROR_GENERIC;
     }
-    lockdown_token.Set(lockdown_token_temp);
+    lockdown_token.Set(temp_token);
   } else {
     result = policy_base->MakeTokens(&initial_token, &lockdown_token);
     if (SBOX_ALL_OK != result)
@@ -523,11 +528,12 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
   // the job object generates notifications using the completion port.
   policy_base->AddRef();
   if (job.IsValid()) {
-    scoped_ptr<JobTracker> tracker(new JobTracker(job.Take(), policy_base));
+    scoped_ptr<JobTracker> tracker(new JobTracker(job.Pass(), policy_base));
 
     // There is no obvious recovery after failure here. Previous version with
     // SpawnCleanup() caused deletion of TargetProcess twice. crbug.com/480639
-    CHECK(AssociateCompletionPort(tracker->job, job_port_, tracker.get()));
+    CHECK(AssociateCompletionPort(tracker->job.Get(), job_port_.Get(),
+                                  tracker.get()));
 
     // Save the tracker because in cleanup we might need to force closing
     // the Jobs.
@@ -538,7 +544,7 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
     // never get a message that this target is being terminated thus we should
     // not block WaitForAllTargets until we have at least one target with job.
     if (child_process_ids_.empty())
-      ::SetEvent(no_targets_);
+      ::SetEvent(no_targets_.Get());
     // We can not track the life time of such processes and it is responsibility
     // of the host application to make sure that spawned targets without jobs
     // are terminated when the main application don't need them anymore.
@@ -553,7 +559,7 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
 
 
 ResultCode BrokerServicesBase::WaitForAllTargets() {
-  ::WaitForSingleObject(no_targets_, INFINITE);
+  ::WaitForSingleObject(no_targets_.Get(), INFINITE);
   return SBOX_ALL_OK;
 }
 
@@ -573,7 +579,7 @@ VOID CALLBACK BrokerServicesBase::RemovePeer(PVOID parameter, BOOLEAN timeout) {
 
 ResultCode BrokerServicesBase::AddTargetPeer(HANDLE peer_process) {
   scoped_ptr<PeerTracker> peer(new PeerTracker(::GetProcessId(peer_process),
-                                               job_port_));
+                                               job_port_.Get()));
   if (!peer->id)
     return SBOX_ERROR_GENERIC;