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

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index bd7bed5278f045b9e79c7fbbed2eba291918d3fc..7c4964fcfaf46ff04d1447aa4f5cead1d13dea01 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -15215,7 +15215,7 @@ error::Error GLES2DecoderImpl::HandleGetProgramInfoCHROMIUM(
 error::Error GLES2DecoderImpl::HandleGetUniformBlocksCHROMIUM(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::GetUniformBlocksCHROMIUM& c =
       *static_cast<const volatile gles2::cmds::GetUniformBlocksCHROMIUM*>(
@@ -15236,7 +15236,7 @@ error::Error GLES2DecoderImpl::HandleGetUniformBlocksCHROMIUM(
 error::Error GLES2DecoderImpl::HandleGetUniformsES3CHROMIUM(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::GetUniformsES3CHROMIUM& c =
       *static_cast<const volatile gles2::cmds::GetUniformsES3CHROMIUM*>(
@@ -15257,7 +15257,7 @@ error::Error GLES2DecoderImpl::HandleGetUniformsES3CHROMIUM(
 error::Error GLES2DecoderImpl::HandleGetTransformFeedbackVarying(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::GetTransformFeedbackVarying& c =
       *static_cast<const volatile gles2::cmds::GetTransformFeedbackVarying*>(
@@ -15288,6 +15288,13 @@ error::Error GLES2DecoderImpl::HandleGetTransformFeedbackVarying(
         "glGetTransformFeedbackVarying", "program not linked");
     return error::kNoError;
   }
+  GLint num_varyings = 0;
+  glGetProgramiv(service_id, GL_TRANSFORM_FEEDBACK_VARYINGS, &num_varyings);
+  if (index >= static_cast<GLuint>(num_varyings)) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_VALUE,
+        "glGetTransformFeedbackVarying", "index out of bounds");
+    return error::kNoError;
+  }
   GLint max_length = 0;
   glGetProgramiv(
       service_id, GL_TRANSFORM_FEEDBACK_VARYING_MAX_LENGTH, &max_length);
@@ -15296,14 +15303,8 @@ error::Error GLES2DecoderImpl::HandleGetTransformFeedbackVarying(
   GLsizei length = 0;
   GLsizei size = 0;
   GLenum type = 0;
-  LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER("GetTransformFeedbackVarying");
   glGetTransformFeedbackVarying(
       service_id, index, max_length, &length, &size, &type, &buffer[0]);
-  GLenum error = glGetError();
-  if (error != GL_NO_ERROR) {
-    LOCAL_SET_GL_ERROR(error, "glGetTransformFeedbackVarying", "");
-    return error::kNoError;
-  }
   result->success = 1;  // true.
   result->size = static_cast<int32_t>(size);
   result->type = static_cast<uint32_t>(type);
@@ -15317,7 +15318,7 @@ error::Error GLES2DecoderImpl::HandleGetTransformFeedbackVarying(
 error::Error GLES2DecoderImpl::HandleGetTransformFeedbackVaryingsCHROMIUM(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::GetTransformFeedbackVaryingsCHROMIUM& c =
       *static_cast<
@@ -17146,7 +17147,7 @@ error::Error GLES2DecoderImpl::HandleUniformBlockBinding(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
   const char* func_name = "glUniformBlockBinding";
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::UniformBlockBinding& c =
       *static_cast<const volatile gles2::cmds::UniformBlockBinding*>(cmd_data);
@@ -17177,7 +17178,7 @@ error::Error GLES2DecoderImpl::HandleClientWaitSync(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
   const char* function_name = "glClientWaitSync";
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::ClientWaitSync& c =
       *static_cast<const volatile gles2::cmds::ClientWaitSync*>(cmd_data);
@@ -17226,7 +17227,7 @@ error::Error GLES2DecoderImpl::HandleClientWaitSync(
 error::Error GLES2DecoderImpl::HandleWaitSync(uint32_t immediate_data_size,
                                               const volatile void* cmd_data) {
   const char* function_name = "glWaitSync";
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::WaitSync& c =
       *static_cast<const volatile gles2::cmds::WaitSync*>(cmd_data);
@@ -17266,7 +17267,7 @@ GLsync GLES2DecoderImpl::DoFenceSync(GLenum condition, GLbitfield flags) {
 error::Error GLES2DecoderImpl::HandleGetInternalformativ(
     uint32_t immediate_data_size,
     const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled())
+  if (!feature_info_->IsWebGL2OrES3Context())
     return error::kUnknownCommand;
   const volatile gles2::cmds::GetInternalformativ& c =
       *static_cast<const volatile gles2::cmds::GetInternalformativ*>(cmd_data);
@@ -17278,7 +17279,8 @@ error::Error GLES2DecoderImpl::HandleGetInternalformativ(
     return error::kNoError;
   }
   if (!validators_->render_buffer_format.IsValid(format)) {
-    LOCAL_SET_GL_ERROR_INVALID_ENUM("glGetInternalformativ", format, "format");
+    LOCAL_SET_GL_ERROR_INVALID_ENUM("glGetInternalformativ", format,
+                                    "internalformat");
     return error::kNoError;
   }
   if (!validators_->internal_format_parameter.IsValid(pname)) {
@@ -17343,8 +17345,13 @@ error::Error GLES2DecoderImpl::HandleGetInternalformativ(
         params[0] = static_cast<GLint>(samples.size());
         break;
       case GL_SAMPLES:
-        for (size_t ii = 0; ii < samples.size(); ++ii) {
-          params[ii] = samples[ii];
+        {
+          // TODO(kainino): should add a test for this code if it's kept

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

Need to resolve this before committing.

+          size_t copyCount = std::min(
+              samples.size(), static_cast<size_t>(result->GetNumResults()));

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

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.

+          for (size_t ii = 0; ii < copyCount; ++ii) {
+            params[ii] = samples[ii];
+          }
         }
         break;
       default:
@@ -17360,7 +17367,7 @@ error::Error GLES2DecoderImpl::HandleGetInternalformativ(
 
 error::Error GLES2DecoderImpl::HandleMapBufferRange(
     uint32_t immediate_data_size, const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled()) {
+  if (!feature_info_->IsWebGL2OrES3Context()) {
     return error::kUnknownCommand;
   }
 
@@ -17471,7 +17478,7 @@ error::Error GLES2DecoderImpl::HandleMapBufferRange(
 
 error::Error GLES2DecoderImpl::HandleUnmapBuffer(
     uint32_t immediate_data_size, const volatile void* cmd_data) {
-  if (!unsafe_es3_apis_enabled()) {
+  if (!feature_info_->IsWebGL2OrES3Context()) {
     return error::kUnknownCommand;
   }
   const char* func_name = "glUnmapBuffer";