Fix sandbox::PolicyBase leak

StartSandboxedProcess was missing a Release call, so this was always
leaking. Use scoped_refptr instead to fix that and prevent it from
happening in the future.
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2316333003
Cr-Commit-Position: refs/heads/master@{#417313}

content/common/sandbox_init_win.cc

@@ -27,10 +27,10 @@ bool InitializeSandbox(sandbox::SandboxInterfaceInfo* sandbox_info) {
     // broken. This has to run before threads and windows are created.
     if (!command_line.HasSwitch(switches::kNoSandbox)) {
       // Precreate the desktop and window station used by the renderers.
-      sandbox::TargetPolicy* policy = broker_services->CreatePolicy();
+      scoped_refptr<sandbox::TargetPolicy> policy =
+          broker_services->CreatePolicy();
       sandbox::ResultCode result = policy->CreateAlternateDesktop(true);
       CHECK(sandbox::SBOX_ERROR_FAILED_TO_SWITCH_BACK_WINSTATION != result);
-      policy->Release();
     }
     return true;
   }

content/common/sandbox_win.cc

@@ -708,7 +708,8 @@ sandbox::ResultCode StartSandboxedProcess(
     return sandbox::SBOX_ALL_OK;
   }
 
-  sandbox::TargetPolicy* policy = g_broker_services->CreatePolicy();
+  scoped_refptr<sandbox::TargetPolicy> policy =
+      g_broker_services->CreatePolicy();
 
   // Add any handles to be inherited to the policy.
   for (HANDLE handle : handles_to_inherit)
@@ -737,7 +738,7 @@ sandbox::ResultCode StartSandboxedProcess(
 #if !defined(NACL_WIN64)
   if (type_str == switches::kRendererProcess &&
       IsWin32kRendererLockdownEnabled()) {
-    result = AddWin32kLockdownPolicy(policy, false);
+    result = AddWin32kLockdownPolicy(policy.get(), false);
     if (result != sandbox::SBOX_ALL_OK)
       return result;
   }
@@ -751,12 +752,12 @@ sandbox::ResultCode StartSandboxedProcess(
   if (result != sandbox::SBOX_ALL_OK)
     return result;
 
-  result = SetJobLevel(*cmd_line, sandbox::JOB_LOCKDOWN, 0, policy);
+  result = SetJobLevel(*cmd_line, sandbox::JOB_LOCKDOWN, 0, policy.get());
   if (result != sandbox::SBOX_ALL_OK)
     return result;
 
   if (!delegate->DisableDefaultPolicy()) {
-    result = AddPolicyForSandboxedProcess(policy);
+    result = AddPolicyForSandboxedProcess(policy.get());
     if (result != sandbox::SBOX_ALL_OK)
       return result;
   }
@@ -765,7 +766,7 @@ sandbox::ResultCode StartSandboxedProcess(
   if (type_str == switches::kRendererProcess ||
       type_str == switches::kPpapiPluginProcess) {
     AddDirectory(base::DIR_WINDOWS_FONTS, NULL, true,
-                 sandbox::TargetPolicy::FILES_ALLOW_READONLY, policy);
+                 sandbox::TargetPolicy::FILES_ALLOW_READONLY, policy.get());
   }
 #endif
 
@@ -776,7 +777,7 @@ sandbox::ResultCode StartSandboxedProcess(
     cmd_line->AppendSwitchASCII("ignored", " --type=renderer ");
   }
 
-  result = AddGenericPolicy(policy);
+  result = AddGenericPolicy(policy.get());
 
   if (result != sandbox::SBOX_ALL_OK) {
     NOTREACHED();
@@ -801,7 +802,7 @@ sandbox::ResultCode StartSandboxedProcess(
   policy->SetStdoutHandle(GetStdHandle(STD_OUTPUT_HANDLE));
   policy->SetStderrHandle(GetStdHandle(STD_ERROR_HANDLE));
 
-  if (!delegate->PreSpawnTarget(policy))
+  if (!delegate->PreSpawnTarget(policy.get()))
     return sandbox::SBOX_ERROR_DELEGATE_PRE_SPAWN;
 
   TRACE_EVENT_BEGIN0("startup", "StartProcessWithAccess::LAUNCHPROCESS");

sandbox/win/sandbox_poc/main_ui_window.cc

@@ -500,7 +500,7 @@ bool MainUIWindow::SpawnTarget() {
 
   arguments[size_call - 1] = L'\0';
 
-  sandbox::TargetPolicy* policy = broker_->CreatePolicy();
+  scoped_refptr<sandbox::TargetPolicy> policy = broker_->CreatePolicy();
   policy->SetJobLevel(sandbox::JOB_LOCKDOWN, 0);
   policy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
                         sandbox::USER_LOCKDOWN);
@@ -519,7 +519,6 @@ bool MainUIWindow::SpawnTarget() {
       broker_->SpawnTarget(spawn_target_.c_str(), arguments, policy,
                            &warning_result, &last_error, &target_);
 
-  policy->Release();
   policy = NULL;
 
   bool return_value = false;

sandbox/win/src/broker_services.cc

@@ -54,7 +54,8 @@ enum {
 // 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();
@@ -65,7 +66,7 @@ struct JobTracker {
   void FreeResources();
 
   base::win::ScopedHandle job;
-  sandbox::PolicyBase* policy;
+  scoped_refptr<sandbox::PolicyBase> policy;
 };
 
 void JobTracker::FreeResources() {
@@ -79,8 +80,7 @@ void JobTracker::FreeResources() {
 
     // In OnJobEmpty() we don't actually use the job handle directly.
     policy->OnJobEmpty(stale_job_handle);
-    policy->Release();
-    policy = NULL;
+    policy = nullptr;
   }
 }
 
@@ -142,10 +142,13 @@ BrokerServicesBase::~BrokerServicesBase() {
   ::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
@@ -268,7 +271,7 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
 // 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) {
@@ -289,7 +292,7 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
   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.
@@ -441,7 +444,6 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
 
   // 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(
         new JobTracker(std::move(job), policy_base));

sandbox/win/src/broker_services.h

@@ -45,10 +45,10 @@ class BrokerServicesBase final : public BrokerServices,
 
   // BrokerServices interface.
   ResultCode Init() override;
-  TargetPolicy* CreatePolicy() override;
+  scoped_refptr<TargetPolicy> CreatePolicy() override;
   ResultCode 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) override;

sandbox/win/src/policy_target_test.cc

@@ -222,9 +222,9 @@ TEST(PolicyTargetTest, DesktopPolicy) {
   BrokerServices* broker = GetBroker();
 
   // Precreate the desktop.
-  TargetPolicy* temp_policy = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> temp_policy = broker->CreatePolicy();
   temp_policy->CreateAlternateDesktop(false);
-  temp_policy->Release();
+  temp_policy = nullptr;
 
   ASSERT_TRUE(broker != NULL);
 
@@ -242,7 +242,7 @@ TEST(PolicyTargetTest, DesktopPolicy) {
   DWORD last_error = ERROR_SUCCESS;
   base::win::ScopedProcessInformation target;
 
-  TargetPolicy* policy = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> policy = broker->CreatePolicy();
   policy->SetAlternateDesktop(false);
   policy->SetTokenLevel(USER_INTERACTIVE, USER_LOCKDOWN);
   PROCESS_INFORMATION temp_process_info = {};
@@ -250,7 +250,7 @@ TEST(PolicyTargetTest, DesktopPolicy) {
       broker->SpawnTarget(prog_name, arguments.c_str(), policy, &warning_result,
                           &last_error, &temp_process_info);
   base::string16 desktop_name = policy->GetAlternateDesktop();
-  policy->Release();
+  policy = nullptr;
 
   EXPECT_EQ(SBOX_ALL_OK, result);
   if (result == SBOX_ALL_OK)
@@ -274,7 +274,7 @@ TEST(PolicyTargetTest, DesktopPolicy) {
   // Close the desktop handle.
   temp_policy = broker->CreatePolicy();
   temp_policy->DestroyAlternateDesktop();
-  temp_policy->Release();
+  temp_policy = nullptr;
 
   // Make sure the desktop does not exist anymore.
   desk = ::OpenDesktop(desktop_name.c_str(), 0, FALSE, DESKTOP_ENUMERATE);
@@ -289,9 +289,9 @@ TEST(PolicyTargetTest, WinstaPolicy) {
   BrokerServices* broker = GetBroker();
 
   // Precreate the desktop.
-  TargetPolicy* temp_policy = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> temp_policy = broker->CreatePolicy();
   temp_policy->CreateAlternateDesktop(true);
-  temp_policy->Release();
+  temp_policy = nullptr;
 
   ASSERT_TRUE(broker != NULL);
 
@@ -308,7 +308,7 @@ TEST(PolicyTargetTest, WinstaPolicy) {
   ResultCode warning_result = SBOX_ALL_OK;
   base::win::ScopedProcessInformation target;
 
-  TargetPolicy* policy = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> policy = broker->CreatePolicy();
   policy->SetAlternateDesktop(true);
   policy->SetTokenLevel(USER_INTERACTIVE, USER_LOCKDOWN);
   PROCESS_INFORMATION temp_process_info = {};
@@ -317,7 +317,7 @@ TEST(PolicyTargetTest, WinstaPolicy) {
       broker->SpawnTarget(prog_name, arguments.c_str(), policy, &warning_result,
                           &last_error, &temp_process_info);
   base::string16 desktop_name = policy->GetAlternateDesktop();
-  policy->Release();
+  policy = nullptr;
 
   EXPECT_EQ(SBOX_ALL_OK, result);
   if (result == SBOX_ALL_OK)
@@ -349,7 +349,7 @@ TEST(PolicyTargetTest, WinstaPolicy) {
   // Close the desktop handle.
   temp_policy = broker->CreatePolicy();
   temp_policy->DestroyAlternateDesktop();
-  temp_policy->Release();
+  temp_policy = nullptr;
 }
 
 // Launches the app in the sandbox and share a handle with it. The app should
@@ -377,7 +377,7 @@ TEST(PolicyTargetTest, ShareHandleTest) {
   wchar_t prog_name[MAX_PATH];
   GetModuleFileNameW(NULL, prog_name, MAX_PATH);
 
-  TargetPolicy* policy = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> policy = broker->CreatePolicy();
   policy->AddHandleToShare(read_only_view.handle().GetHandle());
 
   base::string16 arguments(L"\"");
@@ -397,7 +397,7 @@ TEST(PolicyTargetTest, ShareHandleTest) {
   result =
       broker->SpawnTarget(prog_name, arguments.c_str(), policy, &warning_result,
                           &last_error, &temp_process_info);
-  policy->Release();
+  policy = nullptr;
 
   EXPECT_EQ(SBOX_ALL_OK, result);
   if (result == SBOX_ALL_OK)

sandbox/win/src/sandbox.h

@@ -21,6 +21,7 @@
 
 #include <windows.h>
 
+#include "base/memory/ref_counted.h"
 #include "sandbox/win/src/sandbox_policy.h"
 #include "sandbox/win/src/sandbox_types.h"
 
@@ -56,7 +57,7 @@ class BrokerServices {
   // Returns the interface pointer to a new, empty policy object. Use this
   // interface to specify the sandbox policy for new processes created by
   // SpawnTarget()
-  virtual TargetPolicy* CreatePolicy() = 0;
+  virtual scoped_refptr<TargetPolicy> CreatePolicy() = 0;
 
   // Creates a new target (child process) in a suspended state.
   // Parameters:
@@ -79,7 +80,7 @@ class BrokerServices {
   //   ALL_OK if successful. All other return values imply failure.
   virtual ResultCode 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) = 0;

sandbox/win/tests/common/controller.cc

@@ -136,15 +136,12 @@ void TestRunner::Init(JobLevel job_level,
 }
 
 TargetPolicy* TestRunner::GetPolicy() {
-  return policy_;
+  return policy_.get();
 }
 
 TestRunner::~TestRunner() {
   if (target_process_.IsValid() && kill_on_destruction_)
     ::TerminateProcess(target_process_.Get(), 0);
-
-  if (policy_)
-    policy_->Release();
 }
 
 bool TestRunner::AddRule(TargetPolicy::SubSystem subsystem,

sandbox/win/tests/common/controller.h

@@ -139,7 +139,7 @@ class TestRunner {
   int InternalRunTest(const wchar_t* command);
 
   BrokerServices* broker_;
-  TargetPolicy* policy_;
+  scoped_refptr<TargetPolicy> policy_;
   DWORD timeout_;
   SboxTestsState state_;
   bool is_init_;