Reland "Fix sandbox::PolicyBase leak"

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 <stddef.h>
 
 #include <utility>
@@ skipping 35 matching lines @@
 // executes TargetEventsThread().
 enum {
   THREAD_CTRL_NONE,
   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 {
-  JobTracker(base::win::ScopedHandle job, sandbox::PolicyBase* policy)
+  JobTracker(base::win::ScopedHandle job,
+             scoped_refptr<sandbox::PolicyBase> policy)
       : job(std::move(job)), 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;
+  scoped_refptr<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 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;
+    policy = nullptr;
   }
 }
 
 }  // namespace
 
 namespace sandbox {
 
 BrokerServicesBase::BrokerServicesBase() {}
 
 // The broker uses a dedicated worker thread that services the job completion
@@ skipping 40 matching lines @@
     NOTREACHED();
     return;
   }
 
   tracker_list_.clear();
   thread_pool_.reset();
 
   ::DeleteCriticalSection(&lock_);
 }
 
-TargetPolicy* BrokerServicesBase::CreatePolicy() {
+scoped_refptr<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;
+  scoped_refptr<TargetPolicy> policy(new PolicyBase);
+  // PolicyBase starts with refcount 1.
+  policy->Release();
+  return policy;
 }
 
 // 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;
 
@@ skipping 102 matching lines @@
   }
 
   NOTREACHED();
   return 0;
 }
 
 // SpawnTarget does all the interesting sandbox setup and creates the target
 // process inside the sandbox.
 ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
                                            const wchar_t* command_line,
-                                           TargetPolicy* policy,
+                                           scoped_refptr<TargetPolicy> policy,
                                            ResultCode* last_warning,
                                            DWORD* last_error,
                                            PROCESS_INFORMATION* target_info) {
   if (!exe_path)
     return SBOX_ERROR_BAD_PARAMS;
 
   if (!policy)
     return SBOX_ERROR_BAD_PARAMS;
 
   // Even though the resources touched by SpawnTarget can be accessed in
   // multiple threads, the method itself cannot be called from more than
   // 1 thread. This is to protect the global variables used while setting up
   // the child process.
   static DWORD thread_id = ::GetCurrentThreadId();
   DCHECK(thread_id == ::GetCurrentThreadId());
   *last_warning = SBOX_ALL_OK;
 
   AutoLock lock(&lock_);
 
   // This downcast is safe as long as we control CreatePolicy()
-  PolicyBase* policy_base = static_cast<PolicyBase*>(policy);
+  scoped_refptr<PolicyBase> policy_base(static_cast<PolicyBase*>(policy.get()));
 
   // Construct the tokens and the job object that we are going to associate
   // with the soon to be created target process.
   base::win::ScopedHandle initial_token;
   base::win::ScopedHandle lockdown_token;
   base::win::ScopedHandle lowbox_token;
   ResultCode result = SBOX_ALL_OK;
 
   result =
       policy_base->MakeTokens(&initial_token, &lockdown_token, &lowbox_token);
@@ skipping 131 matching lines @@
   result = policy_base->AddTarget(target);
 
   if (result != SBOX_ALL_OK) {
     *last_error = ::GetLastError();
     SpawnCleanup(target);
     return result;
   }
 
   // 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()) {
     std::unique_ptr<JobTracker> tracker =
         base::MakeUnique<JobTracker>(std::move(job), 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.Get(), job_port_.Get(),
                                   tracker.get()));
 
     // Save the tracker because in cleanup we might need to force closing
     // the Jobs.
     tracker_list_.push_back(std::move(tracker));
     child_process_ids_.insert(process_info.process_id());
   } else {
+    // Leak policy_base. This needs to outlive the child process, but there's
+    // nothing that tracks that lifetime properly if there's no job object.
+    // TODO(wfh): Find a way to make this have the correct lifetime.
+    policy_base->AddRef();

[Will Harris, 2017/01/24 19:47:32]

note: new change

+
     // 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_.Get());
   }
 
   *target_info = process_info.Take();
   return result;
 }
 
 
 ResultCode BrokerServicesBase::WaitForAllTargets() {
   ::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();
 }
 
 }  // namespace sandbox