ozone: Validate the memory buffer used.

ozone: Validate the memory buffer used.

BUG=none

Committed: https://crrev.com/35ab4d767861e705756943e5936baf09891fc012
Cr-Commit-Position: refs/heads/master@{#416335}

[sadrul, 2016-09-01 20:19:54]

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

[sadrul, 2016-09-01 20:20:33]

sadrul@chromium.org changed reviewers:
+ rjkroege@chromium.org, tsepez@chromium.org

[commit-bot: I haz the power, 2016-09-01 20:20:44]

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

[Tom Sepez, 2016-09-01 20:23:49]

lgtm

Thanks.   Note there is an IsValid() call if you want to log a better error
instead of dying in valueordie().

[rjkroege, 2016-09-01 20:45:06]

https://codereview.chromium.org/2302053003/diff/1/ui/ozone/platform/drm/commo...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2302053003/diff/1/ui/ozone/platform/drm/commo...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:74: data_ =
mmap(nullptr, map_size.ValueOrDie(), (PROT_READ | PROT_WRITE),
Won't this will kill the caller if the value is invalid. Is this really what we
want? What we really want is to kill the GPU process if this happens because it
sent us the bad data that brought us here instead of killing (for example) the
browser trying to map the provided GPU memory?

[commit-bot: I haz the power, 2016-09-01 21:00:06]

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

[commit-bot: I haz the power, 2016-09-01 21:00:08]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-09-02 00:48:00]

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

[commit-bot: I haz the power, 2016-09-02 00:48:17]

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-02 01:37:07]

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

[commit-bot: I haz the power, 2016-09-02 01:37:08]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-09-02 03:00:19]

https://codereview.chromium.org/2302053003/diff/1/ui/ozone/platform/drm/commo...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2302053003/diff/1/ui/ozone/platform/drm/commo...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:74: data_ =
mmap(nullptr, map_size.ValueOrDie(), (PROT_READ | PROT_WRITE),
On 2016/09/01 20:45:05, rjkroege wrote:
> Won't this will kill the caller if the value is invalid. Is this really what
we
> want? What we really want is to kill the GPU process if this happens because
it
> sent us the bad data that brought us here instead of killing (for example) the
> browser trying to map the provided GPU memory?

As discussed offline, I have changed this code to not be fatal. But instead, it
will return null, instead of attempting to use the buffer. Killing the GPU isn't
straightforward from here. But it is possible (e.g. a quick hack:
https://codereview.chromium.org/2302733003/). I am going to continue to look
into that as the next step in a subsequent CL.

[rjkroege, 2016-09-02 19:02:03]

https://codereview.chromium.org/2302053003/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2302053003/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:74: :
dmabuf_fd_(dmabuf_fd), map_size_(map_size), stride_(stride) {
Can you retain the gfx::size in the class? It might become necessary for the
ioctls. Or other uses. Otherwise, I agree with the thrust of the CL.

[sadrul, 2016-09-02 19:07:17]

https://codereview.chromium.org/2302053003/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2302053003/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:74: :
dmabuf_fd_(dmabuf_fd), map_size_(map_size), stride_(stride) {
On 2016/09/02 19:02:03, rjkroege wrote:
> Can you retain the gfx::size in the class? It might become necessary for the
> ioctls. Or other uses. Otherwise, I agree with the thrust of the CL.

Done.

[sadrul, 2016-09-02 19:07:20]

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

[commit-bot: I haz the power, 2016-09-02 19:08:00]

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

[rjkroege, 2016-09-02 19:11:02]

lgtm

[sadrul, 2016-09-02 19:13:30]

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

[commit-bot: I haz the power, 2016-09-02 19:13:45]

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-02 19:24:09]

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

[commit-bot: I haz the power, 2016-09-02 19:24:10]

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_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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...)

[sadrul, 2016-09-02 19:26:30]

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

[commit-bot: I haz the power, 2016-09-02 19:26:56]

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-02 20:15:51]

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

[commit-bot: I haz the power, 2016-09-02 20:15:51]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-09-02 20:26:14]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-09-02 20:26:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2302053003/#ps80001
(title: ".")

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

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-02 20:31:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-02 20:33:03]

Description was changed from

==========
ozone: Validate the memory buffer used.

BUG=none
==========

to

==========
ozone: Validate the memory buffer used.

BUG=none

Committed: https://crrev.com/35ab4d767861e705756943e5936baf09891fc012
Cr-Commit-Position: refs/heads/master@{#416335}
==========

[commit-bot: I haz the power, 2016-09-02 20:33:05]

Patchset 5 (id:??) landed as
https://crrev.com/35ab4d767861e705756943e5936baf09891fc012
Cr-Commit-Position: refs/heads/master@{#416335}