Sandbox: Remove ::CloseHandle from BrokerServices.

sandbox/win/src/broker_services.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 "sandbox/win/src/broker_services.h"
 
 #include <AclAPI.h>
 
 #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"
 #include "base/win/startup_information.h"
 #include "base/win/windows_version.h"
 #include "sandbox/win/src/app_container.h"
 #include "sandbox/win/src/process_mitigations.h"
 #include "sandbox/win/src/sandbox_policy_base.h"
 #include "sandbox/win/src/sandbox.h"
 #include "sandbox/win/src/target_process.h"
@@ skipping 27 matching lines @@
 enum {
   THREAD_CTRL_NONE,
   THREAD_CTRL_REMOVE_PEER,
   THREAD_CTRL_QUIT,
   THREAD_CTRL_LAST,
 };
 
 // Helper structure that allows the Broker to associate a job notification
 // with a job object and with a policy.
 struct JobTracker {
-  HANDLE job;
+  JobTracker(base::win::ScopedHandle job, sandbox::PolicyBase* policy)
+      : job(job.Pass()), policy(policy) {
+  }
+  ~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;
-  JobTracker(HANDLE cjob, sandbox::PolicyBase* cpolicy)
-      : job(cjob), policy(cpolicy) {
+};
+
+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 stale_job_handle = job.Get();
+    job.Close();
+
+    // In OnJobEmpty() we don't actually use the job handle directly.
+    policy->OnJobEmpty(stale_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) {
   // Deregistration shouldn't fail, but we leak rather than crash if it does.
   if (::UnregisterWaitEx(peer->wait_object, INVALID_HANDLE_VALUE)) {
     delete peer;
   } else {
     NOTREACHED();
   }
 }
@@ skipping 33 matching lines @@
   key <<= kTokenShift;
   key |= policy->GetIntegrityLevel();
 
   return key;
 }
 
 }  // namespace
 
 namespace sandbox {
 
-BrokerServicesBase::BrokerServicesBase()
-    : job_port_(NULL),
-      no_targets_(NULL),
-      job_thread_(NULL),
-      thread_pool_(NULL) {
+// TODO(rvargas): Replace this structure with a std::pair of ScopedHandles.
+struct BrokerServicesBase::TokenPair {
+  TokenPair(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;
 }
 
 // The destructor should only be called when the Broker process is terminating.
 // Since BrokerServicesBase is a singleton, this is called from the CRT
 // termination handlers, if this code lives on a DLL it is called during
 // DLL_PROCESS_DETACH in other words, holding the loader lock, so we cannot
 // 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)) {
     // 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_);
   delete thread_pool_;
-  ::CloseHandle(no_targets_);
 
   // Cancel the wait events and delete remaining peer trackers.
   for (PeerTrackerMap::iterator it = peer_map_.begin();
        it != peer_map_.end(); ++it) {
     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() {
   // If you change the type of the object being created here you must also
   // change the downcast to it in SpawnTarget().
   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
 // the policy about events.
 DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
   if (NULL == param)
     return 1;
 
   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);
 
   while (true) {
     DWORD events = 0;
     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
       // that jobs can send and some of them depend on the job attributes set.
       JobTracker* tracker = reinterpret_cast<JobTracker*>(key);
 
       switch (events) {
         case JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO: {
           // The job object has signaled that the last process associated
           // with it has terminated. Assuming there is no way for a process
           // 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;
         }
 
         case JOB_OBJECT_MSG_NEW_PROCESS: {
           ++target_counter;
           if (1 == target_counter) {
             ::ResetEvent(no_targets);
           }
           break;
         }
@@ skipping 11 matching lines @@
 
           DCHECK(target_counter >= 0);
           break;
         }
 
         case JOB_OBJECT_MSG_ACTIVE_PROCESS_LIMIT: {
           break;
         }
 
         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;
         }
 
         default: {
           NOTREACHED();
           break;
         }
       }
@@ skipping 49 matching lines @@
   base::win::ScopedHandle initial_token;
   base::win::ScopedHandle lockdown_token;
   ResultCode result = SBOX_ALL_OK;
 
   if (IsTokenCacheable(policy_base)) {
     // Create the master tokens only once and save them in a cache. That way
     // can just duplicate them to avoid hammering LSASS on every sandboxed
     // 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;
+    TokenPair* 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 TokenPair(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)
       return result;
   }
 
   HANDLE job_temp;
   result = policy_base->MakeJobObject(&job_temp);
   if (SBOX_ALL_OK != result)
     return result;
@@ skipping 109 matching lines @@
 
   // Now the policy is the owner of the target.
   if (!policy_base->AddTarget(target)) {
     return SpawnCleanup(target, 0);
   }
 
   // We are going to keep a pointer to the policy because we'll call it when
   // 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.
     tracker_list_.push_back(tracker.release());
     child_process_ids_.insert(process_info.process_id());
   } else {
     // We have to signal the event once here because the completion port will
     // 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.
     // Sandbox policy engine needs to know that these processes are valid
     // targets for e.g. BrokerDuplicateHandle so track them as peer processes.
     AddTargetPeer(process_info.process_handle());
   }
 
   *target_info = process_info.Take();
   return SBOX_ALL_OK;
 }
 
 
 ResultCode BrokerServicesBase::WaitForAllTargets() {
-  ::WaitForSingleObject(no_targets_, INFINITE);
+  ::WaitForSingleObject(no_targets_.Get(), INFINITE);
   return SBOX_ALL_OK;
 }
 
 bool BrokerServicesBase::IsActiveTarget(DWORD process_id) {
   AutoLock lock(&lock_);
   return child_process_ids_.find(process_id) != child_process_ids_.end() ||
          peer_map_.find(process_id) != peer_map_.end();
 }
 
 VOID CALLBACK BrokerServicesBase::RemovePeer(PVOID parameter, BOOLEAN timeout) {
   PeerTracker* peer = reinterpret_cast<PeerTracker*>(parameter);
   // Don't check the return code because we this may fail (safely) at shutdown.
   ::PostQueuedCompletionStatus(
       peer->job_port, 0, THREAD_CTRL_REMOVE_PEER,
       reinterpret_cast<LPOVERLAPPED>(static_cast<uintptr_t>(peer->id)));
 }
 
 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;
 
   HANDLE process_handle;
   if (!::DuplicateHandle(::GetCurrentProcess(), peer_process,
                          ::GetCurrentProcess(), &process_handle,
                          SYNCHRONIZE, FALSE, 0)) {
     return SBOX_ERROR_GENERIC;
   }
   peer->process.Set(process_handle);
@@ skipping 34 matching lines @@
     return SBOX_ERROR_UNSUPPORTED;
 
   base::string16 name = LookupAppContainer(sid);
   if (name.empty())
     return SBOX_ERROR_INVALID_APP_CONTAINER;
 
   return DeleteAppContainer(sid);
 }
 
 }  // namespace sandbox