Patch the results of queries that return object IDs in the passthrough cmd decoder.

Patch the results of queries that return object IDs in the passthrough cmd decoder.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7b76cdffc97efd9152fb88fdecf4a3890825e0ad
Cr-Commit-Position: refs/heads/master@{#433269}

[Geoff Lang, 2016-11-15 21:57:07]

Description was changed from

==========
Patch the results of queries that return object IDs in the passthrough cmd
decoder.

BUG=602688
==========

to

==========
Patch the results of queries that return object IDs in the passthrough cmd
decoder.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Geoff Lang, 2016-11-16 15:24:49]

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

[commit-bot: I haz the power, 2016-11-16 15:25:18]

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

[Geoff Lang, 2016-11-16 16:15:00]

geofflang@chromium.org changed reviewers:
+ piman@chromium.org

[Geoff Lang, 2016-11-16 16:15:01]

PTAL

[commit-bot: I haz the power, 2016-11-16 16:50:58]

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

[commit-bot: I haz the power, 2016-11-16 16:50:59]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-16 18:55:22]

https://codereview.chromium.org/2503453005/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2503453005/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:878: return
PatchGetNumericResults(pname, *length, params);
This makes me uncomfortable. params is in shared memory, controlled by the
untrusted client. It was written by glGetBooleanvRobustANGLE, but by the time
it's read by PatchGetNumericResults it could have been modified by the
(malicious) client, so PatchGetNumericResults has to defend against this. Also
it means the client could temporarily see the service ids, which is an
information leak (though probably harmless?)

How hard would it be to have the glGet's query into local memory, patch there,
and upon success copy into the shared memory?

[Geoff Lang, 2016-11-17 19:32:01]

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

[commit-bot: I haz the power, 2016-11-17 19:34:25]

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

[Geoff Lang, 2016-11-17 19:35:01]

https://codereview.chromium.org/2503453005/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2503453005/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:878: return
PatchGetNumericResults(pname, *length, params);
On 2016/11/16 18:55:22, piman wrote:
> This makes me uncomfortable. params is in shared memory, controlled by the
> untrusted client. It was written by glGetBooleanvRobustANGLE, but by the time
> it's read by PatchGetNumericResults it could have been modified by the
> (malicious) client, so PatchGetNumericResults has to defend against this. Also
> it means the client could temporarily see the service ids, which is an
> information leak (though probably harmless?)
> 
> How hard would it be to have the glGet's query into local memory, patch there,
> and upon success copy into the shared memory?

Good point, I didn't think about these being in shmem.  Updated to do the
queries and patching in a scratch buffer and then copy the results to the shmem.
 Added a test that goes through these code paths.

[commit-bot: I haz the power, 2016-11-17 20:45:16]

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

[commit-bot: I haz the power, 2016-11-17 20:45:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Geoff Lang, 2016-11-17 20:53:43]

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

[commit-bot: I haz the power, 2016-11-17 20:55:37]

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

[piman, 2016-11-17 21:08:52]

LGTM+nits.

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc (right):

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc:558: auto
get_binding_client_id = [this](
nit: this could be a regular template member function, doesn't need to be a
lambda, I think.

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:885: });
nit: would it work to pass glGetBooleanvRobustANGLE directly (which is a
function pointer) instead of adding the lambda? (same below).

[Geoff Lang, 2016-11-17 22:01:15]

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc (right):

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc:558: auto
get_binding_client_id = [this](
On 2016/11/17 21:08:52, piman wrote:
> nit: this could be a regular template member function, doesn't need to be a
> lambda, I think.

Done, added a helper.

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:885: });
On 2016/11/17 21:08:52, piman wrote:
> nit: would it work to pass glGetBooleanvRobustANGLE directly (which is a
> function pointer) instead of adding the lambda? (same below).

This one does need to be a lambda.  The GL entry points are macros that call a
function on the current context.

[Geoff Lang, 2016-11-17 22:02:49]

The CQ bit was checked by geofflang@chromium.org

[Geoff Lang, 2016-11-17 22:02:50]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2503453005/#ps60001
(title: "Helperize GetClientID")

[commit-bot: I haz the power, 2016-11-17 22:04:50]

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

[piman, 2016-11-17 22:09:59]

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:885: });
On 2016/11/17 22:01:15, Geoff Lang wrote:
> On 2016/11/17 21:08:52, piman wrote:
> > nit: would it work to pass glGetBooleanvRobustANGLE directly (which is a
> > function pointer) instead of adding the lambda? (same below).
> 
> This one does need to be a lambda.  The GL entry points are macros that call a
> function on the current context.

The macros should just point to a function pointer member of the current
context, i.e. not actually "call" anything.

[commit-bot: I haz the power, 2016-11-18 00:55:21]

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

[commit-bot: I haz the power, 2016-11-18 00:55:22]

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

[Geoff Lang, 2016-11-18 15:40:08]

On 2016/11/17 22:09:59, piman wrote:
>
https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
(right):
> 
>
https://codereview.chromium.org/2503453005/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:885: });
> On 2016/11/17 22:01:15, Geoff Lang wrote:
> > On 2016/11/17 21:08:52, piman wrote:
> > > nit: would it work to pass glGetBooleanvRobustANGLE directly (which is a
> > > function pointer) instead of adding the lambda? (same below).
> > 
> > This one does need to be a lambda.  The GL entry points are macros that call
a
> > function on the current context.
> 
> The macros should just point to a function pointer member of the current
> context, i.e. not actually "call" anything.

Right, nothing is called but they point to a virtual member function of the
current context, it might be possible to use base::Bind to but it would look
something like Bind(&gl::GLApi::glGetIntegervRobustANGLEFn,
gl::g_current_gl_context) which I think is less readable than the lambda.  Am I
missing something?  There are a few places in the code that would benefit from
easily binding the gl entry points.

[piman, 2016-11-18 18:42:55]

On Fri, Nov 18, 2016 at 7:40 AM, <geofflang@chromium.org> wrote:

> On 2016/11/17 22:09:59, piman wrote:
> >
> https://codereview.chromium.org/2503453005/diff/40001/gpu/
> command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
> > File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
> (right):
> >
> >
> https://codereview.chromium.org/2503453005/diff/40001/gpu/
> command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc#newcode885
> > gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:885:
> });
> > On 2016/11/17 22:01:15, Geoff Lang wrote:
> > > On 2016/11/17 21:08:52, piman wrote:
> > > > nit: would it work to pass glGetBooleanvRobustANGLE directly (which
> is a
> > > > function pointer) instead of adding the lambda? (same below).
> > >
> > > This one does need to be a lambda. The GL entry points are macros that
> call
> a
> > > function on the current context.
> >
> > The macros should just point to a function pointer member of the current
> > context, i.e. not actually "call" anything.
>
> Right, nothing is called but they point to a virtual member function of the
> current context
>

Ah, you're right, this is GLApi::glGetIntegervRobustANGLEFn, which is a
member function, not ProcsGL::glGetIntegervRobustANGLEFn which is a member
field. Nevermind.


> , it might be possible to use base::Bind to but it would look
> something like Bind(&gl::GLApi::glGetIntegervRobustANGLEFn,
> gl::g_current_gl_context) which I think is less readable than the lambda.
> Am I
> missing something? There are a few places in the code that would benefit
> from
> easily binding the gl entry points.
>
> https://codereview.chromium.org/2503453005/
>

-- 
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.

[Geoff Lang, 2016-11-18 18:46:10]

On 2016/11/18 18:42:55, piman wrote:
> On Fri, Nov 18, 2016 at 7:40 AM, <mailto:geofflang@chromium.org> wrote:
> 
> > On 2016/11/17 22:09:59, piman wrote:
> > >
> > https://codereview.chromium.org/2503453005/diff/40001/gpu/
> > command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
> > > File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
> > (right):
> > >
> > >
> > https://codereview.chromium.org/2503453005/diff/40001/gpu/
> > command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc#newcode885
> > > gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:885:
> > });
> > > On 2016/11/17 22:01:15, Geoff Lang wrote:
> > > > On 2016/11/17 21:08:52, piman wrote:
> > > > > nit: would it work to pass glGetBooleanvRobustANGLE directly (which
> > is a
> > > > > function pointer) instead of adding the lambda? (same below).
> > > >
> > > > This one does need to be a lambda. The GL entry points are macros that
> > call
> > a
> > > > function on the current context.
> > >
> > > The macros should just point to a function pointer member of the current
> > > context, i.e. not actually "call" anything.
> >
> > Right, nothing is called but they point to a virtual member function of the
> > current context
> >
> 
> Ah, you're right, this is GLApi::glGetIntegervRobustANGLEFn, which is a
> member function, not ProcsGL::glGetIntegervRobustANGLEFn which is a member
> field. Nevermind.
> 
> 
> > , it might be possible to use base::Bind to but it would look
> > something like Bind(&gl::GLApi::glGetIntegervRobustANGLEFn,
> > gl::g_current_gl_context) which I think is less readable than the lambda.
> > Am I
> > missing something? There are a few places in the code that would benefit
> > from
> > easily binding the gl entry points.
> >
> > https://codereview.chromium.org/2503453005/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Yea, it's unfortunate, a lot of the helpers would love to pass around the entry
points by address.

[Geoff Lang, 2016-11-18 18:46:16]

The CQ bit was checked by geofflang@chromium.org

[commit-bot: I haz the power, 2016-11-18 18:47:08]

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-11-18 19:40:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-18 19:43:38]

Description was changed from

==========
Patch the results of queries that return object IDs in the passthrough cmd
decoder.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Patch the results of queries that return object IDs in the passthrough cmd
decoder.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7b76cdffc97efd9152fb88fdecf4a3890825e0ad
Cr-Commit-Position: refs/heads/master@{#433269}
==========

[commit-bot: I haz the power, 2016-11-18 19:43:39]

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

[lmilko, 2016-11-23 11:38:30]

On 2016/11/18 19:43:39, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/7b76cdffc97efd9152fb88fdecf4a3890825e0ad
> Cr-Commit-Position: refs/heads/master@{#433269}

Added tests are assuming that GLESv3 is supported but they are crashing on
platforms that do not support GLESv3.

[Geoff Lang, 2016-11-23 15:12:39]

On 2016/11/23 11:38:30, lmilko wrote:
> On 2016/11/18 19:43:39, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/7b76cdffc97efd9152fb88fdecf4a3890825e0ad
> > Cr-Commit-Position: refs/heads/master@{#433269}
> 
> Added tests are assuming that GLESv3 is supported but they are crashing on
> platforms that do not support GLESv3.

Sorry about that, fixed here: https://codereview.chromium.org/2527653002