Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1436103005:
Use PathService in installer::GetModulePath(). (Closed)
|
Created:
5 years, 1 month ago by fdoray Modified:
5 years, 1 month ago CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, proberge, Peter Kasting, grt (UTC plus 2) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse PathService in installer::GetModulePath().
When Chrome is launched from the gyp-generated Visual Studio solution,
::GetModuleFileNameW returns a value that contains a path traversal
component. This isn't supported by base::File and causes
installer::GetModulePath to fail.
With this CL, we use base::PathService instead of ::GetModuleFileNameW
to get the path to the current executable. base::PathService
removes all path traversal components.
BUG=554514
Committed: https://crrev.com/28753d2c5f94b74849997b6cba3fd43df9fc1964
Cr-Commit-Position: refs/heads/master@{#360459}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use base::PathService. #Patch Set 3 : self-review. #
Total comments: 4
Patch Set 4 : remove logging. #
Total comments: 8
Patch Set 5 : add NOTREACHED() #Patch Set 6 : use DCHECK instead of NOTREACHED. #
Total comments: 6
Patch Set 7 : nits from gab #Messages
Total messages: 28 (7 generated)
Description was changed from ========== Eliminate path traversal components in installer::GetModulePath. When Chrome is launched from the gyp-generated Visual Studio solution, ::GetModuleFileNameW returns a value that contains a path traversal component. This isn't supported by base::File and causes installer::GetModulePath to fail. With this CL, path traversal components a resolved before the path returned by ::GetModuleFileNameW is passed to base::File. BUG=554514 ========== to ========== Eliminate path traversal components in installer::GetModulePath. When Chrome is launched from the gyp-generated Visual Studio solution, ::GetModuleFileNameW returns a value that contains a path traversal component. This isn't supported by base::File and causes installer::GetModulePath to fail. With this CL, path traversal components are resolved before the path returned by ::GetModuleFileNameW is passed to base::File. BUG=554514 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org
fdoray@chromium.org changed reviewers: + gab@chromium.org
gab@: Can you review this CL? Thanks.
gab@: Can you review this CL? Thanks.
https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:27: // When debugging Chrome from the gyp-generated Visual Studio solution, the 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)
https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:27: // When debugging Chrome from the gyp-generated Visual Studio solution, the On 2015/11/12 18:10:30, gab wrote: > 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) 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...).
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:62: base::FilePath module_dir = GetExecutableDir(); 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.
Could you take another look? Thanks. https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:62: base::FilePath module_dir = GetExecutableDir(); On 2015/11/16 20:25:43, Peter Kasting wrote: > 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. Done.
Description was changed from ========== Eliminate path traversal components in installer::GetModulePath. When Chrome is launched from the gyp-generated Visual Studio solution, ::GetModuleFileNameW returns a value that contains a path traversal component. This isn't supported by base::File and causes installer::GetModulePath to fail. With this CL, path traversal components are resolved before the path returned by ::GetModuleFileNameW is passed to base::File. BUG=554514 ========== to ========== Use PathService in installer::GetModulePath(). When Chrome is launched from the gyp-generated Visual Studio solution, ::GetModuleFileNameW returns a value that contains a path traversal component. This isn't supported by base::File and causes installer::GetModulePath to fail. With this CL, we use base::PathService instead of ::GetModuleFileNameW to get the path to the current executable. base::PathService removes all path traversal components. BUG=554514 ==========
I'm not an OWNER in this area but here are a few comments anyway. https://codereview.chromium.org/1436103005/diff/40001/chrome/installer/util/m... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/40001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:47: if (!base::PathService::Get(base::DIR_EXE, &module_dir)) { Can this ever really fail? It seems like it should always be possible to get the executable directory. Certainly the old code didn't check for GetModuleFileNameW() failing. https://codereview.chromium.org/1436103005/diff/40001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:48: LOG(ERROR) << "Cannot get current executable directory."; We normally avoid logging statements unless you have a specific plan to gather their data and act on it. In this case I don't think you gain anything from logging here. (I would remove the logging statement below while you're at it.)
pkasting@: Done. gab@: Can you review this CL? Thanks. https://codereview.chromium.org/1436103005/diff/40001/chrome/installer/util/m... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/40001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:47: if (!base::PathService::Get(base::DIR_EXE, &module_dir)) { On 2015/11/16 21:39:00, Peter Kasting wrote: > Can this ever really fail? It seems like it should always be possible to get > the executable directory. Certainly the old code didn't check for > GetModuleFileNameW() failing. It should never fail. https://codereview.chromium.org/1436103005/diff/40001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:48: LOG(ERROR) << "Cannot get current executable directory."; On 2015/11/16 21:39:00, Peter Kasting wrote: > We normally avoid logging statements unless you have a specific plan to gather > their data and act on it. In this case I don't think you gain anything from > logging here. > > (I would remove the logging statement below while you're at it.) Done.
Won't DIR_EXE exhibit the same issue? AFAICT from base_paths_win.cc it also doesn't make the path absolute. FWIW, I do think it's a good idea to do so there though (assuming most callers care about an absolute path) as that value is cached so it's little overhead and benefits multiple callers. Out of scope for this CL, but arguably every callsite that uses ::GetModuleFilenameW should actually use PathService::Get(DIR_EXE) and benefit from Chrome's pre-processed and cached value rather than the raw OS value. https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:46: base::FilePath module_dir; This won't store the module's dir, but the application's dir. s/module_dir/application_dir/ https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:48: return base::FilePath(); Add NOTREACHED(), this should never happen. https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:56: return base::FilePath(); Pretty sure this should also never happen, +NOTREACHED() -- i.e. let the developer know the issue is here instead of hunting for "why chrome doesn't launch" if this ever happens.
On 2015/11/17 14:45:58, gab wrote: > Won't DIR_EXE exhibit the same issue? AFAICT from base_paths_win.cc it also > doesn't make the path absolute. Ah nvm, just saw your post on the bug, good to know that PathService::Get() just fixes this issue at scale :-). > > FWIW, I do think it's a good idea to do so there though (assuming most callers > care about an absolute path) as that value is cached so it's little overhead and > benefits multiple callers. Out of scope for this CL, but arguably every callsite > that uses ::GetModuleFilenameW should actually use PathService::Get(DIR_EXE) and > benefit from Chrome's pre-processed and cached value rather than the raw OS > value. > > https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... > File chrome/installer/util/module_util_win.cc (right): > > https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... > chrome/installer/util/module_util_win.cc:46: base::FilePath module_dir; > This won't store the module's dir, but the application's dir. > > s/module_dir/application_dir/ > > https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... > chrome/installer/util/module_util_win.cc:48: return base::FilePath(); > Add NOTREACHED(), this should never happen. > > https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... > chrome/installer/util/module_util_win.cc:56: return base::FilePath(); > Pretty sure this should also never happen, +NOTREACHED() -- i.e. let the > developer know the issue is here instead of hunting for "why chrome doesn't > launch" if this ever happens.
https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:46: base::FilePath module_dir; On 2015/11/17 14:45:58, gab wrote: > This won't store the module's dir, but the application's dir. > > s/module_dir/application_dir/ This has ramifications for the function name here as well. My suggestion of DIR_EXE was based on what I believed the old code was doing. However, the path service does have separate selectors for the executable dir and the module dir. So perhaps either we should make the code here match the names, or else we should change not only this variable name but the function names (etc.) as well to make it clear this is the executable dir. I don't know the context here well enough to judge; I assume you do. https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:48: return base::FilePath(); On 2015/11/17 14:45:58, gab wrote: > Add NOTREACHED(), this should never happen. If it should never happen, NOTREACHED() wouldn't be appropriate, as that would be handling DCHECK failure (which our style guide bans); instead we'd want to do something like: const bool has_path = base::PathService::Get(base::DIR_EXE, &module_dir); DCHECK(has_path); ... The same comment applies below. (Incidentally, if this truly can't ever fail, I'm very much in favor of adding the DCHECKs. In that case, though, we probably also want to go to the caller(s) and remove code that handles failure, as well as documenting in the header that this routine can't fail.)
Can you take another look? Thanks. https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:46: base::FilePath module_dir; On 2015/11/17 20:24:18, Peter Kasting wrote: > On 2015/11/17 14:45:58, gab wrote: > > This won't store the module's dir, but the application's dir. > > > > s/module_dir/application_dir/ > > This has ramifications for the function name here as well. My suggestion of > DIR_EXE was based on what I believed the old code was doing. However, the path > service does have separate selectors for the executable dir and the module dir. > So perhaps either we should make the code here match the names, or else we > should change not only this variable name but the function names (etc.) as well > to make it clear this is the executable dir. > > I don't know the context here well enough to judge; I assume you do. I changed the variable name, but not the function name. The purpose of this function is to get the path to a module (e.g. chrome.dll or chrome_child.dll), not an executable, so the name is correct. Currently, it is used to get the path to chrome.dll or chrome_child.dll from chrome.exe. We use base::DIR_EXE instead of base::DIR_MODULE to avoid having a different logic depending on whether this is called from chrome.exe or chrome.dll. The current implementation works both from chrome.exe and chrome.dll. https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:48: return base::FilePath(); On 2015/11/17 20:24:18, Peter Kasting wrote: > On 2015/11/17 14:45:58, gab wrote: > > Add NOTREACHED(), this should never happen. > > If it should never happen, NOTREACHED() wouldn't be appropriate, as that would > be handling DCHECK failure (which our style guide bans); instead we'd want to do > something like: > > const bool has_path = base::PathService::Get(base::DIR_EXE, &module_dir); > DCHECK(has_path); > > ... > > The same comment applies below. > > (Incidentally, if this truly can't ever fail, I'm very much in favor of adding > the DCHECKs. In that case, though, we probably also want to go to the caller(s) > and remove code that handles failure, as well as documenting in the header that > this routine can't fail.) base::PathService::Get(base::DIR_EXE, &exe_dir) should never fail, so I added a DCHECK. I cannot change the signature of base::PathService::Get because it can fail with parameters other than base::DIR_EXE (e.g. base::DIR_APP_SHORTCUTS prior to Win8). https://codereview.chromium.org/1436103005/diff/60001/chrome/installer/util/m... chrome/installer/util/module_util_win.cc:56: return base::FilePath(); On 2015/11/17 14:45:58, gab wrote: > Pretty sure this should also never happen, +NOTREACHED() -- i.e. let the > developer know the issue is here instead of hunting for "why chrome doesn't > launch" if this ever happens. It will not fail if the Chrome binary has a valid version number, so I added a DCHECK.
https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... chrome/installer/util/module_util_win.cc:59: DCHECK(!version_string.empty()); This will be the failure mode if we ever botch the build and leave out the version resource, which happens from time to time. What exactly will happen if the string is empty? It would be nice if this were easy to diagnose.
+cc:grt@ https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... chrome/installer/util/module_util_win.cc:59: DCHECK(!version_string.empty()); On 2015/11/17 23:22:58, grt wrote: > This will be the failure mode if we ever botch the build and leave out the > version resource, which happens from time to time. What exactly will happen if > the string is empty? It would be nice if this were easy to diagnose. Before this CL, when the version string was empty or invalid, the startup failed in MainDllLoader::Launch, line 219 [1]. Now, for a debug build, it fails when a DCHECK is hit at line 25 or 27 of this file. For a release build, it can fail when |file_version_info| is dereferenced at line 26 of this file, or in MainDllLoader::Launch like before [1]. Should I remove the DCHECK at line 25 and replace it with if (file_version_info.get()) {}, to avoid crashes when the version string is missing (even though that should never happen for a correct build)? IMO, problems are easier to diagnose in debug builds with this CL. For release builds, nothing changed. What do you think? [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/main_dl...
+cc:grt@
gab@: Can you review this CL? Thanks.
lgtm https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... chrome/installer/util/module_util_win.cc:21: // string if none found. Is this comment still correct? https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... chrome/installer/util/module_util_win.cc:51: base::FilePath module_path = exe_dir.Append(module_name); const
https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... chrome/installer/util/module_util_win.cc:21: // string if none found. On 2015/11/18 23:01:40, gab wrote: > Is this comment still correct? Done. Removed. https://codereview.chromium.org/1436103005/diff/100001/chrome/installer/util/... chrome/installer/util/module_util_win.cc:51: base::FilePath module_path = exe_dir.Append(module_name); On 2015/11/18 23:01:40, gab wrote: > const Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1436103005/#ps120001 (title: "nits from gab")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436103005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436103005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/28753d2c5f94b74849997b6cba3fd43df9fc1964 Cr-Commit-Position: refs/heads/master@{#360459} |
