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 <wchar.h>
+#include <windows.h>
 
+#include <memory>
 #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 66 matching lines @@
   // local experimentation validates this simplified and *much* faster approach:
   // [1] Sysinternals RamMap no longer lists these files as cached afterwards.
   // [2] Telemetry performance test startup.cold.blank_page reports sane values.
   BY_HANDLE_FILE_INFORMATION bhi = {0};
   CHECK(::GetFileInformationByHandle(file_handle.Get(), &bhi));
   CHECK(::SetFileTime(file_handle.Get(), &bhi.ftCreationTime,
                       &bhi.ftLastAccessTime, &bhi.ftLastWriteTime));
   return true;
 }
 
+// Deny |permission| on the file |path|, for the current user.
+bool DenyFilePermission(const FilePath& path, DWORD permission) {
+  PACL old_dacl;
+  PSECURITY_DESCRIPTOR security_descriptor;
+
+  int path_size = path.value().size();

[Peter Kasting, 2016/07/11 02:35:53]

Should be size_t.

[WC Leung, 2016/07/18 09:41:35]

Done.

+  std::unique_ptr<TCHAR[]> path_ptr(new TCHAR[path_size + 1]);

[Peter Kasting, 2016/07/11 02:35:53]

Nit: Prefer "= base::MakeUnique" to raw new.

[WC Leung, 2016/07/18 09:41:35]

Done. Thanks for making base::MakeUnique known to me.

+  wcsncpy(path_ptr.get(), path.value().c_str(), path_size + 1);
+  path_ptr[path_size] = L'\0';

[Peter Kasting, 2016/07/11 02:35:53]

I don't see why this line is needed, since the source string is shorter than the
third arg above and thus wcsncpy() should always append the trailing null
(shouldn't it?).

[WC Leung, 2016/07/18 09:41:35]

I'm super-paranoid here because a missing '\0' causes a security problem instead
of a normal bug. So I don't trust the weak guarantee above. :-)

Here's my concern:
1. the string length may not be calculated correctly (either due to my
carelessness or someone else carelessly modifying my code).
2. wcsncpy() isn't coded by us. Basically we're trusting the code in MSVC, gcc
and clang, and these aren't tested by us.

[Peter Kasting, 2016/07/18 17:34:30]

I'm opposed to adding something to account for (2).  wcsncpy() is part of the
language's library.  We trust that we compile on platforms where the library is
implemented correctly.  Otherwise we'd need checks all over the codebase. 
Readers will likewise assume that the library functions work and wonder why you
have this.

For (1), I don't see a way that you were careless and miscalculated the path
length.  That leaves future code modifiers.  The right way to handle such cases,
if they need be handled at all, is with a DCHECK that there's no problem to be
handled, not by actually handling the (phantom) problem.  But I wouldn't bother
with this either.  After all, if a future modifier might change the path
calculation to be in error, they might remove a DCHECK as well, or do any other
silly thing.  So trying to plan for this makes no sense to me.  We should assume
that people won't introduce bugs into the internals of functions when they're
currently correct and there's no motivating reason why someone would be prone to
make that mistake.

In the meantime, having the termination happen here is really misleading,
because it makes it look like wcsncpy() doesn't null-terminate, which will make
a reader go start checking any other such calls in the codebase.  We don't want
people misled about how library functions work.

[WC Leung, 2016/07/19 08:00:12]

I see. I do buy in the readability part. So the line is removed.

+
+  if (GetNamedSecurityInfo(path_ptr.get(), SE_FILE_OBJECT,
+                           DACL_SECURITY_INFORMATION, nullptr, nullptr,
+                           &old_dacl, nullptr,
+                           &security_descriptor) != ERROR_SUCCESS) {
+    return false;
+  }
+
+  LPTSTR current_user = L"CURRENT_USER";
+  EXPLICIT_ACCESS new_access = {
+      permission,
+      DENY_ACCESS,
+      0,
+      {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER,
+       current_user}};
+
+  PACL new_dacl;
+  if (SetEntriesInAcl(1, &new_access, old_dacl, &new_dacl) != ERROR_SUCCESS) {
+    LocalFree(security_descriptor);
+    return false;
+  }
+
+  DWORD rc = SetNamedSecurityInfo(path_ptr.get(), SE_FILE_OBJECT,
+                                  DACL_SECURITY_INFORMATION, nullptr, nullptr,
+                                  new_dacl, nullptr);
+  LocalFree(security_descriptor);
+  LocalFree(new_dacl);
+
+  return rc == ERROR_SUCCESS;
+}
+
 // Checks if the volume supports Alternate Data Streams. This is required for
 // the Zone Identifier implementation.
 bool VolumeSupportsADS(const FilePath& path) {
   wchar_t drive[MAX_PATH] = {0};
   wcscpy_s(drive, MAX_PATH, path.value().c_str());
 
   if (!PathStripToRootW(drive))
     return false;
 
   DWORD fs_flags = 0;
@@ skipping 44 matching lines @@
   DCHECK(info_ != NULL);
   DCHECK_NE(0u, length_);
 }
 
 FilePermissionRestorer::~FilePermissionRestorer() {
   if (!RestorePermissionInfo(path_, info_, length_))
     NOTREACHED();
 }
 
 }  // namespace base