Use PathService in installer::GetModulePath().

chrome/installer/util/module_util_win.cc

 // Copyright (c) 2015 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 <windows.h>
 
 #include "chrome/installer/util/module_util_win.h"
 
 #include "base/file_version_info.h"
 #include "base/files/file.h"
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/version.h"
 
 namespace installer {
 
 namespace {
 
 // Returns the directory in which the currently running executable resides.
 base::FilePath GetExecutableDir() {
-  base::char16 path[MAX_PATH];
-  ::GetModuleFileNameW(nullptr, path, MAX_PATH);
-  return base::FilePath(path).DirName();
+  base::char16 path_buffer[MAX_PATH];
+  ::GetModuleFileNameW(nullptr, path_buffer, MAX_PATH);
+  base::FilePath path(path_buffer);
+
+  // When debugging Chrome from the gyp-generated Visual Studio solution, the

[gab, 2015/11/12 18:10:30]

You mean non-ninja builds? Those are not supported AFAIK.

Otherwise, our official builds use gyp-generated builds? Or is this Debug only?

(Not sure I understand why you're seeing this, but it isn't a broader issue)

[fdoray, 2015/11/12 18:27:23]

When GYP_GENERATORS is set to "ninja,msvs-ninja", a chrome.sln file is
generated. If you click "Local Windows Debugger" to launch Chrome in the VS
debugger, ::GetModuleFileNameW() (above) returns a path with a traversal
component "..", and ModuleCanBeRead() (below) fails because base::File doesn't
support path traversal components. This bug was reported to me here: 
crbug.com/554514

Non-ninja builds are not supported, but generating a VS solution to edit
files/debug chrome seems to still be supported
(https://code.google.com/p/chromium/codesearch#chromium/src/docs/ninja_build.m...).

+  // value returned by ::GetModuleFileNameW can contain a path traversal '..'
+  // component, which isn't supported by base::File.
+  if (path.ReferencesParent())
+    path = base::MakeAbsoluteFilePath(path);
+
+  return path.DirName();
 }
 
 // Returns the version in the current module's version resource or the empty
 // string if none found.
 base::string16 GetCurrentModuleVersion() {
   scoped_ptr<FileVersionInfo> file_version_info(
       CREATE_FILE_VERSION_INFO_FOR_CURRENT_MODULE());
   if (file_version_info.get()) {
     base::string16 version_string(file_version_info->file_version());
     if (Version(base::UTF16ToASCII(version_string)).IsValid())
       return version_string;
   }
   return base::string16();
 }
 
 // Indicates whether a file can be opened using the same flags that
 // ::LoadLibrary() uses to open modules.
 bool ModuleCanBeRead(const base::FilePath file_path) {
   return base::File(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ)
       .IsValid();
 }
 
 }  // namespace
 
 base::FilePath GetModulePath(base::StringPiece16 module_name,
                              base::string16* version) {
   DCHECK(version);
 
   base::FilePath module_dir = GetExecutableDir();

[Peter Kasting, 2015/11/16 20:25:43]

I think if we just replace this with something like "PathService::Get(DIR_EXE,
&module_dir);", we can eliminate the helper function here entirely and fix the
bug in question in the process.

This is clearer, shorter, safer, and more like what other code in Chromium does,
so this seems like a clear win.

[fdoray, 2015/11/16 21:31:36]

Done.

   base::FilePath module = module_dir.Append(module_name);
   if (ModuleCanBeRead(module))
     return module;
 
   base::string16 version_string(GetCurrentModuleVersion());
   if (version_string.empty()) {
     LOG(ERROR) << "No valid Chrome version found";
     return base::FilePath();
   }
   *version = version_string;
   return module_dir.Append(version_string).Append(module_name);
 }
 
 }  // namespace installer