Add MultiDrawManager state invariants and unittests

The gles2_cmd_decoder receives untrusted commands from the client and must
validate that the MultiDrawManager is always used with a valid sequence of
commands.

Bug: 923282
Change-Id: Ic61e8d61fce61431d7aa939261608f5f4f06792d
Reviewed-on: https://chromium-review.googlesource.com/c/1423422
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630083}

gpu/BUILD.gn

@@ -390,6 +390,7 @@ test("gpu_unittests") {
     "command_buffer/service/memory_program_cache_unittest.cc",
     "command_buffer/service/mocks.cc",
     "command_buffer/service/mocks.h",
+    "command_buffer/service/multi_draw_manager_unittest.cc",
     "command_buffer/service/passthrough_program_cache_unittest.cc",
     "command_buffer/service/path_manager_unittest.cc",
     "command_buffer/service/program_cache_unittest.cc",

gpu/command_buffer/service/multi_draw_manager.cc

@@ -12,8 +12,8 @@
 namespace gpu {
 namespace gles2 {
 
-MultiDrawManager::ResultData::ResultData()
-    : draw_function(DrawFunction::None) {}
+MultiDrawManager::ResultData::ResultData() = default;
+MultiDrawManager::ResultData::~ResultData() = default;
 
 MultiDrawManager::ResultData::ResultData(ResultData&& rhs)
     : draw_function(rhs.draw_function),
@@ -25,7 +25,6 @@ MultiDrawManager::ResultData::ResultData(ResultData&& rhs)
       offsets(std::move(rhs.offsets)),
       indices(std::move(rhs.indices)),
       instance_counts(std::move(rhs.instance_counts)) {
-  rhs.draw_function = DrawFunction::None;
 }
 
 MultiDrawManager::ResultData& MultiDrawManager::ResultData::operator=(
@@ -42,33 +41,32 @@ MultiDrawManager::ResultData& MultiDrawManager::ResultData::operator=(
   std::swap(offsets, rhs.offsets);
   std::swap(indices, rhs.indices);
   std::swap(instance_counts, rhs.instance_counts);
-
-  rhs.draw_function = DrawFunction::None;
   return *this;
 }
 
-MultiDrawManager::ResultData::~ResultData() {}
-
 MultiDrawManager::MultiDrawManager(IndexStorageType index_type)
-    : current_draw_offset_(0), index_type_(index_type), result_() {}
+    : draw_state_(DrawState::End),
+      current_draw_offset_(0),
+      index_type_(index_type),
+      result_() {}
 
 bool MultiDrawManager::Begin(GLsizei drawcount) {
-  result_.drawcount = drawcount;
-  current_draw_offset_ = 0;
-  if (result_.draw_function != DrawFunction::None) {
-    NOTREACHED();
+  if (draw_state_ != DrawState::End) {
     return false;
   }
+  result_.drawcount = drawcount;
+  current_draw_offset_ = 0;
+  draw_state_ = DrawState::Begin;
   return true;
 }
 
 bool MultiDrawManager::End(ResultData* result) {
   DCHECK(result);
-
-  if (result_.draw_function == DrawFunction::None ||
+  if (draw_state_ != DrawState::Draw ||
       current_draw_offset_ != result_.drawcount) {
     return false;
   }
+  draw_state_ = DrawState::End;
   *result = std::move(result_);
   return true;
 }
@@ -77,10 +75,7 @@ bool MultiDrawManager::MultiDrawArrays(GLenum mode,
                                        const GLint* firsts,
                                        const GLsizei* counts,
                                        GLsizei drawcount) {
-  if (!EnsureDrawArraysFunction(DrawFunction::DrawArrays, mode) ||
-      base::CheckAdd(current_draw_offset_, drawcount).ValueOrDie() >
-          result_.drawcount) {
-    NOTREACHED();
+  if (!EnsureDrawArraysFunction(DrawFunction::DrawArrays, mode, drawcount)) {
     return false;
   }
   std::copy(firsts, firsts + drawcount, &result_.firsts[current_draw_offset_]);
@@ -94,10 +89,8 @@ bool MultiDrawManager::MultiDrawArraysInstanced(GLenum mode,
                                                 const GLsizei* counts,
                                                 const GLsizei* instance_counts,
                                                 GLsizei drawcount) {
-  if (!EnsureDrawArraysFunction(DrawFunction::DrawArraysInstanced, mode) ||
-      base::CheckAdd(current_draw_offset_, drawcount).ValueOrDie() >
-          result_.drawcount) {
-    NOTREACHED();
+  if (!EnsureDrawArraysFunction(DrawFunction::DrawArraysInstanced, mode,
+                                drawcount)) {
     return false;
   }
   std::copy(firsts, firsts + drawcount, &result_.firsts[current_draw_offset_]);
@@ -113,10 +106,8 @@ bool MultiDrawManager::MultiDrawElements(GLenum mode,
                                          GLenum type,
                                          const GLsizei* offsets,
                                          GLsizei drawcount) {
-  if (!EnsureDrawElementsFunction(DrawFunction::DrawElements, mode, type) ||
-      base::CheckAdd(current_draw_offset_, drawcount).ValueOrDie() >
-          result_.drawcount) {
-    NOTREACHED();
+  if (!EnsureDrawElementsFunction(DrawFunction::DrawElements, mode, type,
+                                  drawcount)) {
     return false;
   }
   std::copy(counts, counts + drawcount, &result_.counts[current_draw_offset_]);
@@ -145,10 +136,7 @@ bool MultiDrawManager::MultiDrawElementsInstanced(
     const GLsizei* instance_counts,
     GLsizei drawcount) {
   if (!EnsureDrawElementsFunction(DrawFunction::DrawElementsInstanced, mode,
-                                  type) ||
-      base::CheckAdd(current_draw_offset_, drawcount).ValueOrDie() >
-          result_.drawcount) {
-    NOTREACHED();
+                                  type, drawcount)) {
     return false;
   }
   std::copy(counts, counts + drawcount, &result_.counts[current_draw_offset_]);
@@ -199,30 +187,65 @@ void MultiDrawManager::ResizeArrays() {
   }
 }
 
+bool MultiDrawManager::ValidateDrawcount(GLsizei drawcount) const {
+  if (drawcount < 0) {
+    return false;
+  }
+  GLsizei new_offset;
+  if (!base::CheckAdd(current_draw_offset_, drawcount)
+           .AssignIfValid(&new_offset)) {
+    return false;
+  }
+  if (new_offset > result_.drawcount) {
+    return false;
+  }
+  return true;
+}
+
 bool MultiDrawManager::EnsureDrawArraysFunction(DrawFunction draw_function,
-                                                GLenum mode) {
-  bool first_call = result_.draw_function == DrawFunction::None;
+                                                GLenum mode,
+                                                GLsizei drawcount) {
+  if (!ValidateDrawcount(drawcount)) {
+    return false;
+  }
+  bool invalid_draw_state = draw_state_ == DrawState::End;
+  bool first_call = draw_state_ == DrawState::Begin;
   bool enums_match = result_.mode == mode;
+
+  if (invalid_draw_state || (!first_call && !enums_match)) {
+    return false;
+  }
   if (first_call) {
+    draw_state_ = DrawState::Draw;
     result_.draw_function = draw_function;
     result_.mode = mode;
     ResizeArrays();
   }
-  return first_call || enums_match;
+  return true;
 }
 
 bool MultiDrawManager::EnsureDrawElementsFunction(DrawFunction draw_function,
                                                   GLenum mode,
-                                                  GLenum type) {
-  bool first_call = result_.draw_function == DrawFunction::None;
+                                                  GLenum type,
+                                                  GLsizei drawcount) {
+  if (!ValidateDrawcount(drawcount)) {
+    return false;
+  }
+  bool invalid_draw_state = draw_state_ == DrawState::End;
+  bool first_call = draw_state_ == DrawState::Begin;
   bool enums_match = result_.mode == mode && result_.type == type;
+
+  if (invalid_draw_state || (!first_call && !enums_match)) {
+    return false;
+  }
   if (first_call) {
+    draw_state_ = DrawState::Draw;
     result_.draw_function = draw_function;
     result_.mode = mode;
     result_.type = type;
     ResizeArrays();
   }
-  return first_call || enums_match;
+  return true;
 }
 
 }  // namespace gles2

gpu/command_buffer/service/multi_draw_manager.h

@@ -20,14 +20,13 @@ namespace gles2 {
 class GPU_GLES2_EXPORT MultiDrawManager {
  public:
   enum class DrawFunction {
-    None,
     DrawArrays,
     DrawArraysInstanced,
     DrawElements,
     DrawElementsInstanced,
   };
 
-  struct ResultData {
+  struct GPU_GLES2_EXPORT ResultData {
     DrawFunction draw_function;
     GLsizei drawcount;
     GLenum mode;
@@ -39,9 +38,9 @@ class GPU_GLES2_EXPORT MultiDrawManager {
     std::vector<GLsizei> instance_counts;
 
     ResultData();
+    ~ResultData();
     ResultData(ResultData&& rhs);
     ResultData& operator=(ResultData&& rhs);
-    ~ResultData();
   };
 
   enum class IndexStorageType {
@@ -76,11 +75,22 @@ class GPU_GLES2_EXPORT MultiDrawManager {
 
  private:
   void ResizeArrays();
-  bool EnsureDrawArraysFunction(DrawFunction draw_function, GLenum mode);
+  bool ValidateDrawcount(GLsizei drawcount) const;
+  bool EnsureDrawArraysFunction(DrawFunction draw_function,
+                                GLenum mode,
+                                GLsizei drawcount);
   bool EnsureDrawElementsFunction(DrawFunction draw_function,
                                   GLenum mode,
-                                  GLenum type);
+                                  GLenum type,
+                                  GLsizei drawcount);
+
+  enum class DrawState {
+    Begin,
+    Draw,
+    End,
+  };
 
+  DrawState draw_state_;
   GLsizei current_draw_offset_;
   IndexStorageType index_type_;
   ResultData result_;

gpu/command_buffer/service/multi_draw_manager_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "gpu/command_buffer/service/multi_draw_manager.h"
+
+#include <memory>
+#include <tuple>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gl/gl_bindings.h"
+
+namespace gpu {
+namespace gles2 {
+
+namespace {
+
+using Param = std::tuple<MultiDrawManager::IndexStorageType,
+                         MultiDrawManager::DrawFunction>;
+
+}  // namespace
+
+class MultiDrawManagerTest : public testing::TestWithParam<Param> {
+ public:
+  MultiDrawManagerTest()
+      : multi_draw_manager_(new MultiDrawManager(std::get<0>(GetParam()))) {}
+
+ protected:
+  bool DoMultiDraw(uint32_t count,
+                   GLenum mode = GL_TRIANGLES,
+                   GLenum type = GL_UNSIGNED_INT) {
+    std::vector<GLsizei> data(count);
+    switch (std::get<1>(GetParam())) {
+      case MultiDrawManager::DrawFunction::DrawArrays:
+        return multi_draw_manager_->MultiDrawArrays(mode, data.data(),
+                                                    data.data(), count);
+
+      case MultiDrawManager::DrawFunction::DrawArraysInstanced:
+        return multi_draw_manager_->MultiDrawArraysInstanced(
+            mode, data.data(), data.data(), data.data(), count);
+
+      case MultiDrawManager::DrawFunction::DrawElements:
+        return multi_draw_manager_->MultiDrawElements(mode, data.data(), type,
+                                                      data.data(), count);
+
+      case MultiDrawManager::DrawFunction::DrawElementsInstanced:
+        return multi_draw_manager_->MultiDrawElementsInstanced(
+            mode, data.data(), type, data.data(), data.data(), count);
+    }
+  }
+
+  void CheckResultSize(uint32_t count,
+                       const MultiDrawManager::ResultData& result) {
+    MultiDrawManager::DrawFunction draw_function = std::get<1>(GetParam());
+    EXPECT_TRUE(draw_function == result.draw_function);
+
+    switch (draw_function) {
+      case MultiDrawManager::DrawFunction::DrawArraysInstanced:
+        EXPECT_TRUE(result.instance_counts.size() == count);
+        FALLTHROUGH;
+      case MultiDrawManager::DrawFunction::DrawArrays:
+        EXPECT_TRUE(result.firsts.size() == count);
+        EXPECT_TRUE(result.counts.size() == count);
+        break;
+      case MultiDrawManager::DrawFunction::DrawElementsInstanced:
+        EXPECT_TRUE(result.instance_counts.size() == count);
+        FALLTHROUGH;
+      case MultiDrawManager::DrawFunction::DrawElements:
+        EXPECT_TRUE(result.counts.size() == count);
+        switch (std::get<0>(GetParam())) {
+          case MultiDrawManager::IndexStorageType::Offset:
+            EXPECT_TRUE(result.offsets.size() == count);
+            break;
+          case MultiDrawManager::IndexStorageType::Pointer:
+            EXPECT_TRUE(result.indices.size() == count);
+            break;
+        }
+        break;
+    }
+  }
+
+  std::unique_ptr<MultiDrawManager> multi_draw_manager_;
+};
+
+// Test that the simple case succeeds
+TEST_P(MultiDrawManagerTest, Success) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(100));
+  EXPECT_TRUE(multi_draw_manager_->End(&result));
+  CheckResultSize(100, result);
+}
+
+// Test that the internal state of the multi draw manager resets such that
+// successive valid multi draws can be executed
+TEST_P(MultiDrawManagerTest, SuccessAfterSuccess) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(100));
+  EXPECT_TRUE(multi_draw_manager_->End(&result));
+  CheckResultSize(100, result);
+
+  EXPECT_TRUE(multi_draw_manager_->Begin(1000));
+  EXPECT_TRUE(DoMultiDraw(1000));
+  EXPECT_TRUE(multi_draw_manager_->End(&result));
+  CheckResultSize(1000, result);
+}
+
+// Test that multiple chunked multi draw calls succeed
+TEST_P(MultiDrawManagerTest, SuccessMultiple) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(83));
+  EXPECT_TRUE(DoMultiDraw(4));
+  EXPECT_TRUE(DoMultiDraw(13));
+  EXPECT_TRUE(multi_draw_manager_->End(&result));
+  CheckResultSize(100, result);
+}
+
+// Test that it is invalid to submit an empty multi draw
+TEST_P(MultiDrawManagerTest, Empty) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(0));
+  EXPECT_FALSE(multi_draw_manager_->End(&result));
+}
+
+// Test that it is invalid to end a multi draw if it has not been started
+TEST_P(MultiDrawManagerTest, EndBeforeBegin) {
+  MultiDrawManager::ResultData result;
+  EXPECT_FALSE(multi_draw_manager_->End(&result));
+}
+
+// Test that it is invalid to begin a multi draw twice
+TEST_P(MultiDrawManagerTest, BeginAfterBegin) {
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_FALSE(multi_draw_manager_->Begin(100));
+}
+
+// Test that it is invalid to begin a multi draw twice, even if
+// the first begin was empty
+TEST_P(MultiDrawManagerTest, BeginAfterEmptyBegin) {
+  EXPECT_TRUE(multi_draw_manager_->Begin(0));
+  EXPECT_FALSE(multi_draw_manager_->Begin(100));
+}
+
+// Test that it is invalid to do a multi draw before begin
+TEST_P(MultiDrawManagerTest, DrawBeforeBegin) {
+  EXPECT_FALSE(DoMultiDraw(1));
+}
+
+// Test that it is invalid to end a multi draw twice
+TEST_P(MultiDrawManagerTest, DoubleEnd) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(1));
+  EXPECT_TRUE(DoMultiDraw(1));
+  EXPECT_TRUE(multi_draw_manager_->End(&result));
+  EXPECT_FALSE(multi_draw_manager_->End(&result));
+}
+
+// Test that it is invalid to end a multi draw before the drawcount
+// is saturated
+TEST_P(MultiDrawManagerTest, Underflow) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(99));
+  EXPECT_FALSE(multi_draw_manager_->End(&result));
+}
+
+// Test that it is invalid to end a multi draw before the drawcount
+// is saturated, using multiple chunks
+TEST_P(MultiDrawManagerTest, UnderflowMultiple) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(42));
+  EXPECT_TRUE(DoMultiDraw(31));
+  EXPECT_TRUE(DoMultiDraw(26));
+  EXPECT_FALSE(multi_draw_manager_->End(&result));
+}
+
+// Test that it is invalid to do a multi draw that overflows the drawcount
+TEST_P(MultiDrawManagerTest, Overflow) {
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_FALSE(DoMultiDraw(101));
+}
+
+// Test that it is invalid to do a multi draw that overflows the drawcount,
+// using multiple chunks
+TEST_P(MultiDrawManagerTest, OverflowMultiple) {
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(31));
+  EXPECT_TRUE(DoMultiDraw(49));
+  EXPECT_FALSE(DoMultiDraw(21));
+}
+
+// Test that it is invalid to do a multi draw that does not match the first
+// chunk's draw mode
+TEST_P(MultiDrawManagerTest, DrawModeMismatch) {
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(50, GL_TRIANGLES));
+  EXPECT_FALSE(DoMultiDraw(50, GL_LINES));
+}
+
+// Test that it is invalid to do a multi draw that does not match the first
+// chunk's element type
+TEST_P(MultiDrawManagerTest, ElementTypeMismatch) {
+  MultiDrawManager::DrawFunction draw_function = std::get<1>(GetParam());
+  if (draw_function != MultiDrawManager::DrawFunction::DrawElements &&
+      draw_function != MultiDrawManager::DrawFunction::DrawElementsInstanced) {
+    return;
+  }
+
+  MultiDrawManager::ResultData result;
+  EXPECT_TRUE(multi_draw_manager_->Begin(100));
+  EXPECT_TRUE(DoMultiDraw(50, GL_TRIANGLES, GL_UNSIGNED_INT));
+  EXPECT_FALSE(DoMultiDraw(50, GL_TRIANGLES, GL_UNSIGNED_SHORT));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ,
+    MultiDrawManagerTest,
+    testing::Combine(
+        testing::Values(MultiDrawManager::IndexStorageType::Offset,
+                        MultiDrawManager::IndexStorageType::Pointer),
+        testing::Values(
+            MultiDrawManager::DrawFunction::DrawArrays,
+            MultiDrawManager::DrawFunction::DrawArraysInstanced,
+            MultiDrawManager::DrawFunction::DrawElements,
+            MultiDrawManager::DrawFunction::DrawElementsInstanced)));
+
+}  // namespace gles2
+}  // namespace gpu