Refactor and reset acl for ServiceUtilCleanerRunningServiceTest

https://ci.chromium.org/p/chromium/builders/ci/Win7%20Tests%20%28dbg%29%281%29
https://ci.chromium.org/p/chromium/builders/ci/ToTWin%28dll%29
They don't fail anymore for CleanerSandboxInterfaceTest. So consider the fix
works.
I don't see it's necessary to make the resetacl more general(i.e. pass in base::FilePath)
for now. But it is in a general place. Let me know if a general
signature is favorable.

Bug: 956016
Change-Id: I870e01dfa62a09ada372985dc5d9c9dcdc01b328
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1956145
Commit-Queue: Sven Zheng <svenzheng@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722661}

Author: Sven Zheng

chrome/chrome_cleaner/engines/broker/cleaner_sandbox_interface_unittest.cc

@@ -18,7 +18,6 @@
 #include "base/path_service.h"
 #include "base/process/kill.h"
 #include "base/process/process.h"
-#include "base/process/launch.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string16.h"
 #include "base/test/task_environment.h"
@@ -771,32 +770,11 @@ TEST_F(CleanerInterfaceRegistryTest, NtChangeRegistryValue_AllowNormalization) {
                                            normalize_all_values));
 }
 
-// On Windows, sometimes the copied files don't have correct ACLs.
-// So we reset ACL before running the test.
-// See crbug.com/956016.
-void ResetACLs() {
-  base::FilePath exe_path;
-  ASSERT_TRUE(base::PathService::Get(base::BasePathKey::DIR_EXE, &exe_path));
-  base::FilePath abs_path = base::MakeAbsoluteFilePath(exe_path);
-#ifdef NDEBUG
-  base::FilePath ucrt_path = abs_path.Append(L"ucrtbase.dll");
-#else
-  base::FilePath ucrt_path = abs_path.Append(L"ucrtbased.dll");
-#endif
-  base::CommandLine cmd({L"icacls"});
-  cmd.AppendArgPath(ucrt_path);
-  cmd.AppendArg("/reset");
-  cmd.AppendArg("/t");
-  base::Process process = base::LaunchProcess(cmd, base::LaunchOptionsForTest());
-  int exit_code = 0;
-  ASSERT_TRUE(process.WaitForExitWithTimeout(TestTimeouts::action_timeout(), &exit_code));
-  ASSERT_EQ(exit_code, 0);
-}
-
-class CleanerSandboxInterfaceTest : public ::testing::Test {
+class CleanerSandboxInterfaceRunningServiceTest : public ::testing::Test {
  public:
   static void SetUpTestCase() {
-    ResetACLs();
+    // Tests calling StartService() need this.
+    ASSERT_TRUE(chrome_cleaner::ResetAclForUcrtbase());
   }
 };
 
@@ -817,7 +795,7 @@ TEST(CleanerSandboxInterface, DeleteService_Success) {
   EXPECT_FALSE(chrome_cleaner::DoesServiceExist(service_handle.service_name()));
 }
 
-TEST_F(CleanerSandboxInterfaceTest, DeleteService_Running) {
+TEST_F(CleanerSandboxInterfaceRunningServiceTest, DeleteService_Running) {
   ASSERT_TRUE(chrome_cleaner::EnsureNoTestServicesRunning());
 
   chrome_cleaner::TestScopedServiceHandle service_handle;
@@ -830,7 +808,7 @@ TEST_F(CleanerSandboxInterfaceTest, DeleteService_Running) {
   EXPECT_FALSE(chrome_cleaner::DoesServiceExist(service_handle.service_name()));
 }
 
-TEST_F(CleanerSandboxInterfaceTest, DeleteService_HandleHeld) {
+TEST_F(CleanerSandboxInterfaceRunningServiceTest, DeleteService_HandleHeld) {
   ASSERT_TRUE(chrome_cleaner::EnsureNoTestServicesRunning());
 
   chrome_cleaner::TestScopedServiceHandle service_handle;

chrome/chrome_cleaner/os/system_util_cleaner_unittest.cc

@@ -49,8 +49,13 @@ namespace chrome_cleaner {
 
 namespace {
 
-class ServiceUtilCleanerTest : public testing::Test {
+class ServiceUtilCleanerRunningServiceTest : public testing::Test {
  public:
+  static void SetUpTestCase() {
+    // Tests calling StartService() need this.
+    ASSERT_TRUE(chrome_cleaner::ResetAclForUcrtbase());
+  }
+
   void SetUp() override {
     // Cleanup previous run. This may happen when previous execution of unittest
     // crashed, leaving background processes/services.
@@ -74,7 +79,7 @@ TEST(SystemUtilCleanerTests, OpenRegistryKeyWithInvalidParameter) {
   EXPECT_FALSE(key_path.Open(KEY_READ, &key));
 }
 
-TEST_F(ServiceUtilCleanerTest, DeleteService) {
+TEST_F(ServiceUtilCleanerRunningServiceTest, DeleteService) {
   TestScopedServiceHandle service_handle;
   ASSERT_TRUE(service_handle.InstallService());
   service_handle.Close();
@@ -85,8 +90,7 @@ TEST_F(ServiceUtilCleanerTest, DeleteService) {
   EXPECT_FALSE(DoesServiceExist(service_handle.service_name()));
 }
 
-// Disabled: https://crbug.com/956016
-TEST_F(ServiceUtilCleanerTest, DISABLED_StopAndDeleteRunningService) {
+TEST_F(ServiceUtilCleanerRunningServiceTest, StopAndDeleteRunningService) {
   // Install and launch the service.
   TestScopedServiceHandle service_handle;
   ASSERT_TRUE(service_handle.InstallService());
@@ -109,8 +113,7 @@ TEST_F(ServiceUtilCleanerTest, DISABLED_StopAndDeleteRunningService) {
   EXPECT_FALSE(IsProcessRunning(kTestServiceExecutableName));
 }
 
-// Disabled: https://crbug.com/956016
-TEST_F(ServiceUtilCleanerTest, DISABLED_DeleteRunningService) {
+TEST_F(ServiceUtilCleanerRunningServiceTest, DeleteRunningService) {
   // Install and launch the service.
   TestScopedServiceHandle service_handle;
   ASSERT_TRUE(service_handle.InstallService());
@@ -128,7 +131,7 @@ TEST_F(ServiceUtilCleanerTest, DISABLED_DeleteRunningService) {
   EXPECT_FALSE(IsProcessRunning(kTestServiceExecutableName));
 }
 
-TEST_F(ServiceUtilCleanerTest, QuarantineFolderPermission) {
+TEST_F(ServiceUtilCleanerRunningServiceTest, QuarantineFolderPermission) {
   base::ScopedPathOverride local_app_data_override(
       CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA));
 
@@ -174,7 +177,7 @@ TEST_F(ServiceUtilCleanerTest, QuarantineFolderPermission) {
   ::LocalFree(security_descriptor);
 }
 
-TEST_F(ServiceUtilCleanerTest, DefaultQuarantineFolderPath) {
+TEST_F(ServiceUtilCleanerRunningServiceTest, DefaultQuarantineFolderPath) {
   base::ScopedPathOverride local_app_data_override(
       CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA));
 
@@ -187,7 +190,7 @@ TEST_F(ServiceUtilCleanerTest, DefaultQuarantineFolderPath) {
   EXPECT_EQ(quarantine_path, default_path);
 }
 
-TEST_F(ServiceUtilCleanerTest, SpecifiedQuarantineFolderPath) {
+TEST_F(ServiceUtilCleanerRunningServiceTest, SpecifiedQuarantineFolderPath) {
   // Override the default path of local appdata, so if we fail to redirect the
   // quarantine folder, the test won't drop any file in the real directory.
   base::ScopedPathOverride local_app_data_override(

chrome/chrome_cleaner/test/test_util.cc

@@ -20,12 +20,14 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/launcher/unit_test_launcher.h"
 #include "base/test/test_suite.h"
 #include "base/test/test_switches.h"
+#include "base/test/test_timeouts.h"
 #include "base/time/time.h"
 #include "base/win/scoped_com_initializer.h"
 #include "base/win/windows_version.h"
@@ -410,4 +412,35 @@ bool CheckTestPrivileges() {
   return true;
 }
 
+bool ResetAclForUcrtbase() {
+  base::FilePath exe_path;
+  if (!base::PathService::Get(base::BasePathKey::DIR_EXE, &exe_path)) {
+    LOG(ERROR) << "Failed to get directory path.";
+    return false;
+  }
+  base::FilePath abs_path = base::MakeAbsoluteFilePath(exe_path);
+#ifdef NDEBUG
+  base::FilePath ucrt_path = abs_path.Append(L"ucrtbase.dll");
+#else
+  base::FilePath ucrt_path = abs_path.Append(L"ucrtbased.dll");
+#endif
+  base::CommandLine cmd({L"icacls"});
+  cmd.AppendArgPath(ucrt_path);
+  cmd.AppendArg("/reset");
+  cmd.AppendArg("/t");
+  base::Process process =
+      base::LaunchProcess(cmd, base::LaunchOptionsForTest());
+  int exit_code = 0;
+  if (!process.WaitForExitWithTimeout(TestTimeouts::action_timeout(),
+                                      &exit_code)) {
+    LOG(ERROR) << "Failed to reset acl for file " << ucrt_path.AsUTF16Unsafe();
+    return false;
+  }
+  if (exit_code) {
+    LOG(ERROR) << "Failed to reset acl for file " << ucrt_path.AsUTF16Unsafe()
+               << " with exit code " << exit_code;
+  }
+  return !exit_code;
+}
+
 }  // namespace chrome_cleaner

chrome/chrome_cleaner/test/test_util.h

@@ -175,6 +175,13 @@ class ScopedTempDirNoWow64 : protected base::ScopedTempDir {
 // privileges. Returns false if the privileges could not be made correct.
 bool CheckTestPrivileges();
 
+// On Windows, sometimes the copied files don't have correct ACLs.
+// So we reset ACL before running the test.
+// For debug, it will reset ucrtbased.dll. For release, it will reset
+// ucrtbase.dll.
+// See crbug.com/956016.
+bool ResetAclForUcrtbase();
+
 // Accepts PUPData::PUP parameters with id equals to |expected_id|.
 MATCHER_P(PupHasId, expected_id, "") {
   return arg->signature().id == expected_id;