Use safe math in ValidateCopyBufferSubData.

src/libANGLE/validationES3.cpp

@@ -8,6 +8,9 @@
 
 #include "libANGLE/validationES3.h"
 
+#include "base/numerics/safe_conversions.h"
+#include "common/mathutil.h"
+#include "common/utilities.h"
 #include "libANGLE/validationES.h"
 #include "libANGLE/Context.h"
 #include "libANGLE/Texture.h"
@@ -16,9 +19,6 @@
 #include "libANGLE/formatutils.h"
 #include "libANGLE/FramebufferAttachment.h"
 
-#include "common/mathutil.h"
-#include "common/utilities.h"
-
 using namespace angle;
 
 namespace gl
@@ -1948,13 +1948,14 @@ bool ValidateCopyBufferSubData(ValidationContext *context,
 {
     if (context->getClientMajorVersion() < 3)
     {
-        context->handleError(Error(GL_INVALID_OPERATION));
+        context->handleError(
+            Error(GL_INVALID_OPERATION, "CopyBufferSubData requires ES 3 or greater"));
         return false;
     }
 
     if (!ValidBufferTarget(context, readTarget) || !ValidBufferTarget(context, writeTarget))
     {
-        context->handleError(Error(GL_INVALID_ENUM));
+        context->handleError(Error(GL_INVALID_ENUM, "Invalid buffer target"));
         return false;
     }
 
@@ -1963,31 +1964,68 @@ bool ValidateCopyBufferSubData(ValidationContext *context,
 
     if (!readBuffer || !writeBuffer)
     {
-        context->handleError(Error(GL_INVALID_OPERATION));
+        context->handleError(Error(GL_INVALID_OPERATION, "No buffer bound to target"));
         return false;
     }
 
     // Verify that readBuffer and writeBuffer are not currently mapped
     if (readBuffer->isMapped() || writeBuffer->isMapped())
     {
-        context->handleError(Error(GL_INVALID_OPERATION));
+        context->handleError(
+            Error(GL_INVALID_OPERATION, "Cannot call CopyBufferSubData on a mapped buffer"));
         return false;
     }
 
-    if (readOffset < 0 || writeOffset < 0 || size < 0 ||
-        static_cast<unsigned int>(readOffset + size) > readBuffer->getSize() ||
-        static_cast<unsigned int>(writeOffset + size) > writeBuffer->getSize())
+    CheckedNumeric<GLintptr> checkedReadOffset(readOffset);
+    CheckedNumeric<GLintptr> checkedWriteOffset(writeOffset);
+    CheckedNumeric<GLintptr> checkedSize(size);
+
+    auto checkedReadSum  = checkedReadOffset + checkedSize;
+    auto checkedWriteSum = checkedWriteOffset + checkedSize;
+
+    if (!checkedReadSum.IsValid() || !checkedWriteSum.IsValid() ||
+        !IsValueInRangeForNumericType<GLintptr>(readBuffer->getSize()) ||
+        !IsValueInRangeForNumericType<GLintptr>(writeBuffer->getSize()))
     {
-        context->handleError(Error(GL_INVALID_VALUE));
+        context->handleError(
+            Error(GL_INVALID_VALUE, "Integer overflow when validating copy offsets."));
         return false;
     }
 
-    if (readBuffer == writeBuffer && std::abs(readOffset - writeOffset) < size)
+    if (readOffset < 0 || writeOffset < 0 || size < 0)
     {
-        context->handleError(Error(GL_INVALID_VALUE));
+        context->handleError(
+            Error(GL_INVALID_VALUE, "readOffset, writeOffset and size must all be non-negative"));
         return false;
     }
 
+    if (checkedReadSum.ValueOrDie() > readBuffer->getSize() ||
+        checkedWriteSum.ValueOrDie() > writeBuffer->getSize())
+    {
+        context->handleError(
+            Error(GL_INVALID_VALUE, "Buffer offset overflow in CopyBufferSubData"));
+        return false;
+    }
+
+    if (readBuffer == writeBuffer)
+    {
+        auto checkedOffsetDiff = (checkedReadOffset - checkedWriteOffset).Abs();
+        if (!checkedOffsetDiff.IsValid())
+        {
+            // This shold not be possible.
+            UNREACHABLE();
+            context->handleError(
+                Error(GL_INVALID_VALUE, "Integer overflow when validating same buffer copy."));
+            return false;
+        }
+
+        if (checkedOffsetDiff.ValueOrDie() < size)
+        {
+            context->handleError(Error(GL_INVALID_VALUE));
+            return false;
+        }
+    }
+
     return true;
 }
 

src/tests/gl_tests/BufferDataTest.cpp

@@ -5,6 +5,7 @@
 //
 
 #include "test_utils/ANGLETest.h"
+#include "test_utils/gl_raii.h"
 
 #include <stdint.h>
 
@@ -447,24 +448,8 @@ class BufferDataOverflowTest : public ANGLETest
 {
   protected:
     BufferDataOverflowTest()
-        : mProgram(0)
     {
     }
-
-    ~BufferDataOverflowTest()
-    {
-        if (!mBuffers.empty())
-        {
-            glDeleteBuffers(static_cast<GLsizei>(mBuffers.size()), &mBuffers[0]);
-        }
-        if (mProgram != 0u)
-        {
-            glDeleteProgram(mProgram);
-        }
-    }
-
-    std::vector<GLuint> mBuffers;
-    GLuint mProgram;
 };
 
 // See description above.
@@ -472,9 +457,9 @@ TEST_P(BufferDataOverflowTest, VertexBufferIntegerOverflow)
 {
     // These values are special, to trigger the rounding bug.
     unsigned int numItems = 0x7FFFFFE;
-    GLsizei bufferCnt = 8;
+    constexpr GLsizei bufferCnt = 8;
 
-    mBuffers.resize(bufferCnt);
+    std::vector<GLBuffer> buffers(bufferCnt);
 
     std::stringstream vertexShaderStr;
 
@@ -502,30 +487,27 @@ TEST_P(BufferDataOverflowTest, VertexBufferIntegerOverflow)
         "  gl_FragColor = vec4(v_attrib, 0, 0, 1);\n"
         "}";
 
-    mProgram = CompileProgram(vertexShaderStr.str(), fragmentShader);
-    ASSERT_NE(0u, mProgram);
-    glUseProgram(mProgram);
-
-    glGenBuffers(bufferCnt, &mBuffers[0]);
+    ANGLE_GL_PROGRAM(program, vertexShaderStr.str(), fragmentShader);
+    glUseProgram(program.get());
 
     std::vector<GLfloat> data(numItems, 1.0f);
 
     for (GLsizei bufferIndex = 0; bufferIndex < bufferCnt; ++bufferIndex)
     {
-        glBindBuffer(GL_ARRAY_BUFFER, mBuffers[bufferIndex]);
+        glBindBuffer(GL_ARRAY_BUFFER, buffers[bufferIndex].get());
         glBufferData(GL_ARRAY_BUFFER, numItems * sizeof(float), &data[0], GL_DYNAMIC_DRAW);
 
         std::stringstream attribNameStr;
         attribNameStr << "attrib" << bufferIndex;
 
-        GLint attribLocation = glGetAttribLocation(mProgram, attribNameStr.str().c_str());
+        GLint attribLocation = glGetAttribLocation(program.get(), attribNameStr.str().c_str());
         ASSERT_NE(-1, attribLocation);
 
         glVertexAttribPointer(attribLocation, 1, GL_FLOAT, GL_FALSE, 4, nullptr);
         glEnableVertexAttribArray(attribLocation);
     }
 
-    GLint positionLocation = glGetAttribLocation(mProgram, "position");
+    GLint positionLocation = glGetAttribLocation(program.get(), "position");
     ASSERT_NE(-1, positionLocation);
     glDisableVertexAttribArray(positionLocation);
     glVertexAttrib2f(positionLocation, 1.0f, 1.0f);
@@ -535,6 +517,28 @@ TEST_P(BufferDataOverflowTest, VertexBufferIntegerOverflow)
     EXPECT_GL_ERROR(GL_OUT_OF_MEMORY);
 }
 
+// Tests a security bug in our CopyBufferSubData validation (integer overflow).
+TEST_P(BufferDataOverflowTest, CopySubDataValidation)
+{
+    GLBuffer readBuffer, writeBuffer;
+
+    glBindBuffer(GL_COPY_READ_BUFFER, readBuffer.get());
+    glBindBuffer(GL_COPY_WRITE_BUFFER, writeBuffer.get());
+
+    constexpr int bufSize = 100;
+
+    glBufferData(GL_COPY_READ_BUFFER, bufSize, nullptr, GL_STATIC_DRAW);
+    glBufferData(GL_COPY_WRITE_BUFFER, bufSize, nullptr, GL_STATIC_DRAW);
+
+    GLintptr big = std::numeric_limits<GLintptr>::max() - bufSize + 90;
+
+    glCopyBufferSubData(GL_COPY_READ_BUFFER, GL_COPY_WRITE_BUFFER, big, 0, 50);
+    EXPECT_GL_ERROR(GL_INVALID_VALUE);
+
+    glCopyBufferSubData(GL_COPY_READ_BUFFER, GL_COPY_WRITE_BUFFER, 0, big, 50);
+    EXPECT_GL_ERROR(GL_INVALID_VALUE);
+}
+
 ANGLE_INSTANTIATE_TEST(BufferDataOverflowTest, ES3_D3D11());
 
 #endif  // _WIN64