Fix crash when switching to a profile that cannot be opened

base/test/test_file_util_win.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 "base/test/test_file_util.h"
 
 #include <windows.h>
 #include <aclapi.h>
 #include <shlwapi.h>
 #include <stddef.h>
 
 #include <vector>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/strings/string_split.h"
 #include "base/threading/platform_thread.h"
 #include "base/win/scoped_handle.h"
 
 namespace base {
 
 namespace {
 
 struct PermissionInfo {
   PSECURITY_DESCRIPTOR security_descriptor;
   ACL dacl;
 };
 
-// Deny |permission| on the file |path|, for the current user.
-bool DenyFilePermission(const FilePath& path, DWORD permission) {
-  PACL old_dacl;
-  PSECURITY_DESCRIPTOR security_descriptor;
-  if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()),
-                           SE_FILE_OBJECT,
-                           DACL_SECURITY_INFORMATION, NULL, NULL, &old_dacl,
-                           NULL, &security_descriptor) != ERROR_SUCCESS) {
-    return false;
-  }
-
-  EXPLICIT_ACCESS change;
-  change.grfAccessPermissions = permission;
-  change.grfAccessMode = DENY_ACCESS;
-  change.grfInheritance = 0;
-  change.Trustee.pMultipleTrustee = NULL;
-  change.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
-  change.Trustee.TrusteeForm = TRUSTEE_IS_NAME;
-  change.Trustee.TrusteeType = TRUSTEE_IS_USER;
-  change.Trustee.ptstrName = const_cast<wchar_t*>(L"CURRENT_USER");
-
-  PACL new_dacl;
-  if (SetEntriesInAcl(1, &change, old_dacl, &new_dacl) != ERROR_SUCCESS) {
-    LocalFree(security_descriptor);
-    return false;
-  }
-
-  DWORD rc = SetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()),
-                                  SE_FILE_OBJECT, DACL_SECURITY_INFORMATION,
-                                  NULL, NULL, new_dacl, NULL);
-  LocalFree(security_descriptor);
-  LocalFree(new_dacl);
-
-  return rc == ERROR_SUCCESS;
-}
-
 // Gets a blob indicating the permission information for |path|.
 // |length| is the length of the blob.  Zero on failure.
 // Returns the blob pointer, or NULL on failure.
 void* GetPermissionInfo(const FilePath& path, size_t* length) {
   DCHECK(length != NULL);
   *length = 0;
   PACL dacl = NULL;
   PSECURITY_DESCRIPTOR security_descriptor;
   if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()),
                            SE_FILE_OBJECT,
@@ skipping 28 matching lines @@
   LocalFree(perm->security_descriptor);
 
   char* char_array = reinterpret_cast<char*>(info);
   delete [] char_array;
 
   return rc == ERROR_SUCCESS;
 }
 
 }  // namespace
 
+// Deny |permission| on the file |path|, for the current user.

[Peter Kasting, 2016/06/16 07:02:10]

Nit: Definition order in .cc file must match declaration order in .h.

[WC Leung, 2016/07/04 18:45:37]

Acknowledged.

+bool DenyFilePermission(const FilePath& path, DWORD permission) {
+  PACL old_dacl;
+  PSECURITY_DESCRIPTOR security_descriptor;
+  if (GetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()),

[Peter Kasting, 2016/06/16 07:02:10]

My comments about const_cast stand (3 places).  In the case of the fixed
constant below this is easy, you can just declare a local wchar_t* = L"..."
directly.  The other two places you will need to declare a wchar_t[] on the
stack with sufficient size to hold path.value(), then copy in with wcscpy(). 
(Or use wcscpy_s() if you want to be really safe.)

[WC Leung, 2016/07/04 18:45:37]

Thanks a lot. I'll use wcsncpy() because
- wcscpy() is too dangerous for possible buffer overruns.
- wcscpy_s() is not available if __STDC_WANT_LIB_EXT1__ is not defined. But to
define it it seems we need to modify the whole chromium source tree.

BTW, do you know why that LPTSTR was not an LPCTSTR at the first place?

+                           SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr,
+                           nullptr, &old_dacl, nullptr,
+                           &security_descriptor) != ERROR_SUCCESS) {
+    return false;
+  }
+
+  EXPLICIT_ACCESS new_access = {
+      permission,
+      DENY_ACCESS,
+      0,
+      {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER,
+       const_cast<wchar_t*>(L"CURRENT_USER")}};
+
+  PACL new_dacl;
+  if (SetEntriesInAcl(1, &new_access, old_dacl, &new_dacl) != ERROR_SUCCESS) {
+    LocalFree(security_descriptor);

[Peter Kasting, 2016/06/16 07:02:10]

I assume you decided to investigate the scoping object in a separate CL?

[WC Leung, 2016/06/16 10:04:10]

Yes.

[WC Leung, 2016/06/20 16:54:08]

Created issue 621559 to track this.

+    return false;
+  }
+
+  DWORD rc = SetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()),
+                                  SE_FILE_OBJECT, DACL_SECURITY_INFORMATION,
+                                  nullptr, nullptr, new_dacl, nullptr);
+  LocalFree(security_descriptor);
+  LocalFree(new_dacl);
+
+  return rc == ERROR_SUCCESS;
+}
+
 bool DieFileDie(const FilePath& file, bool recurse) {
   // It turns out that to not induce flakiness a long timeout is needed.
   const int kIterations = 25;
   const TimeDelta kTimeout = TimeDelta::FromSeconds(10) / kIterations;
 
   if (!PathExists(file))
     return true;
 
   // Sometimes Delete fails, so try a few more times. Divide the timeout
   // into short chunks, so that if a try succeeds, we won't delay the test
@@ skipping 82 matching lines @@
   DCHECK(info_ != NULL);
   DCHECK_NE(0u, length_);
 }
 
 FilePermissionRestorer::~FilePermissionRestorer() {
   if (!RestorePermissionInfo(path_, info_, length_))
     NOTREACHED();
 }
 
 }  // namespace base