Add quotes around executable pathes in the mini installer

Add quotes around executable pathes in the mini installer

BUG=340387
TEST=N/A

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250500

[tommi (sloooow) - chröme, 2014-02-04 20:48:52]

Is there a chance that the string may be stored quoted?

[Cris Neckar, 2014-02-04 20:54:19]

On 2014/02/04 20:48:52, tommi wrote:
> Is there a chance that the string may be stored quoted?

Nope, the only calls are when building a command line to be executed.

[tommi (sloooow) - chröme, 2014-02-04 21:55:13]

https://codereview.chromium.org/154113004/diff/1/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/154113004/diff/1/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:235: unquoted_path,
I mean here.  Since unquoted_path will contain a value read from the registry,
how can you know that it is guaranteed to be unquoted?  I don't see any checks
for it.

What I'm being paranoid about is if there are any changes made to the code that
sets this value and someone "fixes" the quote problem on that side, not
realizing the effect it might have on this code.
Would it make sense to copy unquoted_path directly over to path if
unquoted_path[0] == '\"'?

[Cris Neckar, 2014-02-04 22:53:05]

https://codereview.chromium.org/154113004/diff/1/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/154113004/diff/1/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:235: unquoted_path,
On 2014/02/04 21:55:14, tommi wrote:
> I mean here.  Since unquoted_path will contain a value read from the registry,
> how can you know that it is guaranteed to be unquoted?  I don't see any checks
> for it.
> 
> What I'm being paranoid about is if there are any changes made to the code
that
> sets this value and someone "fixes" the quote problem on that side, not
> realizing the effect it might have on this code.
> Would it make sense to copy unquoted_path directly over to path if
> unquoted_path[0] == '\"'?

Ah I misunderstood, thought you were asking if this code saves the path
somewhere after quoting it. 

Done.

[grt (UTC plus 2), 2014-02-05 03:13:50]

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:226: bool
GetSetupExePathForGuidFromRegistry(bool system_level,
please rename this to GetQuotedSetupExePathForGuidFromRegistry and update the
comment above

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:232: wchar_t
unquoted_path[MAX_PATH] = {0};
{0} -> {}

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:236: MAX_PATH) !=
ERROR_SUCCESS)) {
MAX_PATH -> arraysize(unquoted_path)

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:243: SafeStrCat(path, size,
unquoted_path) &&
nit: either four-space indent, or add parens for:
  return (blah &&
          bletch &&
          phlam);

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:248: // string.  |size| is
measured in wchar_t units.
please rename this to GetQuotedSetupExePathFromRegistry and update the comment
above

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:287: if
(!::CreateProcess(exe_path, cmdline, NULL, NULL, FALSE, CREATE_NO_WINDOW,
if the intent of this CL is to mitigate against bugs resulting from the path to
setup containing spaces, then a better approach would be to pass the path to
setup.exe into this function as |exe_path|.

[Cris Neckar, 2014-02-06 20:56:23]

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:226: bool
GetSetupExePathForGuidFromRegistry(bool system_level,
On 2014/02/05 03:13:50, grt wrote:
> please rename this to GetQuotedSetupExePathForGuidFromRegistry and update the
> comment above

Done.

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:232: wchar_t
unquoted_path[MAX_PATH] = {0};
On 2014/02/05 03:13:50, grt wrote:
> {0} -> {}

Done.

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:236: MAX_PATH) !=
ERROR_SUCCESS)) {
On 2014/02/05 03:13:50, grt wrote:
> MAX_PATH -> arraysize(unquoted_path)

Done.

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:243: SafeStrCat(path, size,
unquoted_path) &&
On 2014/02/05 03:13:50, grt wrote:
> nit: either four-space indent, or add parens for:
>   return (blah &&
>           bletch &&
>           phlam);

Done.

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:248: // string.  |size| is
measured in wchar_t units.
On 2014/02/05 03:13:50, grt wrote:
> please rename this to GetQuotedSetupExePathFromRegistry and update the comment
> above

Done.

https://codereview.chromium.org/154113004/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:287: if
(!::CreateProcess(exe_path, cmdline, NULL, NULL, FALSE, CREATE_NO_WINDOW,
On 2014/02/05 03:13:50, grt wrote:
> if the intent of this CL is to mitigate against bugs resulting from the path
to
> setup containing spaces, then a better approach would be to pass the path to
> setup.exe into this function as |exe_path|.

I like this way better. Done.

[tommi (sloooow) - chröme, 2014-02-06 21:00:57]

lgtm

[Cris Neckar, 2014-02-06 21:51:50]

The CQ bit was checked by cdn@chromium.org

[Cris Neckar, 2014-02-06 21:51:59]

The CQ bit was unchecked by cdn@chromium.org

[Cris Neckar, 2014-02-06 21:52:34]

On 2014/02/06 21:51:59, Cris Neckar wrote:
> The CQ bit was unchecked by mailto:cdn@chromium.org

grt@ I'll wait on your review to commit. But I changed this to do what you
suggested as it is cleaner

[grt (UTC plus 2), 2014-02-07 19:49:23]

Patch Set 3 LGTM. Please test manually with a component=static_library build
since the automated mini_install tests aren't yet being run on trybots or cq.

[Cris Neckar, 2014-02-10 21:34:04]

On 2014/02/07 19:49:23, grt wrote:
> Patch Set 3 LGTM. Please test manually with a component=static_library build
> since the automated mini_install tests aren't yet being run on trybots or cq.

Per chat with Greg we are going to land and hope :)

[Cris Neckar, 2014-02-10 21:34:10]

The CQ bit was checked by cdn@chromium.org

[commit-bot: I haz the power, 2014-02-10 21:37:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/154113004/130001

[commit-bot: I haz the power, 2014-02-10 21:51:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 21:51:25]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Cris Neckar, 2014-02-11 18:47:07]

The CQ bit was checked by cdn@chromium.org

[commit-bot: I haz the power, 2014-02-11 18:47:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/154113004/130001

[commit-bot: I haz the power, 2014-02-11 21:03:19]

Change committed as 250500