Adds sync brokering to Windows EDK

Adds sync brokering to Windows EDK

We use a sync channel on other platforms to facilitate synchronous
shared buffer creation from within a sandbox. We need to do this
on Windows too.

This implements that, and adds a test for sandboxed shared buffer
creation.

BUG=594883
Committed: https://crrev.com/b214d02f960a60409d5163ef06e4d91ceb71e983
Cr-Commit-Position: refs/heads/master@{#418679}

[Ken Rockot(use gerrit already), 2016-09-08 18:16:45]

Description was changed from

==========
[WIP] sandboxed shared buffer test

adapted from https://codereview.chromium.org/1799923003/

BUG=
==========

to

==========
Adds sync brokering to Windows EDK

We use a sync channel on other platforms to facilitate synchronous
shared buffer creation from within a sandbox. We need to do this
on Windows too.

This implements that, and adds a test for sandboxed shared buffer
creation.

BUG=594883
==========

[Ken Rockot(use gerrit already), 2016-09-08 18:16:45]

rockot@chromium.org changed reviewers:
+ dcheng@chromium.org, yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-09-08 18:16:51]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2016-09-08 18:16:57]

Patchset #1 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-08 18:17:00]

Patchset #1 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-08 18:17:04]

Patchset #1 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-08 18:17:12]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:17:46]

Dry run: 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-09-08 18:22:38]

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

[commit-bot: I haz the power, 2016-09-08 18:22:39]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-09-08 18:22:43]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:23:23]

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

[Ken Rockot(use gerrit already), 2016-09-08 18:26:09]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2016-09-08 18:26:22]

Patchset #1 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-08 18:26:27]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-09-08 18:27:08]

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

[Ken Rockot(use gerrit already), 2016-09-08 19:06:57]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 19:07:53]

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

[Ken Rockot(use gerrit already), 2016-09-08 19:20:40]

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

[Ken Rockot(use gerrit already), 2016-09-08 19:20:41]

yzshen@ please take a look for general EDK review
dcheng@ for security
jam@ for content

[commit-bot: I haz the power, 2016-09-08 20:19:15]

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

[commit-bot: I haz the power, 2016-09-08 20:19:16]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-08 20:56:03]

lgtm with merging test interfaces

[dcheng, 2016-09-08 21:04:13]

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

[dcheng, 2016-09-08 21:04:14]

+wfh, do you mind reviewing this from a security perspective?

[Will Harris, 2016-09-08 21:37:02]

expectation management: I won't be able to look at this this week, but I can
next week (prob tuesday 13 Sep). If you need a quicker review then perhaps
tsepez can volunteer.

[Ken Rockot(use gerrit already), 2016-09-08 21:39:24]

This is not high priority work as it's not blocking anyone. I've had it
lying around for several weeks now and finally had a few minutes to clean
it up. I'll ping the CL again mid next week. Thanks!

On Thu, Sep 8, 2016 at 2:37 PM, <wfh@chromium.org> wrote:

> expectation management: I won't be able to look at this this week, but I
> can
> next week (prob tuesday 13 Sep). If you need a quicker review then perhaps
> tsepez can volunteer.
>
> https://codereview.chromium.org/2264543003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-09-08 22:21:45]

LGTM

Thanks!

[Ken Rockot(use gerrit already), 2016-09-13 01:48:07]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 01:48:40]

Dry run: 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-09-13 03:10:52]

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

[commit-bot: I haz the power, 2016-09-13 03:10:53]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-09-13 22:32:24]

Friendly ping wfh@ just to get it back on your radar

[Will Harris, 2016-09-13 23:54:44]

I have some Qs about the casts, otherwise looks mostly good.

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_shared_buffer.cc (right):

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_shared_buffer.cc:269: false);
I think this is correct...? as presumably you create the handle rw here then dup
it to a ro one on L274 but can you add a comment as it looks wonky to create a
ro_handle with |read_only| set to false.

Unless this is wrong, in which case why have the two stage create?

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/system/broker...
File mojo/edk/system/broker_win.cc (right):

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/system/broker...
mojo/edk/system/broker_win.cc:89: DCHECK_LE(num_bytes,
std::numeric_limits<uint32_t>::max());
why not use safe_numerics here? also I feel this should be stronger than a
DCHECK like a fail or a CHECK

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/system/broker...
mojo/edk/system/broker_win.cc:90: buffer_request->size =
static_cast<uint32_t>(num_bytes);
BufferRequestData's size member is a size_t - why the cast at all? tbh I'm
unsure why anyone would want a shared buffer > 4Gb anyway... can't you change
this to a uint32_t and move the responsibility of down casting and checking the
length to the callers of this API?

[Ken Rockot(use gerrit already), 2016-09-14 19:56:05]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:56:55]

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

[Ken Rockot(use gerrit already), 2016-09-14 19:57:05]

Thanks!

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_shared_buffer.cc (right):

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_shared_buffer.cc:269: false);
On 2016/09/13 at 23:54:43, Will Harris wrote:
> I think this is correct...? as presumably you create the handle rw here then
dup it to a ro one on L274 but can you add a comment as it looks wonky to create
a ro_handle with |read_only| set to false.
> 
> Unless this is wrong, in which case why have the two stage create?

The SharedMemoryHandle constructor doesn't know anything about read_only. The
|false| in these constructors is for |auto_close| (see base::FileDescriptor).
It's correct since we pass ownership of the handles into base::SharedMemory and
don't want them scoped to the local handle here.

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/system/broker...
File mojo/edk/system/broker_win.cc (right):

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/system/broker...
mojo/edk/system/broker_win.cc:89: DCHECK_LE(num_bytes,
std::numeric_limits<uint32_t>::max());
On 2016/09/13 at 23:54:43, Will Harris wrote:
> why not use safe_numerics here? also I feel this should be stronger than a
DCHECK like a fail or a CHECK

Switched to using base::checked_cast below and killed this DCHECK

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/system/broker...
mojo/edk/system/broker_win.cc:90: buffer_request->size =
static_cast<uint32_t>(num_bytes);
On 2016/09/13 at 23:54:43, Will Harris wrote:
> BufferRequestData's size member is a size_t - why the cast at all? tbh I'm
unsure why anyone would want a shared buffer > 4Gb anyway... can't you change
this to a uint32_t and move the responsibility of down casting and checking the
length to the callers of this API?

It's not a size_t, it's a uint32_t.
https://cs.chromium.org/chromium/src/mojo/edk/system/broker_messages.h?rcl=0&...

I have however updated the BrokerHost interface and impl to reflect that this is
a uint32_t so hopefully that's less confusing now.

[Will Harris, 2016-09-14 20:24:21]

lgtm thanks for changes, as discussed maybe evalulate use of size_t vs uint64_t
vs uint32_t in a future CL. Also address the static_cast in
SharedBufferDispatcher if possible. Thanks!

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_shared_buffer.cc (right):

https://codereview.chromium.org/2264543003/diff/160001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_shared_buffer.cc:269: false);
On 2016/09/14 19:57:04, Ken Rockot wrote:
> On 2016/09/13 at 23:54:43, Will Harris wrote:
> > I think this is correct...? as presumably you create the handle rw here then
> dup it to a ro one on L274 but can you add a comment as it looks wonky to
create
> a ro_handle with |read_only| set to false.
> > 
> > Unless this is wrong, in which case why have the two stage create?
> 
> The SharedMemoryHandle constructor doesn't know anything about read_only. The
> |false| in these constructors is for |auto_close| (see base::FileDescriptor).
> It's correct since we pass ownership of the handles into base::SharedMemory
and
> don't want them scoped to the local handle here.

Acknowledged.

[Ken Rockot(use gerrit already), 2016-09-14 20:26:19]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-14 20:26:23]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-14 20:26:23]

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

[commit-bot: I haz the power, 2016-09-14 20:27:20]

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-09-14 21:29:14]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2016-09-14 21:32:00]

Description was changed from

==========
Adds sync brokering to Windows EDK

We use a sync channel on other platforms to facilitate synchronous
shared buffer creation from within a sandbox. We need to do this
on Windows too.

This implements that, and adds a test for sandboxed shared buffer
creation.

BUG=594883
==========

to

==========
Adds sync brokering to Windows EDK

We use a sync channel on other platforms to facilitate synchronous
shared buffer creation from within a sandbox. We need to do this
on Windows too.

This implements that, and adds a test for sandboxed shared buffer
creation.

BUG=594883

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

[commit-bot: I haz the power, 2016-09-14 21:32:02]

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