[Chromoting] Move CreateSessionToken() next to launch process utilities.

[Chromoting] Move CreateSessionToken() next to launch process utilities.

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

[alexeypa (please no reviews), 2012-08-03 20:26:38]

Please take a look.

[Wez, 2012-08-03 20:56:06]

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:43: // |desired_access| rights.
nit: Clarify that we make an impersonation token from it.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:45: HANDLE handle;
Why not use ScopedHandle::Receive() to avoid the unmanaged HANDLE?

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:70: bool
CreatePrivilegedToken(ScopedHandle* token_out) {
nit: Why not have CreatePrivilegedToken() accept the token to duplicate to be
privileged as a parameter, pass the process token in, and have this method
duplicate it?

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:312: // Temporarily enable the
SE_TCB_NAME privilege.
nit: Update comment to indicate _why_ we're doing this.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:317: 
nit: Lose the blank line; it implies that the impersonation isn't related to the
SE_TCB_NAME comment.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.h:13: #include
"base/win/scoped_handle.h"
nit: Forward-declare since you don't actually need to know the internals of
ScopedHandle in this header?

[alexeypa (please no reviews), 2012-08-03 21:40:38]

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:43: // |desired_access| rights.
On 2012/08/03 20:56:06, Wez wrote:
> nit: Clarify that we make an impersonation token from it.

Done.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:45: HANDLE handle;
On 2012/08/03 20:56:06, Wez wrote:
> Why not use ScopedHandle::Receive() to avoid the unmanaged HANDLE?

Good point. I missed this piece.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:70: bool
CreatePrivilegedToken(ScopedHandle* token_out) {
On 2012/08/03 20:56:06, Wez wrote:
> nit: Why not have CreatePrivilegedToken() accept the token to duplicate to be
> privileged as a parameter, pass the process token in, and have this method
> duplicate it?

The main reason is that it is not going to be used by the existing code. Should
such need arise it will be easy to make the change.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:312: // Temporarily enable the
SE_TCB_NAME privilege.
On 2012/08/03 20:56:06, Wez wrote:
> nit: Update comment to indicate _why_ we're doing this.

Done.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.cc:317: 
On 2012/08/03 20:56:06, Wez wrote:
> nit: Lose the blank line; it implies that the impersonation isn't related to
the
> SE_TCB_NAME comment.

Done.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.h:13: #include
"base/win/scoped_handle.h"
On 2012/08/03 20:56:06, Wez wrote:
> nit: Forward-declare since you don't actually need to know the internals of
> ScopedHandle in this header?

ScopedHandle is a template specialization which cannot be forward-declared.
However we don't really need ScopedHandle here at all.

[Wez, 2012-08-06 17:41:04]

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.h:13: #include
"base/win/scoped_handle.h"
On 2012/08/03 21:40:39, alexeypa wrote:
> On 2012/08/03 20:56:06, Wez wrote:
> > nit: Forward-declare since you don't actually need to know the internals of
> > ScopedHandle in this header?
> 
> ScopedHandle is a template specialization which cannot be forward-declared.
> However we don't really need ScopedHandle here at all.

Personally I'd leave the API using ScopedHandle, at the cost of requiring this
include, to keep HANDLE ownership clear.

http://codereview.chromium.org/10828160/diff/4001/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10828160/diff/4001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:45: bool CopyProcessToken(DWORD
desired_access, HANDLE* token_out) {
Why has this become HANDLE, rather than staying ScopedHandle and using
Set(Take()) below?

http://codereview.chromium.org/10828160/diff/4001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:70: bool
CreatePrivilegedToken(HANDLE* token_out) {
Why not ScopedHandle* here any more?

[alexeypa (please no reviews), 2012-08-06 17:48:36]

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.h:13: #include
"base/win/scoped_handle.h"
On 2012/08/06 17:41:04, Wez wrote:
> Personally I'd leave the API using ScopedHandle, at the cost of requiring this
> include, to keep HANDLE ownership clear.

Getting rid of include wasn't the reason to switch to HANDLE*. ScopedHandle
designed to pass and get ownership via Take() and Receive(). The code looks more
consistent and cleaner if they used. It starts getting uglier once I try passing
ScopedHandle instead of a naked type. ScopedHandle.Pass() would be handy, but
given the lack of it I prefer to stick to Take() and Release().

http://codereview.chromium.org/10828160/diff/4001/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10828160/diff/4001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:45: bool CopyProcessToken(DWORD
desired_access, HANDLE* token_out) {
On 2012/08/06 17:41:04, Wez wrote:
> Why has this become HANDLE, rather than staying ScopedHandle and using
> Set(Take()) below?

See my previous comment. I believe it looks cleaner this way.

http://codereview.chromium.org/10828160/diff/4001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:70: bool
CreatePrivilegedToken(HANDLE* token_out) {
On 2012/08/06 17:41:04, Wez wrote:
> Why not ScopedHandle* here any more?

See my previous comment. I believe it looks cleaner this way.

[Wez, 2012-08-06 17:57:30]

LGTM but see comment re HANDLE vs ScoedHandle.

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/10828160/diff/1/remoting/host/win/launch_proce...
remoting/host/win/launch_process_with_token.h:13: #include
"base/win/scoped_handle.h"
On 2012/08/06 17:48:36, alexeypa wrote:
> On 2012/08/06 17:41:04, Wez wrote:
> > Personally I'd leave the API using ScopedHandle, at the cost of requiring
this
> > include, to keep HANDLE ownership clear.
> 
> Getting rid of include wasn't the reason to switch to HANDLE*. ScopedHandle
> designed to pass and get ownership via Take() and Receive(). The code looks
more
> consistent and cleaner if they used. It starts getting uglier once I try
passing
> ScopedHandle instead of a naked type. ScopedHandle.Pass() would be handy, but
> given the lack of it I prefer to stick to Take() and Release().

Right; Pass() is syntactic sugar for Set(Take()); having in-parameters be
HANDLEs, with the assumption being that the function does not "own" HANDLEs is
fine, but for an out-parameter it feels clearer to use ScopedHandle* similarly
to the way we use scoped_ptr<>*.

[alexeypa (please no reviews), 2012-08-06 21:17:12]

+brettw

Brett, could you please take a look at base/win/scoped_handle.h?

[alexeypa (please no reviews), 2012-08-06 22:20:33]

+jar@ since brettw@ is OOO.

jar@, please take a look at src/base/win/scoped_handle.h.

[jar (doing other things), 2012-08-07 01:00:30]

looking at scoped_handle.h....

I'm not familiar with the new C++ stuff you cited... and couldn't find it via
google... perhaps you can at least add pointers in your response to my question
so I can read about it.

https://chromiumcodereview.appspot.com/10828160/diff/3002/base/win/scoped_han...
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/10828160/diff/3002/base/win/scoped_han...
base/win/scoped_handle.h:56: :
handle_(reinterpret_cast<GenericScopedHandle&>(other).Take()) {
Should be a static_cast (possibly of pointers)?  If not, why not?

https://chromiumcodereview.appspot.com/10828160/diff/3002/base/win/scoped_han...
base/win/scoped_handle.h:68: GenericScopedHandle& operator=(RValue& other) {
I'm a little surprised you didn't pass in a const ref.... but then I see you
used Swap(), which required a non-const.

It seems strange (unexpected?) that an assignment operator modifies the rvalue. 
I would (at most) have expected it to close the assigned-to handle, and replace
the rvalue's handle with an invalid handle.

Can you justify this semantic, or add a comment to that effect into the code?

[alexeypa (please no reviews), 2012-08-07 17:55:14]

Addressed comments in scoped_handle.h.

https://chromiumcodereview.appspot.com/10828160/diff/3002/base/win/scoped_han...
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/10828160/diff/3002/base/win/scoped_han...
base/win/scoped_handle.h:56: :
handle_(reinterpret_cast<GenericScopedHandle&>(other).Take()) {
On 2012/08/07 01:00:30, jar wrote:
> Should be a static_cast (possibly of pointers)?  If not, why not?

This code was copied from scoped_handle.h. It reinterpret_cast not seem to be
needed though. RValue is a structure that inherits from GenericScopedHandle
(defined by MOVE_ONLY_TYPE_FOR_CPP_03 macro). Take() should be accessible.

https://chromiumcodereview.appspot.com/10828160/diff/3002/base/win/scoped_han...
base/win/scoped_handle.h:68: GenericScopedHandle& operator=(RValue& other) {
On 2012/08/07 01:00:30, jar wrote:
> I'm a little surprised you didn't pass in a const ref.... but then I see you
> used Swap(), which required a non-const.
> It seems strange (unexpected?) that an assignment operator modifies the
rvalue. 

This is copied from scoped_ptr too. |other| has to be modified because
operator=() passes ownership.

> I would (at most) have expected it to close the assigned-to handle, and
replace
> the rvalue's handle with an invalid handle.

This will break self-assignment (when this == &other). Swap allows to avoid that
and it is also very cheap.

> Can you justify this semantic, or add a comment to that effect into the code?

Added.

[jar (doing other things), 2012-08-08 01:42:52]

LGTM (only because of similar code for smart_ptr )

Can you cite a reference?  

I'd also suggest deleting the comment about self assignment, as it doesn't seem
to help.

https://chromiumcodereview.appspot.com/10828160/diff/11004/base/win/scoped_ha...
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/10828160/diff/11004/base/win/scoped_ha...
base/win/scoped_handle.h:65: // Swapping the handles helps to avoid problems
while assigning a handle
The usual "trick" with dealing with self assignment is to do nothing when the
objects are the same (have same addresses).

I think the only answer is that you're copying it the "Move operator= for
C++03..." but I'm not familiar with this phrasing.  Can you point me to where I
can read something about it?  Google didn't like "C++03" (or at least it wasn't
helpful).

[alexeypa (please no reviews), 2012-08-08 01:59:29]

https://chromiumcodereview.appspot.com/10828160/diff/11004/base/win/scoped_ha...
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/10828160/diff/11004/base/win/scoped_ha...
base/win/scoped_handle.h:65: // Swapping the handles helps to avoid problems
while assigning a handle
On 2012/08/08 01:42:52, jar wrote:
> The usual "trick" with dealing with self assignment is to do nothing when the
> objects are the same (have same addresses).
> 
> I think the only answer is that you're copying it the "Move operator= for
> C++03..." but I'm not familiar with this phrasing.  Can you point me to where
I
> can read something about it?  Google didn't like "C++03" (or at least it
wasn't
> helpful).

src/base/move.h has a long explanation what it is doing. 

C++03 is the C++ standard from 2003. It does not support Rvalue references
(type&& was added in C++11) so move.h employs some trickery to do something
similar.

[commit-bot: I haz the power, 2012-08-08 05:12:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10828160/11004

[commit-bot: I haz the power, 2012-08-08 06:45:37]

Change committed as 150508