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.
+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/13 04:43:20]

Nit: Avoid const_cast. Store this in a local and pass the address of the local.
(2 locals covering 3 places)

[WC Leung, 2016/06/13 08:06:45]

Acknowledged.

[WC Leung, 2016/06/15 13:08:38]

Do you know of any good method to copy the contents a std::wstring into a local
C string?

+                           SE_FILE_OBJECT,
+                           DACL_SECURITY_INFORMATION, NULL, NULL, &old_dacl,

[Peter Kasting, 2016/06/13 04:43:20]

Nit: nullptr (several places)

[WC Leung, 2016/06/13 08:06:45]

Acknowledged.

[WC Leung, 2016/06/15 13:08:38]

Done.

+                           NULL, &security_descriptor) != ERROR_SUCCESS) {
+    return false;
+  }
+
+  EXPLICIT_ACCESS change;

[Peter Kasting, 2016/06/13 04:43:20]

Stolen from my comments on one of your previous CLs:

Nit: Try to use initializer values if possible, e.g.:

  EXPLICIT_ACCESS explicit_access =
      {permission, DENY_ACCESS, 0,
       {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER,
        trustee}};

(Not sure how clang-format would wrap/indent this)

[WC Leung, 2016/06/13 08:06:45]

Pawel: WDYT about the proposed changes? I'll let you decide on this one.
(Anyway, both the current way and the initializer values looks good to me.)

[WC Leung, 2016/06/15 13:08:38]

Done. Pawel: please see if this is good.

On 2016/06/13 08:06:45, WC Leung wrote:

+  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);

[Peter Kasting, 2016/06/13 04:43:20]

Stolen from my comments on one of your previous CLs:

Consider creating a scoping object that frees these automatically to avoid the
possibility of forgetting to free on exit/later refactoring breaking this.

This is more compelling if added to base/win as another ScopedXXX object, and
then used across the rest of the codebase where LocalFrees are currently
performed, in which case you'd want to split this to a separate CL.  The class
could be something like ScopedLocal<T> where T is e.g. ACL or
PSECURITY_DESCRIPTOR, and there should be a Receive() method as on
ScopedComPtr() to allow initialization via outparam.

[WC Leung, 2016/06/13 08:06:45]

SGTM. BTW, I'll check against the existing ScopedXXX objects before adding one.

+    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;
+}
+
 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