Do not launch rename process directly from system-level Chrome.

Do not launch rename process directly from system-level Chrome.

It is useless to launch a rename process directly from a system-level
Chrome. That process can't rename the Chrome executables since it isn't
elevated.

BUG=650981
Committed: https://crrev.com/e9c97a0e4aa14a2473395a5c8dade22ce91c5729
Cr-Commit-Position: refs/heads/master@{#422760}

[fdoray, 2016-09-28 18:41:22]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-09-28 18:41:22]

grt@: You can take a first look and tell me if you think it's a good approach.

Disclaimer: Haven't tested that it fixes the bug yet, but I think it should.

[grt (UTC plus 2), 2016-09-28 19:52:38]

On 2016/09/28 18:41:22, fdoray wrote:
> grt@: You can take a first look and tell me if you think it's a good approach.
> 
> Disclaimer: Haven't tested that it fixes the bug yet, but I think it should.

IIUC, this avoids the crash in setup.exe by not running it at medium integrity
in this one case. Since there are other ways setup.exe could be run at medium
integrity (such as developers forgetting to switch to an elevated cmd prompt
when running with --system-level), I think it would be better to make the
singleton gracefully fail in the face of access denied. This brings us back to
the old behavior: setup.exe would harmlessly fail rather than crash. What do you
think?

[fdoray, 2016-09-28 20:05:38]

On 2016/09/28 19:52:38, grt (UTC plus 2) wrote:
> On 2016/09/28 18:41:22, fdoray wrote:
> > grt@: You can take a first look and tell me if you think it's a good
approach.
> > 
> > Disclaimer: Haven't tested that it fixes the bug yet, but I think it should.
> 
> IIUC, this avoids the crash in setup.exe by not running it at medium integrity
> in this one case. Since there are other ways setup.exe could be run at medium
> integrity (such as developers forgetting to switch to an elevated cmd prompt
> when running with --system-level), I think it would be better to make the
> singleton gracefully fail in the face of access denied. This brings us back to
> the old behavior: setup.exe would harmlessly fail rather than crash. What do
you
> think?

I think we should do these 3 things:
1. Not launching rename processes for system-level installs without elevated
privileges (this CL).
2. Making SetupSingleton fail gracefully when an error occurs during creation of
mutexes and events.
3. Creating mutexes and events with security attributes that allow access to
every user to get rid of ERROR_ACCESS_DENIED.

[grt (UTC plus 2), 2016-09-28 20:36:13]

On 2016/09/28 20:05:38, fdoray wrote:
> On 2016/09/28 19:52:38, grt (UTC plus 2) wrote:
> > On 2016/09/28 18:41:22, fdoray wrote:
> > > grt@: You can take a first look and tell me if you think it's a good
> approach.
> > > 
> > > Disclaimer: Haven't tested that it fixes the bug yet, but I think it
should.
> > 
> > IIUC, this avoids the crash in setup.exe by not running it at medium
integrity
> > in this one case. Since there are other ways setup.exe could be run at
medium
> > integrity (such as developers forgetting to switch to an elevated cmd prompt
> > when running with --system-level), I think it would be better to make the
> > singleton gracefully fail in the face of access denied. This brings us back
to
> > the old behavior: setup.exe would harmlessly fail rather than crash. What do
> you
> > think?
> 
> I think we should do these 3 things:
> 1. Not launching rename processes for system-level installs without elevated
> privileges (this CL).
> 2. Making SetupSingleton fail gracefully when an error occurs during creation
of
> mutexes and events.
> 3. Creating mutexes and events with security attributes that allow access to
> every user to get rid of ERROR_ACCESS_DENIED.

Part of me wants these to be done in this order: 2, 3, 1. #2 gets us to where we
were before, #3 (for system-level operations only) makes the singleton work as
desired, and #1 should be a noop. I can see that doing #1 quickly is a way to
stop the crashing, but it's the part of this I like the least since it isn't
addressing the real issue. How difficult is #2?

[fdoray, 2016-10-03 20:31:52]

I suggest that we do this, but not https://codereview.chromium.org/2385673003/

[grt (UTC plus 2), 2016-10-04 06:42:37]

lgtm

[fdoray, 2016-10-04 11:28:10]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-04 11:28:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 12:05:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-04 12:07:53]

Description was changed from

==========
Do not launch rename process directly from system-level Chrome.

It is useless to launch a rename process directly from a system-level
Chrome. That process can't rename the Chrome executables since it isn't
elevated.

BUG=650981
==========

to

==========
Do not launch rename process directly from system-level Chrome.

It is useless to launch a rename process directly from a system-level
Chrome. That process can't rename the Chrome executables since it isn't
elevated.

BUG=650981

Committed: https://crrev.com/e9c97a0e4aa14a2473395a5c8dade22ce91c5729
Cr-Commit-Position: refs/heads/master@{#422760}
==========

[commit-bot: I haz the power, 2016-10-04 12:07:53]

Patchset 1 (id:??) landed as
https://crrev.com/e9c97a0e4aa14a2473395a5c8dade22ce91c5729
Cr-Commit-Position: refs/heads/master@{#422760}