Add support for alternate window station.

Add support for alternate window station.

TEST: Start chrome, make sure it loads pages, then user process explorer to make sure the WindowStation handle name is not the same as the browser process.

BUG:10996


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=16483

[Nicolas Sylvain, 2009-05-11 14:22:06]

I'm going to make a change to move the HDESK/HWINSTA creation higher up, so it
does not have to be static. I'll upload a new version shortly.  Let me know
which one you prefer.

[Nicolas Sylvain, 2009-05-11 14:44:27]

I'm punting on this right now. I found a problem and it's forcing me to give a
Broker* to Policy, which I really want to avoid. 

So basically, what I wanted to do :

Move CreateAlternateDesktop to BrokerService and then in SpawnTarget, I would
pass the handles to the targetservice.  This wont work because I don't want to
force precreating the handles, to policy->SetAlternateDesktop()  needs to be
able to set the handles too. So Policy would need access to the higher class,
and I don't feel like initializing Policy with the BrokerSerice* is a good idea.
It would work though. If you think we don't care too much about circular
dependencies there, let me know.

On 2009/05/11 14:22:06, Nicolas Sylvain wrote:
> I'm going to make a change to move the HDESK/HWINSTA creation higher up, so it
> does not have to be static. I'll upload a new version shortly.  Let me know
> which one you prefer.

[rvargas (doing something else), 2009-05-11 19:52:46]

Thanks for doing this.

So far mostly nits.

You are not calling DestroyAlternateDesktop(), are you?

http://codereview.chromium.org/113190/diff/1008/1022
File chrome/browser/sandbox_policy.cc (right):

http://codereview.chromium.org/113190/diff/1008/1022#newcode407
Line 407: ::GetThreadDesktop(target.dwThreadId);
This should not be working properly. target.dwThreadId should not be valid for
this process.

http://codereview.chromium.org/113190/diff/1008/1018
File sandbox/src/policy_target_test.cc (right):

http://codereview.chromium.org/113190/diff/1008/1018#newcode273
Line 273: // this should fail if the desktop is really on another window
station.
nit: this -> This

http://codereview.chromium.org/113190/diff/1008/1016
File sandbox/src/sandbox_policy.h (right):

http://codereview.chromium.org/113190/diff/1008/1016#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: update year (also on the other files)

http://codereview.chromium.org/113190/diff/1008/1012
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/113190/diff/1008/1012#newcode52
Line 52: /* static */
nit: c++ comment

http://codereview.chromium.org/113190/diff/1008/1012#newcode392
Line 392: ResultCode PolicyBase::CreateAlternateDesktop(bool
alternate_winstation) {
nit: could you place the two new methods together?

http://codereview.chromium.org/113190/diff/1008/1012#newcode402
Line 402: // Create the window station.
DCHECK that alternate_wsta_handle_ is null.

http://codereview.chromium.org/113190/diff/1008/1012#newcode404
Line 404: return SBOX_ERROR_GENERIC;
nit: use more descriptive error code.

http://codereview.chromium.org/113190/diff/1008/1012#newcode414
Line 414: return SBOX_ERROR_GENERIC;
nit: same here. Compounded by the possibility of critical failure inside
CreateDesktop.

http://codereview.chromium.org/113190/diff/1008/1017
File sandbox/src/sandbox_policy_base.h (right):

http://codereview.chromium.org/113190/diff/1008/1017#newcode150
Line 150: bool use_alternate_winstation_;
I don't see these values being initialized.

http://codereview.chromium.org/113190/diff/1008/1013
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/113190/diff/1008/1013#newcode130
Line 130: // Start the target process suspended
nit: period at the end

http://codereview.chromium.org/113190/diff/1008/1015
File sandbox/src/window.cc (right):

http://codereview.chromium.org/113190/diff/1008/1015#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: just 2009.

http://codereview.chromium.org/113190/diff/1008/1015#newcode14
Line 14: // Get the security attributes of a window object referenced by
|handle|. The
nit: Gets

http://codereview.chromium.org/113190/diff/1008/1015#newcode36
Line 36: // Get the security attributes from the current window station, we will
nit: use semicolon or period instead of comma.

http://codereview.chromium.org/113190/diff/1008/1015#newcode70
Line 70: wchar_t buffer[1024];
nit: isn't this closer to 11 than to 1024?

http://codereview.chromium.org/113190/diff/1008/1015#newcode92
Line 92: return false;
I guess this is a critical failure that should be communicated to the caller
somehow.

http://codereview.chromium.org/113190/diff/1008/1020
File sandbox/src/window.h (right):

http://codereview.chromium.org/113190/diff/1008/1020#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: This should be just 2009.

http://codereview.chromium.org/113190/diff/1008/1020#newcode34
Line 34: 
nit: remove one line.

[Nicolas Sylvain, 2009-05-12 00:20:44]

On 2009/05/11 19:52:46, rvargas wrote:
> Thanks for doing this.
> 
> So far mostly nits.
> 
> You are not calling DestroyAlternateDesktop(), are you?
> 
> http://codereview.chromium.org/113190/diff/1008/1022
> File chrome/browser/sandbox_policy.cc (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1022#newcode407
> Line 407: ::GetThreadDesktop(target.dwThreadId);
> This should not be working properly. target.dwThreadId should not be valid for
> this process.

Yep, this was broken. Fixed.

> 
> http://codereview.chromium.org/113190/diff/1008/1018
> File sandbox/src/policy_target_test.cc (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1018#newcode273
> Line 273: // this should fail if the desktop is really on another window
> station.
> nit: this -> This
done
> 
> http://codereview.chromium.org/113190/diff/1008/1016
> File sandbox/src/sandbox_policy.h (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1016#newcode1
> Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
> nit: update year (also on the other files)
done

> 
> http://codereview.chromium.org/113190/diff/1008/1012
> File sandbox/src/sandbox_policy_base.cc (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1012#newcode52
> Line 52: /* static */
> nit: c++ comment
sort of done

> 
> http://codereview.chromium.org/113190/diff/1008/1012#newcode392
> Line 392: ResultCode PolicyBase::CreateAlternateDesktop(bool
> alternate_winstation) {
> nit: could you place the two new methods together?
done

> 
> http://codereview.chromium.org/113190/diff/1008/1012#newcode402
> Line 402: // Create the window station.
> DCHECK that alternate_wsta_handle_ is null.
done

> 
> http://codereview.chromium.org/113190/diff/1008/1012#newcode404
> Line 404: return SBOX_ERROR_GENERIC;
> nit: use more descriptive error code.
done, all over.
> 
> http://codereview.chromium.org/113190/diff/1008/1012#newcode414
> Line 414: return SBOX_ERROR_GENERIC;
> nit: same here. Compounded by the possibility of critical failure inside
sort of done

> CreateDesktop.
> 
> http://codereview.chromium.org/113190/diff/1008/1017
> File sandbox/src/sandbox_policy_base.h (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1017#newcode150
> Line 150: bool use_alternate_winstation_;
> I don't see these values being initialized.
Oooopsie. done

> 
> http://codereview.chromium.org/113190/diff/1008/1013
> File sandbox/src/target_process.cc (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1013#newcode130
> Line 130: // Start the target process suspended
> nit: period at the end
> 
Done

> http://codereview.chromium.org/113190/diff/1008/1015
> File sandbox/src/window.cc (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1015#newcode1
> Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
> nit: just 2009.

done

> 
> http://codereview.chromium.org/113190/diff/1008/1015#newcode14
> Line 14: // Get the security attributes of a window object referenced by
> |handle|. The
> nit: Gets
done

> 
> http://codereview.chromium.org/113190/diff/1008/1015#newcode36
> Line 36: // Get the security attributes from the current window station, we
will
> nit: use semicolon or period instead of comma.
done

> 
> http://codereview.chromium.org/113190/diff/1008/1015#newcode70
> Line 70: wchar_t buffer[1024];
> nit: isn't this closer to 11 than to 1024?

changed to 16 to be safe
> 
> http://codereview.chromium.org/113190/diff/1008/1015#newcode92
> Line 92: return false;
> I guess this is a critical failure that should be communicated to the caller
> somehow.
Yeah, but this should not fail. (Eh.). I can,t find a good way to communicate
that, and also, at least for chrome, I don't know how they could fix this
anyway.  I vote for leaving it like this. What do you think?
> 
> http://codereview.chromium.org/113190/diff/1008/1020
> File sandbox/src/window.h (right):
> 
> http://codereview.chromium.org/113190/diff/1008/1020#newcode1
> Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
> nit: This should be just 2009.
Done

> 
> http://codereview.chromium.org/113190/diff/1008/1020#newcode34
> Line 34: 
> nit: remove one line.
Done


Thanks! 

Nicolas

[cpu_(ooo_6.6-7.5), 2009-05-12 18:02:08]

http://codereview.chromium.org/113190/diff/2002/2018
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/113190/diff/2002/2018#newcode565
Line 565: }
When you reach this block, how many threads are alive? if more than one, maybe a
good place is line 482.

http://codereview.chromium.org/113190/diff/2002/2012
File sandbox/src/policy_target_test.cc (right):

http://codereview.chromium.org/113190/diff/2002/2012#newcode268
Line 268: 
ASSERT that desktop_name is not empty. because substr will make test crash.

http://codereview.chromium.org/113190/diff/2002/2006
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/113190/diff/2002/2006#newcode52
Line 52: // Initializes static members.
it seems that our style is to use g_alternate_winstation_handle

http://codereview.chromium.org/113190/diff/2002/2011
File sandbox/src/sandbox_policy_base.h (right):

http://codereview.chromium.org/113190/diff/2002/2011#newcode71
Line 71: if (alternate_desktop_handle_) {
So you don't have to switch the thread's winstation to close the desktop? I am
confused now.

[cpu_(ooo_6.6-7.5), 2009-05-12 18:21:55]

http://codereview.chromium.org/113190/diff/2002/2006
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/113190/diff/2002/2006#newcode148
Line 148: // The desktop and winstation should have been create by now.
typo 'created'

http://codereview.chromium.org/113190/diff/2002/2009
File sandbox/src/window.cc (right):

http://codereview.chromium.org/113190/diff/2002/2009#newcode33
Line 33: 
I have an issue with the names of the functions here. For example look at
winuser.h:

#ifdef UNICODE
#define CreateWindowStation  CreateWindowStationW
#else
#define CreateWindowStation  CreateWindowStationA
#endif // !UNICODE

My guess is that this macro is affecting your code and the reason it works is
the argument matching.

http://codereview.chromium.org/113190/diff/2002/2009#newcode38
Line 38: if (!GetSecurityAttributes(::GetProcessWindowStation(), &attributes)) {
Ok, but what do we gain by this?

http://codereview.chromium.org/113190/diff/2002/2009#newcode80
Line 80: }
can we move block 58-64 to line 81 ? so we minimize the time we are in the
alternate desktop.

[rvargas (doing something else), 2009-05-12 21:25:35]

> > http://codereview.chromium.org/113190/diff/1008/1015#newcode92
> > Line 92: return false;
> > I guess this is a critical failure that should be communicated to the caller
> > somehow.
> Yeah, but this should not fail. (Eh.). I can,t find a good way to communicate
> that, and also, at least for chrome, I don't know how they could fix this
> anyway.  I vote for leaving it like this. What do you think?

I guess that the right thing to do is change the prototype of CreateDesktop to
return an error code, and provide a specific error code for this case (with a
note on the declaration of the function). When that error code surfaces to the
Chrome layer, we probably want to  CHECK on this error.

[rvargas (doing something else), 2009-05-12 21:29:22]

http://codereview.chromium.org/113190/diff/2002/2006
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/113190/diff/2002/2006#newcode176
Line 176: return SBOX_ERROR_CANNOT_CREATE_DESKTOP;
nit: SBOX_ERROR_CANNOT_CREATE_WINSTATION?

http://codereview.chromium.org/113190/diff/2002/2009
File sandbox/src/window.cc (right):

http://codereview.chromium.org/113190/diff/2002/2009#newcode29
Line 29: }
nit: we actually need a line after this one.

http://codereview.chromium.org/113190/diff/2002/2005
File sandbox/tests/validation_tests/suite.cc (right):

http://codereview.chromium.org/113190/diff/2002/2005#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: update

[Nicolas Sylvain, 2009-05-14 00:15:28]

http://codereview.chromium.org/113190/diff/2002/2018
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/113190/diff/2002/2018#newcode565
Line 565: }
On 2009/05/12 18:02:08, cpu wrote:
> When you reach this block, how many threads are alive? if more than one, maybe
a
> good place is line 482.
Done, but scared.

http://codereview.chromium.org/113190/diff/2002/2012
File sandbox/src/policy_target_test.cc (right):

http://codereview.chromium.org/113190/diff/2002/2012#newcode268
Line 268: 
On 2009/05/12 18:02:08, cpu wrote:
> ASSERT that desktop_name is not empty. because substr will make test crash.
> 

Done.

http://codereview.chromium.org/113190/diff/2002/2006
File sandbox/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/113190/diff/2002/2006#newcode52
Line 52: // Initializes static members.
On 2009/05/12 18:02:08, cpu wrote:
> it seems that our style is to use g_alternate_winstation_handle
not done, since they are members. Are you sure? If you are sure i'll change it.

http://codereview.chromium.org/113190/diff/2002/2006#newcode148
Line 148: // The desktop and winstation should have been create by now.
On 2009/05/12 18:21:55, cpu wrote:
> typo 'created'

Done.

http://codereview.chromium.org/113190/diff/2002/2006#newcode176
Line 176: return SBOX_ERROR_CANNOT_CREATE_DESKTOP;
On 2009/05/12 21:29:22, rvargas wrote:
> nit: SBOX_ERROR_CANNOT_CREATE_WINSTATION?

Done.

http://codereview.chromium.org/113190/diff/2002/2011
File sandbox/src/sandbox_policy_base.h (right):

http://codereview.chromium.org/113190/diff/2002/2011#newcode71
Line 71: if (alternate_desktop_handle_) {
On 2009/05/12 18:02:08, cpu wrote:
> So you don't have to switch the thread's winstation to close the desktop? I am
> confused now.
> 

Done.

http://codereview.chromium.org/113190/diff/2002/2009
File sandbox/src/window.cc (right):

http://codereview.chromium.org/113190/diff/2002/2009#newcode29
Line 29: }
On 2009/05/12 21:29:22, rvargas wrote:
> nit: we actually need a line after this one.

Done.

http://codereview.chromium.org/113190/diff/2002/2009#newcode33
Line 33: 
On 2009/05/12 18:21:55, cpu wrote:
> I have an issue with the names of the functions here. For example look at
> winuser.h:
> 
> #ifdef UNICODE
> #define CreateWindowStation  CreateWindowStationW
> #else
> #define CreateWindowStation  CreateWindowStationA
> #endif // !UNICODE
> 
> My guess is that this macro is affecting your code and the reason it works is
> the argument matching. 

right. added an s/Create/CreateAlt/g 
Done

http://codereview.chromium.org/113190/diff/2002/2009#newcode38
Line 38: if (!GetSecurityAttributes(::GetProcessWindowStation(), &attributes)) {
On 2009/05/12 18:21:55, cpu wrote:
> Ok, but what do we gain by this?

talked offline. Short story, default dacl is Everyone Full Access. We don't have
world access, so we need at least the Logon Session Id (on vista and windows 7).
The easiest way, and safer way, is to use a "standard" windowstation dacl, like
Windows does.

http://codereview.chromium.org/113190/diff/2002/2009#newcode80
Line 80: }
On 2009/05/12 18:21:55, cpu wrote:
> can we move block 58-64 to line 81 ? so we minimize the time we are in the
> alternate desktop.

Done.

http://codereview.chromium.org/113190/diff/2002/2005
File sandbox/tests/validation_tests/suite.cc (right):

http://codereview.chromium.org/113190/diff/2002/2005#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/05/12 21:29:22, rvargas wrote:
> nit: update

Done.

[rvargas (doing something else), 2009-05-14 21:34:52]

LGTM with a few minor nits.

http://codereview.chromium.org/113190/diff/1078/125
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/113190/diff/1078/125#newcode282
Line 282: sandbox::BrokerServices* broker_services =
nit: please add a comment to this block, mentioning why we want to do this
before other threads are created.

http://codereview.chromium.org/113190/diff/1078/127
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/113190/diff/1078/127#newcode8
Line 8: #define CHROME_COMMON_CHROME_SWITCHES_H__
nit: could you update this guard?

http://codereview.chromium.org/113190/diff/1078/121
File sandbox/src/window.h (right):

http://codereview.chromium.org/113190/diff/1078/121#newcode22
Line 22: // security descriptor of the desktop associated with the current
thread.
nit: please mention that this function can fail to go back to the original wsta.

[cpu_(ooo_6.6-7.5), 2009-05-15 22:44:45]

LGTM

Thanks for doing this.


On 2009/05/14 21:34:52, rvargas wrote:
> LGTM with a few minor nits.
> 
> http://codereview.chromium.org/113190/diff/1078/125
> File chrome/browser/browser_main.cc (right):
> 
> http://codereview.chromium.org/113190/diff/1078/125#newcode282
> Line 282: sandbox::BrokerServices* broker_services =
> nit: please add a comment to this block, mentioning why we want to do this
> before other threads are created.
> 
> http://codereview.chromium.org/113190/diff/1078/127
> File chrome/common/chrome_switches.h (right):
> 
> http://codereview.chromium.org/113190/diff/1078/127#newcode8
> Line 8: #define CHROME_COMMON_CHROME_SWITCHES_H__
> nit: could you update this guard?
> 
> http://codereview.chromium.org/113190/diff/1078/121
> File sandbox/src/window.h (right):
> 
> http://codereview.chromium.org/113190/diff/1078/121#newcode22
> Line 22: // security descriptor of the desktop associated with the current
> thread.
> nit: please mention that this function can fail to go back to the original
wsta.

[rvargas (doing something else), 2009-05-20 01:21:01]

OK

http://codereview.chromium.org/113190/diff/160/180
File chrome/renderer/renderer_main_platform_delegate_win.cc (right):

http://codereview.chromium.org/113190/diff/160/180#newcode31
Line 31: NOTREACHED();
nit: could add a log message here?

http://codereview.chromium.org/113190/diff/160/180#newcode39
Line 39: ::DestroyWindow(window);
nit: could you move the !window check here?

[cpu_(ooo_6.6-7.5), 2009-05-20 01:51:22]

LGTM w/ nits:

http://codereview.chromium.org/113190/diff/160/180
File chrome/renderer/renderer_main_platform_delegate_win.cc (right):

http://codereview.chromium.org/113190/diff/160/180#newcode35
Line 35: // Create a windows on the WinSta0 to initialize theme support.
don't need the comment, see lines 16-20.

http://codereview.chromium.org/113190/diff/160/180#newcode49
Line 49: if (!CloseWindowStation(winsta0)) {
it seems the style here is :: for windows calls