command buffer: audit validation of ES3 commands (part 3)

command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context()' on the newly audited commands.

BUG=654787
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/11c1b392ed36430a40e36a1886e6c672137e468c
Cr-Commit-Position: refs/heads/master@{#429059}

[Kai Ninomiya, 2016-11-01 00:38:45]

Description was changed from

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context' on the newly audited commands.

BUG=654787
==========

to

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context' on the newly audited commands.

BUG=654787
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
==========

[Kai Ninomiya, 2016-11-01 00:40:02]

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

[commit-bot: I haz the power, 2016-11-01 00:40:31]

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-11-01 01:20:24]

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

[commit-bot: I haz the power, 2016-11-01 01:20:25]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Kai Ninomiya, 2016-11-01 01:36:58]

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

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

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

[Kai Ninomiya, 2016-11-01 01:38:16]

The CQ bit was unchecked by kainino@chromium.org

[Kai Ninomiya, 2016-11-01 01:40:29]

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

https://codereview.chromium.org/2470623002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17349: // TODO(kainino): should
add a test for this code if it's kept
Need to resolve this before committing.

[Kai Ninomiya, 2016-11-01 01:56:22]

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

[commit-bot: I haz the power, 2016-11-01 01:56:36]

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

[Kai Ninomiya, 2016-11-01 01:56:39]

Description was changed from

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context' on the newly audited commands.

BUG=654787
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

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context()' on the newly audited commands.

BUG=654787
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
==========

[piman, 2016-11-01 02:38:15]

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

[piman, 2016-11-01 02:38:15]

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

https://codereview.chromium.org/2470623002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17351: samples.size(),
static_cast<size_t>(result->GetNumResults()));
Drive-by: you don't want to read result->GetNumResults(), it's not set yet (set
on l.17364) and even if it was set, it would be a security issue.

|result| resides in shared memory, which is writable by the client. The
(untrusted) client could race and modify this value between when it's set and
when it's read here, meaning we would use a totally untrusted value.

You can use num_values instead. But then again, samples.size() == num_values in
the GL_SAMPLES case, so this logic is not needed.
The code on l.17332 made sure the result contains space for num_values entries.

[Kai Ninomiya, 2016-11-01 02:58:51]

On 2016/11/01 02:38:15, piman wrote:
>
https://codereview.chromium.org/2470623002/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2470623002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:17351: samples.size(),
> static_cast<size_t>(result->GetNumResults()));
> Drive-by: you don't want to read result->GetNumResults(), it's not set yet
(set
> on l.17364) and even if it was set, it would be a security issue.
> 
> |result| resides in shared memory, which is writable by the client. The
> (untrusted) client could race and modify this value between when it's set and
> when it's read here, meaning we would use a totally untrusted value.
> 
> You can use num_values instead. But then again, samples.size() == num_values
in
> the GL_SAMPLES case, so this logic is not needed.
> The code on l.17332 made sure the result contains space for num_values
entries.

Thanks piman! That resolves my question of whether it was needed.

[Kai Ninomiya, 2016-11-01 02:59:00]

The CQ bit was unchecked by kainino@chromium.org

[Kai Ninomiya, 2016-11-01 03:06:41]

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

[commit-bot: I haz the power, 2016-11-01 03:06:59]

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

[Kai Ninomiya, 2016-11-01 03:08:24]

The CQ bit was unchecked by kainino@chromium.org

[Kai Ninomiya, 2016-11-01 03:39:10]

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

[commit-bot: I haz the power, 2016-11-01 03:39:25]

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-11-01 04:18:27]

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

[commit-bot: I haz the power, 2016-11-01 04:18:28]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-01 15:52:08]

lgtm

[Kai Ninomiya, 2016-11-01 17:59:13]

kainino@chromium.org changed reviewers:
+ zmo@chromium.org

[Kai Ninomiya, 2016-11-01 17:59:14]

zmo, PTAL as well

[Zhenyao Mo, 2016-11-01 18:10:35]

On 2016/11/01 17:59:14, Kai Ninomiya wrote:
> zmo, PTAL as well

lgtm

[Kai Ninomiya, 2016-11-01 18:26:28]

The CQ bit was checked by kainino@chromium.org

[commit-bot: I haz the power, 2016-11-01 18:26:48]

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-01 18:31:31]

Description was changed from

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context()' on the newly audited commands.

BUG=654787
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

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context()' on the newly audited commands.

BUG=654787
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
==========

[commit-bot: I haz the power, 2016-11-01 18:31:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-01 18:54:25]

Description was changed from

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context()' on the newly audited commands.

BUG=654787
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

==========
command buffer: audit validation of ES3 commands (part 3)

Add some validation, and switch the gating from 'unsafe_es3_apis_enabled()' to
'feature_info_->IsWebGL2OrES3Context()' on the newly audited commands.

BUG=654787
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/11c1b392ed36430a40e36a1886e6c672137e468c
Cr-Commit-Position: refs/heads/master@{#429059}
==========

[commit-bot: I haz the power, 2016-11-01 18:54:26]

Patchset 2 (id:??) landed as
https://crrev.com/11c1b392ed36430a40e36a1886e6c672137e468c
Cr-Commit-Position: refs/heads/master@{#429059}