Add validation for the pack buffer in ReadPixels

BUG=angleproject:1512 Change-Id: Ia6bac628c35f04bc5d3adfde1569902475519698 Reviewed-on: https://chromium-review.googlesource.com/387668 Commit-Queue: Corentin Wallez <[email protected]> Reviewed-by: Geoff Lang <[email protected]> Reviewed-by: Jamie Madill <[email protected]>
fjhenigman · Sep 27, 2016 · ece7c5a · ece7c5a
1 parent 2cacb77
commit ece7c5a
Show file tree

Hide file tree

Showing 5 changed files with 192 additions and 82 deletions.
diff --git a/src/libANGLE/formatutils.cpp b/src/libANGLE/formatutils.cpp
@@ -939,6 +939,32 @@ gl::ErrorOrResult<GLuint> InternalFormat::computeSkipBytes(GLuint rowPitch,
     return skipBytes.ValueOrDie();
 }
 
+gl::ErrorOrResult<GLuint> InternalFormat::computePackSize(GLenum formatType,
+                                                          const gl::Extents &size,
+                                                          const gl::PixelPackState &pack) const
+{
+    ASSERT(!compressed);
+
+    if (size.height == 0)
+    {
+        return 0;
+    }
+
+    CheckedNumeric<GLuint> rowPitch;
+    ANGLE_TRY_RESULT(computeRowPitch(formatType, size.width, pack.alignment, pack.rowLength),
+                     rowPitch);
+
+    CheckedNumeric<GLuint> heightMinusOne = size.height - 1;
+    CheckedNumeric<GLuint> bytes          = computePixelBytes(formatType);
+
+    CheckedNumeric<GLuint> totalSize = heightMinusOne * rowPitch;
+    totalSize += size.width * bytes;
+
+    ANGLE_TRY_CHECKED_MATH(totalSize);
+
+    return totalSize.ValueOrDie();
+}
+
 gl::ErrorOrResult<GLuint> InternalFormat::computeUnpackSize(
     GLenum formatType,
     const gl::Extents &size,
@@ -950,6 +976,11 @@ gl::ErrorOrResult<GLuint> InternalFormat::computeUnpackSize(
         return computeCompressedImageSize(formatType, size);
     }
 
+    if (size.height == 0 || size.depth == 0)
+    {
+        return 0;
+    }
+
     CheckedNumeric<GLuint> rowPitch;
     CheckedNumeric<GLuint> depthPitch;
     ANGLE_TRY_RESULT(computeRowPitch(formatType, size.width, unpack.alignment, unpack.rowLength),
@@ -971,6 +1002,26 @@ gl::ErrorOrResult<GLuint> InternalFormat::computeUnpackSize(
     return totalSize.ValueOrDie();
 }
 
+gl::ErrorOrResult<GLuint> InternalFormat::computePackEndByte(GLenum formatType,
+                                                             const gl::Extents &size,
+                                                             const gl::PixelPackState &pack) const
+{
+    GLuint rowPitch;
+    CheckedNumeric<GLuint> checkedSkipBytes;
+    CheckedNumeric<GLuint> checkedCopyBytes;
+
+    ANGLE_TRY_RESULT(computeRowPitch(formatType, size.width, pack.alignment, pack.rowLength),
+                     rowPitch);
+    ANGLE_TRY_RESULT(computeSkipBytes(rowPitch, 0, 0, pack.skipRows, pack.skipPixels, false),
+                     checkedSkipBytes);
+    ANGLE_TRY_RESULT(computePackSize(formatType, size, pack), checkedCopyBytes);
+
+    CheckedNumeric<GLuint> endByte = checkedCopyBytes + checkedSkipBytes;
+
+    ANGLE_TRY_CHECKED_MATH(endByte);
+    return endByte.ValueOrDie();
+}
+
 gl::ErrorOrResult<GLuint> InternalFormat::computeUnpackEndByte(GLenum formatType,
                                                                const gl::Extents &size,
                                                                const gl::PixelUnpackState &unpack,

diff --git a/src/libANGLE/formatutils.h b/src/libANGLE/formatutils.h
@@ -67,10 +67,17 @@ struct InternalFormat
                                                GLint skipRows,
                                                GLint skipPixels,
                                                bool applySkipImages) const;
+
+    gl::ErrorOrResult<GLuint> computePackSize(GLenum formatType,
+                                              const gl::Extents &size,
+                                              const gl::PixelPackState &pack) const;
     gl::ErrorOrResult<GLuint> computeUnpackSize(GLenum formatType,
                                                 const gl::Extents &size,
                                                 const gl::PixelUnpackState &unpack) const;
 
+    gl::ErrorOrResult<GLuint> computePackEndByte(GLenum formatType,
+                                                 const gl::Extents &size,
+                                                 const gl::PixelPackState &pack) const;
     gl::ErrorOrResult<GLuint> computeUnpackEndByte(GLenum formatType,
                                                    const gl::Extents &size,
                                                    const gl::PixelUnpackState &unpack,

diff --git a/src/libANGLE/validationES.cpp b/src/libANGLE/validationES.cpp
@@ -1168,6 +1168,44 @@ bool ValidateReadPixels(ValidationContext *context,
         return false;
     }
 
+    // Check for pixel pack buffer related API errors
+    gl::Buffer *pixelPackBuffer = context->getGLState().getTargetBuffer(GL_PIXEL_PACK_BUFFER);
+    if (pixelPackBuffer != nullptr)
+    {
+        // ..  the data would be packed to the buffer object such that the memory writes required
+        // would exceed the data store size.
+        GLenum sizedInternalFormat       = GetSizedInternalFormat(format, type);
+        const InternalFormat &formatInfo = GetInternalFormatInfo(sizedInternalFormat);
+        const gl::Extents size(width, height, 1);
+        const auto &pack = context->getGLState().getPackState();
+
+        auto endByteOrErr = formatInfo.computePackEndByte(type, size, pack);
+        if (endByteOrErr.isError())
+        {
+            context->handleError(endByteOrErr.getError());
+            return false;
+        }
+
+        CheckedNumeric<size_t> checkedEndByte(endByteOrErr.getResult());
+        CheckedNumeric<size_t> checkedOffset(reinterpret_cast<size_t>(pixels));
+        checkedEndByte += checkedOffset;
+
+        if (checkedEndByte.ValueOrDie() > static_cast<size_t>(pixelPackBuffer->getSize()))
+        {
+            // Overflow past the end of the buffer
+            context->handleError(
+                Error(GL_INVALID_OPERATION, "Writes would overflow the pixel pack buffer."));
+            return false;
+        }
+
+        // ...the buffer object's data store is currently mapped.
+        if (pixelPackBuffer->isMapped())
+        {
+            context->handleError(Error(GL_INVALID_OPERATION, "Pixel pack buffer is mapped."));
+            return false;
+        }
+    }
+
     return true;
 }
 
@@ -1188,30 +1226,20 @@ bool ValidateReadnPixelsEXT(Context *context,
     }
 
     GLenum sizedInternalFormat = GetSizedInternalFormat(format, type);
-    const InternalFormat &sizedFormatInfo = GetInternalFormatInfo(sizedInternalFormat);
-
-    auto outputPitchOrErr =
-        sizedFormatInfo.computeRowPitch(type, width, context->getGLState().getPackAlignment(),
-                                        context->getGLState().getPackRowLength());
+    const InternalFormat &formatInfo = GetInternalFormatInfo(sizedInternalFormat);
+    const gl::Extents size(width, height, 1);
+    const auto &pack = context->getGLState().getPackState();
 
-    if (outputPitchOrErr.isError())
+    auto endByteOrErr = formatInfo.computePackEndByte(type, size, pack);
+    if (endByteOrErr.isError())
     {
-        context->handleError(outputPitchOrErr.getError());
+        context->handleError(endByteOrErr.getError());
         return false;
     }
 
-    CheckedNumeric<GLuint> checkedOutputPitch(outputPitchOrErr.getResult());
-    auto checkedRequiredSize = checkedOutputPitch * height;
-    if (!checkedRequiredSize.IsValid())
+    if (endByteOrErr.getResult() > static_cast<GLuint>(bufSize))
     {
-        context->handleError(Error(GL_INVALID_OPERATION, "Unsigned multiplication overflow."));
-        return false;
-    }
-
-    // sized query sanity check
-    if (checkedRequiredSize.ValueOrDie() > static_cast<GLuint>(bufSize))
-    {
-        context->handleError(Error(GL_INVALID_OPERATION));
+        context->handleError(Error(GL_INVALID_OPERATION, "Writes would overflow past bufSize."));
         return false;
     }
 

diff --git a/src/libANGLE/validationES3.cpp b/src/libANGLE/validationES3.cpp
@@ -477,7 +477,7 @@ bool ValidateES3TexImageParametersBase(Context *context,
 
     // Check for pixel unpack buffer related API errors
     gl::Buffer *pixelUnpackBuffer = context->getGLState().getTargetBuffer(GL_PIXEL_UNPACK_BUFFER);
-    if (pixelUnpackBuffer != NULL)
+    if (pixelUnpackBuffer != nullptr)
     {
         // ...the data would be unpacked from the buffer object such that the memory reads required
         // would exceed the data store size.
@@ -513,15 +513,16 @@ bool ValidateES3TexImageParametersBase(Context *context,
 
             if ((checkedOffset.ValueOrDie() % dataBytesPerPixel) != 0)
             {
-                context->handleError(Error(GL_INVALID_OPERATION));
+                context->handleError(
+                    Error(GL_INVALID_OPERATION, "Reads would overflow the pixel unpack buffer."));
                 return false;
             }
         }
 
         // ...the buffer object's data store is currently mapped.
         if (pixelUnpackBuffer->isMapped())
         {
-            context->handleError(Error(GL_INVALID_OPERATION));
+            context->handleError(Error(GL_INVALID_OPERATION, "Pixel unpack buffer is mapped."));
             return false;
         }
     }