Create a broker interface for the new Mojo EDK so that the browser can create and duplicate messa...

Create a broker interface for the new Mojo EDK so that the browser can create and duplicate message pipes.

This is needed on Windows because sandboxed processes can't create named pipes or duplicate their handles to other processes. Since the new EDK uses one OS pipe per Mojo pipe, child processes need the parent process' help. When a message pipe is created in a child process, it will ask the browser process to do it. When a child process wants to send a mojo pipe to another process, it exchanges the pipe handle with an integer token that it sends instead. The receiving process can exchange that token with a pipe handle.

With this patch, --use-new-edk works with chrome's sandbox.

BUG=478251
R=rickyz@chromium.org, tsepez@chromium.org, yzshen@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/fbada2c0dfa76e3cad57432dc72fc033aa331b50

[jam, 2015-10-20 01:15:03]

Patchset #8 (id:140001) has been deleted

[jam, 2015-10-20 01:15:09]

Patchset #7 (id:120001) has been deleted

[jam, 2015-11-18 00:42:07]

Patchset #14 (id:300001) has been deleted

[jam, 2015-11-18 01:07:30]

Patchset #14 (id:320001) has been deleted

[jam, 2015-11-18 01:49:17]

Description was changed from

==========
Use a token serializer interface in mojo/edk/system to convert PlatformHandle's
to uint64_t's.

The multiprocess implementation uses IPCs to talk from child processes to the
shell process. A child process exchanges a platform handle with an integer token
that it can write into shared memory etc... The receiving process can exchange
that token with a platform handle.

On Windows, this is used whenever a platform handle is sent. This is because
child processes can't duplicate handles when they're sandboxed.

On POSIX, this will be used to serialize FDs of messages waiting to be written
when serializing a RawChannel. This is because there's no guarantee that we can
write the FDs to the pipe (other side might not be readable).

BUG=478251
==========

to

==========
Use a token serializer interface in mojo/edk/system to convert PlatformHandle's
to uint64_t's.

This is needed on Windows because sandboxed processes can't duplicate handles to
other processes. Since the new EDK uses one OS handle per Mojo primitive, child
processes need the parent process' help. A child process exchanges a platform
handle with an integer token that it can write into shared memory etc... The
receiving process can exchange that token with a platform handle.

BUG=478251
==========

[jam, 2015-11-19 20:49:53]

Description was changed from

==========
Use a token serializer interface in mojo/edk/system to convert PlatformHandle's
to uint64_t's.

This is needed on Windows because sandboxed processes can't duplicate handles to
other processes. Since the new EDK uses one OS handle per Mojo primitive, child
processes need the parent process' help. A child process exchanges a platform
handle with an integer token that it can write into shared memory etc... The
receiving process can exchange that token with a platform handle.

BUG=478251
==========

to

==========
Use a token serializer interface in mojo/edk/system to convert PlatformHandle's
to uint64_t's.

This is needed on Windows because sandboxed processes can't duplicate handles to
other processes. Since the new EDK uses one OS handle per Mojo primitive, child
processes need the parent process' help. A child process exchanges a platform
handle with an integer token that it can write into shared memory, send over IPC
etc... The receiving process can exchange that token with a platform handle.

With this patch, --use-new-edk works with chrome's sandbox.

BUG=478251
==========

[jam, 2015-11-19 20:49:54]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[jam, 2015-11-20 01:30:15]

Patchset #20 (id:460001) has been deleted

[jam, 2015-11-20 15:18:33]

Patchset #24 (id:560001) has been deleted

[jam, 2015-11-20 16:37:24]

Description was changed from

==========
Use a token serializer interface in mojo/edk/system to convert PlatformHandle's
to uint64_t's.

This is needed on Windows because sandboxed processes can't duplicate handles to
other processes. Since the new EDK uses one OS handle per Mojo primitive, child
processes need the parent process' help. A child process exchanges a platform
handle with an integer token that it can write into shared memory, send over IPC
etc... The receiving process can exchange that token with a platform handle.

With this patch, --use-new-edk works with chrome's sandbox.

BUG=478251
==========

to

==========
Create a broker interface for the new Mojo EDK so that the browser can create
and duplicate message pipes.

This is needed on Windows because sandboxed processes can't create named pipes
or duplicate their handles to other processes. Since the new EDK uses one OS
pipe per Mojo pipe, child processes need the parent process' help. When a
message pipe is created in a child process, it will ask the browser process to
do it. When a child process wants to send a mojo pipe to another process, it
exchanges the pipe handle with an integer token that it sends instead. The
receiving process can exchange that token with a pipe handle.

With this patch, --use-new-edk works with chrome's sandbox.

BUG=478251
==========

[jam, 2015-11-20 16:37:25]

jam@chromium.org changed reviewers:
+ jschuh@chromium.org

[jam, 2015-11-20 16:38:17]

Yuzhu: main reviewer
Justin: this is the change we discussed, please review from a security
perspective as this opens a new channel to the browser.

[jam, 2015-11-20 16:48:25]

btw I don't forsee new messages being added to token_serializer_messages_win.h
(since that's all we need). Do you think I should make security folks the only
owners for it?

[jam, 2015-11-20 18:55:04]

btw to summarize an in-person conversation with Yuzhu about handle leakage:
It is possible that a process exchanges a handle to a token, and before it sends
it or before the receiver exchanges it back to a token, the process dies. This
would lead to handle leakage. I don't see a way to solve this since the handle
can always be in transit, and the browser doesn't know which process it's going
to. However, this should be rare since the crash would have to happen in very
short time periods, as right after the token exchange the handle is written, and
right after it's received off the pipe it's exchanged.

[yzshen1, 2015-11-20 20:39:55]

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
File mojo/edk/system/child_token_serializer_win.cc (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.cc:54: WriteAndReadResponse(message,
tokens, response_size);
Is there any error handling needed if this call fails?

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.cc:97: &bytes_written, NULL);
Do we need to handle partial read/write as non-error? IIUC, blocking read/write
doesn't guaratee reading/writing the full input buffer.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
File mojo/edk/system/child_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.h:20: // ParentTokenSerializer) to
convert handles to tokens and vice versa.
Please consider commenting on thread safety.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:218: server_handle = channel_pair.PassServerHandle();
(optional comment) one idea is to create a token serializer impl for POSIX,
which just handles the work locally.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
File mojo/edk/system/default_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
mojo/edk/system/default_token_serializer_win.h:14: // production use (i.e. with
multi-process, sandboxes). It's provided for use by
If it is used for testing only, can it be named TokenSerializerForTest instead?
That way it is more clear that it is not for production use.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_state_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_state_win.h:22: // TokenSerializer
interface for use by code in the parent process.
Please consider commenting about on what thread methods are supported to be
called.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_win.cc (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:35:
read_data_.resize(kDefaultReadBufferSize);
Please move this resize() above the post task. Because it is possible (though
unlikely) the task will be run before line 35 is run, which results in a race.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:53: GetLastError() ==
ERROR_BROKEN_PIPE);
When the result is ERROR_BROKEN_PIPE, do we still get a call of OnIOCompleted so
that we can delete this?

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:140: BOOL rv =
WriteFile(pipe_.get().handle, &write_data_[0],
I think it is not guaranteed that the write could be finished in one shot. We
need to handle partial write just like we handle partial read.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
File mojo/edk/system/token_serializer_messages_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
mojo/edk/system/token_serializer_messages_win.h:6: #define
MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_WIN_H_
Should be MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_MESSAGES_WIN_H_

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
mojo/edk/system/token_serializer_messages_win.h:46: #endif  //
MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_WIN_H_
MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_MESSAGES_WIN_H_

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
File mojo/edk/system/token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
mojo/edk/system/token_serializer_win.h:16: // An interface for serializing a
Mojo handle to memory. A child process will
nit: This interface deals with platform handles instead of Mojo handles.

https://codereview.chromium.org/1387963004/diff/620001/mojo/runner/host/child...
File mojo/runner/host/child_process_host.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/runner/host/child...
mojo/runner/host/child_process_host.h:96: // Platform-specific "pipe" to the
child process. Valid immediately after
nit: Please update the comment since it is no longer "platform-specific pipe".

[jam, 2015-11-21 01:24:38]

Patchset #25 (id:600001) has been deleted

[jam, 2015-11-21 01:24:50]

Patchset #10 (id:220001) has been deleted

[jam, 2015-11-21 01:26:10]

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
File mojo/edk/system/child_token_serializer_win.cc (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.cc:54: WriteAndReadResponse(message,
tokens, response_size);
On 2015/11/20 20:39:55, yzshen1 wrote:
> Is there any error handling needed if this call fails?

it'll fail because bogus handles are passed in. but in that case, the receiver
should handle this case i think. there's not really a mechanism to bubble back
this error from what i can see, looking at HandleToToken's callsites.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.cc:97: &bytes_written, NULL);
On 2015/11/20 20:39:55, yzshen1 wrote:
> Do we need to handle partial read/write as non-error? IIUC, blocking
read/write
> doesn't guaratee reading/writing the full input buffer.

per
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747(v=vs.85).aspx
and
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365150(v=vs.85).aspx,
I don't see write will return earlier (it'll block per doc). i also just tried
this now, and tried writing 120K and it worked synchronously. however you're
right read could return earlier. I'll fix that (I had made parent support this,
but overlooked this file)https://codereview.chromium.org/static/zippyplus.gif

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
File mojo/edk/system/child_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.h:20: // ParentTokenSerializer) to
convert handles to tokens and vice versa.
On 2015/11/20 20:39:55, yzshen1 wrote:
> Please consider commenting on thread safety.

added comment in the interface

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:218: server_handle = channel_pair.PassServerHandle();
On 2015/11/20 20:39:55, yzshen1 wrote:
> (optional comment) one idea is to create a token serializer impl for POSIX,
> which just handles the work locally.

yeah i originally went that route, but then it felt needless and the calling
sites for token<>handle were windows-only anyways so it would be more confusing.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
File mojo/edk/system/default_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
mojo/edk/system/default_token_serializer_win.h:14: // production use (i.e. with
multi-process, sandboxes). It's provided for use by
On 2015/11/20 20:39:55, yzshen1 wrote:
> If it is used for testing only, can it be named TokenSerializerForTest
instead?
> That way it is more clear that it is not for production use.

well; it can also be used by code that's not sandboxed... so that's why i didn't
put test in the name.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_state_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_state_win.h:22: // TokenSerializer
interface for use by code in the parent process.
On 2015/11/20 20:39:55, yzshen1 wrote:
> Please consider commenting about on what thread methods are supported to be
> called.

(per other comment, left comment in interface)

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_win.cc (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:35:
read_data_.resize(kDefaultReadBufferSize);
On 2015/11/20 20:39:55, yzshen1 wrote:
> Please move this resize() above the post task. Because it is possible (though
> unlikely) the task will be run before line 35 is run, which results in a race.

done, good catch

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:53: GetLastError() ==
ERROR_BROKEN_PIPE);
On 2015/11/20 20:39:55, yzshen1 wrote:
> When the result is ERROR_BROKEN_PIPE, do we still get a call of OnIOCompleted
so
> that we can delete this?

good point, deleted for broken pipe case

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:140: BOOL rv =
WriteFile(pipe_.get().handle, &write_data_[0],
On 2015/11/20 20:39:55, yzshen1 wrote:
> I think it is not guaranteed that the write could be finished in one shot. We
> need to handle partial write just like we handle partial read.

since this side uses overlapped IO (as opposed to child side which doesn't),
while it might not complete synchronously, it will complete async with that data
(i.e. return value of 0 and last error of ERROR_IO_PENDING). There isn't a
problem of reusing the write context because the other side (child) won't send
more messages till they read all their previous response.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
File mojo/edk/system/token_serializer_messages_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
mojo/edk/system/token_serializer_messages_win.h:6: #define
MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_WIN_H_
On 2015/11/20 20:39:55, yzshen1 wrote:
> Should be MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_MESSAGES_WIN_H_

Done.

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/token_...
mojo/edk/system/token_serializer_messages_win.h:46: #endif  //
MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_WIN_H_
On 2015/11/20 20:39:55, yzshen1 wrote:
> MOJO_EDK_SYSTEM_TOKEN_SERIALIZER_MESSAGES_WIN_H_

Done.

https://codereview.chromium.org/1387963004/diff/620001/mojo/runner/host/child...
File mojo/runner/host/child_process_host.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/runner/host/child...
mojo/runner/host/child_process_host.h:96: // Platform-specific "pipe" to the
child process. Valid immediately after
On 2015/11/20 20:39:55, yzshen1 wrote:
> nit: Please update the comment since it is no longer "platform-specific pipe".

Done.

[yzshen1, 2015-11-23 16:47:02]

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
File mojo/edk/system/default_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
mojo/edk/system/default_token_serializer_win.h:14: // production use (i.e. with
multi-process, sandboxes). It's provided for use by
On 2015/11/21 01:26:09, jam wrote:
> On 2015/11/20 20:39:55, yzshen1 wrote:
> > If it is used for testing only, can it be named TokenSerializerForTest
> instead?
> > That way it is more clear that it is not for production use.
> 
> well; it can also be used by code that's not sandboxed... so that's why i
didn't
> put test in the name.

I feel that it is a little inaccurate to name testing/non-sandboxed code
"Default". Maybe we could consider NonSandboxedTokenSerializer to make it
obvious?

https://codereview.chromium.org/1387963004/diff/660001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_win.cc (right):

https://codereview.chromium.org/1387963004/diff/660001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:149: DCHECK(rv || rv ==
ERROR_IO_PENDING);
rv is of type BOOL, so it cannot be compared with ERROR_IO_PENDING.

[jam, 2015-11-23 17:12:35]

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
File mojo/edk/system/default_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/620001/mojo/edk/system/defaul...
mojo/edk/system/default_token_serializer_win.h:14: // production use (i.e. with
multi-process, sandboxes). It's provided for use by
On 2015/11/23 16:47:02, yzshen1 wrote:
> On 2015/11/21 01:26:09, jam wrote:
> > On 2015/11/20 20:39:55, yzshen1 wrote:
> > > If it is used for testing only, can it be named TokenSerializerForTest
> > instead?
> > > That way it is more clear that it is not for production use.
> > 
> > well; it can also be used by code that's not sandboxed... so that's why i
> didn't
> > put test in the name.
> 
> I feel that it is a little inaccurate to name testing/non-sandboxed code
> "Default". Maybe we could consider NonSandboxedTokenSerializer to make it
> obvious?

renamed to simple per discussion

https://codereview.chromium.org/1387963004/diff/660001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_win.cc (right):

https://codereview.chromium.org/1387963004/diff/660001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_win.cc:149: DCHECK(rv || rv ==
ERROR_IO_PENDING);
On 2015/11/23 16:47:02, yzshen1 wrote:
> rv is of type BOOL, so it cannot be compared with ERROR_IO_PENDING.

oops, thanks fixed to GetLastError()

[jam, 2015-11-23 17:22:45]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org
- jschuh@chromium.org

[jam, 2015-11-23 17:22:46]

Switching to Tom for security review since jschuh is ooo

[yzshen1, 2015-11-23 17:34:39]

On 2015/11/23 17:22:46, jam wrote:
> Switching to Tom for security review since jschuh is ooo

LGTM thanks!

[Tom Sepez, 2015-11-23 18:14:40]

tsepez@chromium.org changed reviewers:
+ wfh@chromium.org

[Tom Sepez, 2015-11-23 18:14:42]

+wfh for the specifics of the windows system calls.

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_state_win.cc (right):

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_state_win.cc:36: token =
base::RandUint64();
As we discussed, this is too predictable to use for tokens.

[Tom Sepez, 2015-11-23 18:28:53]

tsepez@chromium.org changed reviewers:
+ jln@chromium.org

[Tom Sepez, 2015-11-23 18:28:54]

+jln for mojo/runner/host/child_process.cc

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_posix.cc (right):

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:49: // don't peak) as a way of
determining later if two sockets are connected to
nit: sp. peek

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:51: int identifier =
base::RandInt(kint32min, kint32max);
Hmm.  If we rely on this (rather than as a sanity check), then this is too small
and we'll get collisions in the field.

[jam, 2015-11-23 18:29:01]

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/system/parent...
File mojo/edk/system/parent_token_serializer_state_win.cc (right):

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/system/parent...
mojo/edk/system/parent_token_serializer_state_win.cc:36: token =
base::RandUint64();
On 2015/11/23 18:14:42, Tom Sepez wrote:
> As we discussed, this is too predictable to use for tokens.

Thanks. I looked at base/rand_util_win.cc and the comment above RandUint64 says
that it's cryptographically secure:
https://code.google.com/p/chromium/codesearch#chromium/src/base/rand_util_win...

the method in crypto/random.h just calls out to the same implementation
(base::RandBytes)

[jam, 2015-11-23 18:38:16]

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_posix.cc (right):

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:49: // don't peak) as a way of
determining later if two sockets are connected to
On 2015/11/23 18:28:54, Tom Sepez wrote:
> nit: sp. peek

Done.

https://codereview.chromium.org/1387963004/diff/680001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:51: int identifier =
base::RandInt(kint32min, kint32max);
On 2015/11/23 18:28:54, Tom Sepez wrote:
> Hmm.  If we rely on this (rather than as a sanity check), then this is too
small
> and we'll get collisions in the field.

good point, I didn't consider that. for now, i've put this behind official_build
so we don't get collisions in the field.

[Tom Sepez, 2015-11-23 18:38:40]

> 
> the method in crypto/random.h just calls out to the same implementation
> (base::RandBytes)
Ah, OK so long as its windows only.

[Will Harris, 2015-11-23 18:49:46]

On 2015/11/23 18:38:40, Tom Sepez wrote:
> > 
> > the method in crypto/random.h just calls out to the same implementation
> > (base::RandBytes)
> Ah, OK so long as its windows only.

yup base::RandUint64 is as random as we get, so it should be okay for use as a
token.

[Will Harris, 2015-11-23 18:50:01]

wfh@chromium.org changed reviewers:
- wfh@chromium.org

[jam, 2015-11-23 18:58:21]

On 2015/11/23 18:28:54, Tom Sepez wrote:
> +jln for mojo/runner/host/child_process.cc

btw the non-windows changes there are just code moves?

[Tom Sepez, 2015-11-23 21:16:58]

On 2015/11/23 18:58:21, jam wrote:
> On 2015/11/23 18:28:54, Tom Sepez wrote:
> > +jln for mojo/runner/host/child_process.cc
> 
> btw the non-windows changes there are just code moves?

Wanted jln to look at the sandbox stuff.

[jln (very slow on Chromium), 2015-11-23 21:27:41]

jln@chromium.org changed reviewers:
+ rickyz@chromium.org

[jln (very slow on Chromium), 2015-11-23 21:28:23]

On 2015/11/23 21:16:58, Tom Sepez wrote:
> On 2015/11/23 18:58:21, jam wrote:
> > On 2015/11/23 18:28:54, Tom Sepez wrote:
> > > +jln for mojo/runner/host/child_process.cc
> > 
> > btw the non-windows changes there are just code moves?
> 
> Wanted jln to look at the sandbox stuff.

Thanks Tom. After a quick glance, looks like a code move for the sandbox stuff
as well.
cc-ing Rickyz who is following this more closely than I am.

[Tom Sepez, 2015-11-24 00:17:48]

> > Wanted jln to look at the sandbox stuff.
> 
> Thanks Tom. After a quick glance, looks like a code move for the sandbox stuff
> as well.
> cc-ing Rickyz who is following this more closely than I am.


Ok, so I'm LGTM once the others sign off.  Thanks.

[rickyz (no longer on Chrome), 2015-11-24 00:30:43]

sandbox move lgtm

https://codereview.chromium.org/1387963004/diff/700001/mojo/edk/system/child_...
File mojo/edk/system/child_token_serializer_win.h (right):

https://codereview.chromium.org/1387963004/diff/700001/mojo/edk/system/child_...
mojo/edk/system/child_token_serializer_win.h:53: base::internal::LockImpl lock_;
Unlocking this on a different thread looks like it should work fine on Windows
(which I guess is all that matters here), but the API doesn't appear to
guarantee supporting it - the Linux version would error, I think.

[jam, 2015-11-24 01:24:02]

The CQ bit was checked by jam@chromium.org

[jam, 2015-11-24 01:24:02]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1387963004/#ps720001
(title: "merge")

[commit-bot: I haz the power, 2015-11-24 01:27:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387963004/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387963004/720001

[commit-bot: I haz the power, 2015-11-24 03:12:44]

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

[commit-bot: I haz the power, 2015-11-24 03:12:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2015-11-24 04:19:03]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-11-24 04:20:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387963004/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387963004/720001

[commit-bot: I haz the power, 2015-11-24 04:32:08]

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

[commit-bot: I haz the power, 2015-11-24 04:32:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jam, 2015-11-24 05:13:22]

The CQ bit was checked by jam@chromium.org

[jam, 2015-11-24 05:13:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rickyz@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1387963004/#ps740001
(title: "presubmit whitespace error")

[commit-bot: I haz the power, 2015-11-24 05:14:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387963004/740001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387963004/740001

[commit-bot: I haz the power, 2015-11-24 06:40:55]

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

[jam, 2015-11-24 06:41:18]

Committed patchset #30 (id:740001) manually as
fbada2c0dfa76e3cad57432dc72fc033aa331b50 (presubmit successful).

[brucedawson, 2015-11-25 20:44:10]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-11-25 20:44:11]

https://codereview.chromium.org/1387963004/diff/740001/mojo/edk/system/token_...
File mojo/edk/system/token_serializer_messages_win.h (right):

https://codereview.chromium.org/1387963004/diff/740001/mojo/edk/system/token_...
mojo/edk/system/token_serializer_messages_win.h:30: struct ALIGNAS(4)
TokenSerializerMessage {
What is the purpose of the ALIGNAS(4) directive here? VC++ 2015 doesn't like it:

warning C4359: 'mojo::edk::TokenSerializerMessage': Alignment specifier is less
than actual alignment (8), and will be ignored.

The problem is the uint64_t member which (even in 32-bit x86 builds with VC++)
has 8-byte alignment. And, it appears that __declspec(align()) can only be used
to increase the alignment, not decrease it.

Was the goal to change the *packing* of the struct, or the alignment? If the
packing of the struct was set to 4 then that would ensure that it was optimally
packed without hitting this problem, and would also reduce the alignment
requirements.

As it stands the struct is already optimally packed, although it's 8-byte
alignment requirement may affect structures that it is placed in.

I think that the fix is to remove the ALIGNAS(4) directive. VC++ 2013 is already
ignoring it - VC++ 2015 is just more honest about it.