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 <cwallez@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Reviewed-by: Jamie Madill <jmadill@chromium.org>
diff --git a/src/libANGLE/formatutils.cpp b/src/libANGLE/formatutils.cpp
index 780f2f7..4222d72 100644
--- a/src/libANGLE/formatutils.cpp
+++ b/src/libANGLE/formatutils.cpp
@@ -939,6 +939,32 @@
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 @@
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 @@
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
index b5dc6a4..0bd3ae6 100644
--- a/src/libANGLE/formatutils.h
+++ b/src/libANGLE/formatutils.h
@@ -67,10 +67,17 @@
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
index 3ab4f6b..2d3474a 100644
--- a/src/libANGLE/validationES.cpp
+++ b/src/libANGLE/validationES.cpp
@@ -1168,6 +1168,44 @@
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 @@
}
GLenum sizedInternalFormat = GetSizedInternalFormat(format, type);
- const InternalFormat &sizedFormatInfo = GetInternalFormatInfo(sizedInternalFormat);
+ const InternalFormat &formatInfo = GetInternalFormatInfo(sizedInternalFormat);
+ const gl::Extents size(width, height, 1);
+ const auto &pack = context->getGLState().getPackState();
- auto outputPitchOrErr =
- sizedFormatInfo.computeRowPitch(type, width, context->getGLState().getPackAlignment(),
- context->getGLState().getPackRowLength());
-
- 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
index c891563..782646f 100644
--- a/src/libANGLE/validationES3.cpp
+++ b/src/libANGLE/validationES3.cpp
@@ -477,7 +477,7 @@
// 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,7 +513,8 @@
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;
}
}
@@ -521,7 +522,7 @@
// ...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;
}
}
diff --git a/src/tests/gl_tests/ReadPixelsTest.cpp b/src/tests/gl_tests/ReadPixelsTest.cpp
index 2a9fe8e..47d2e3d 100644
--- a/src/tests/gl_tests/ReadPixelsTest.cpp
+++ b/src/tests/gl_tests/ReadPixelsTest.cpp
@@ -32,7 +32,7 @@
}
};
-// Test out of bounds reads.
+// Test out of bounds framebuffer reads.
TEST_P(ReadPixelsTest, OutOfBounds)
{
// TODO: re-enable once root cause of http://anglebug.com/1413 is fixed
@@ -49,26 +49,17 @@
GLsizei pixelsWidth = 32;
GLsizei pixelsHeight = 32;
GLint offset = 16;
- std::vector<GLubyte> pixels((pixelsWidth + offset) * (pixelsHeight + offset) * 4);
+ std::vector<GLColor> pixels((pixelsWidth + offset) * (pixelsHeight + offset));
glReadPixels(-offset, -offset, pixelsWidth + offset, pixelsHeight + offset, GL_RGBA, GL_UNSIGNED_BYTE, &pixels[0]);
EXPECT_GL_NO_ERROR();
+ // Expect that all pixels which fell within the framebuffer are red
for (int y = pixelsHeight / 2; y < pixelsHeight; y++)
{
for (int x = pixelsWidth / 2; x < pixelsWidth; x++)
{
- const GLubyte* pixel = &pixels[0] + ((y * (pixelsWidth + offset) + x) * 4);
- unsigned int r = static_cast<unsigned int>(pixel[0]);
- unsigned int g = static_cast<unsigned int>(pixel[1]);
- unsigned int b = static_cast<unsigned int>(pixel[2]);
- unsigned int a = static_cast<unsigned int>(pixel[3]);
-
- // Expect that all pixels which fell within the framebuffer are red
- EXPECT_EQ(255u, r);
- EXPECT_EQ(0u, g);
- EXPECT_EQ(0u, b);
- EXPECT_EQ(255u, a);
+ EXPECT_EQ(GLColor::red, pixels[y * (pixelsWidth + offset) + x]);
}
}
}
@@ -83,16 +74,22 @@
ANGLETest::SetUp();
glGenBuffers(1, &mPBO);
+ glGenFramebuffers(1, &mFBO);
+
+ Reset(4 * getWindowWidth() * getWindowHeight(), 4, 1);
+ }
+
+ void Reset(GLuint bufferSize, GLuint fboWidth, GLuint fboHeight)
+ {
glBindBuffer(GL_PIXEL_PACK_BUFFER, mPBO);
- glBufferData(GL_PIXEL_PACK_BUFFER, 4 * getWindowWidth() * getWindowHeight(), nullptr,
- GL_STATIC_DRAW);
+ glBufferData(GL_PIXEL_PACK_BUFFER, bufferSize, nullptr, GL_STATIC_DRAW);
glBindBuffer(GL_PIXEL_PACK_BUFFER, 0);
+ glDeleteTextures(1, &mTexture);
glGenTextures(1, &mTexture);
glBindTexture(GL_TEXTURE_2D, mTexture);
- glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGBA8, 4, 1);
+ glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGBA8, fboWidth, fboHeight);
- glGenFramebuffers(1, &mFBO);
glBindFramebuffer(GL_FRAMEBUFFER, mFBO);
glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, mTexture, 0);
glBindFramebuffer(GL_FRAMEBUFFER, 0);
@@ -109,11 +106,60 @@
ANGLETest::TearDown();
}
- GLuint mPBO;
- GLuint mTexture;
- GLuint mFBO;
+ GLuint mPBO = 0;
+ GLuint mTexture = 0;
+ GLuint mFBO = 0;
};
+// Test basic usage of PBOs.
+TEST_P(ReadPixelsPBOTest, Basic)
+{
+ glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
+ glClear(GL_COLOR_BUFFER_BIT);
+ EXPECT_GL_NO_ERROR();
+
+ glBindBuffer(GL_PIXEL_PACK_BUFFER, mPBO);
+ glReadPixels(0, 0, 16, 16, GL_RGBA, GL_UNSIGNED_BYTE, 0);
+
+ GLvoid *mappedPtr = glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, 32, GL_MAP_READ_BIT);
+ GLColor *dataColor = static_cast<GLColor *>(mappedPtr);
+ EXPECT_GL_NO_ERROR();
+
+ EXPECT_EQ(GLColor::red, dataColor[0]);
+
+ glUnmapBuffer(GL_PIXEL_PACK_BUFFER);
+ EXPECT_GL_NO_ERROR();
+}
+
+// Test an error is generated when the PBO is too small.
+TEST_P(ReadPixelsPBOTest, PBOTooSmall)
+{
+ Reset(4 * 16 * 16 - 1, 16, 16);
+
+ glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
+ glClear(GL_COLOR_BUFFER_BIT);
+ EXPECT_GL_NO_ERROR();
+
+ glBindBuffer(GL_PIXEL_PACK_BUFFER, mPBO);
+ glReadPixels(0, 0, 16, 16, GL_RGBA, GL_UNSIGNED_BYTE, 0);
+
+ EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+}
+
+// Test an error is generated when the PBO is mapped.
+TEST_P(ReadPixelsPBOTest, PBOMapped)
+{
+ glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
+ glClear(GL_COLOR_BUFFER_BIT);
+ EXPECT_GL_NO_ERROR();
+
+ glBindBuffer(GL_PIXEL_PACK_BUFFER, mPBO);
+ glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, 32, GL_MAP_READ_BIT);
+ glReadPixels(0, 0, 16, 16, GL_RGBA, GL_UNSIGNED_BYTE, 0);
+
+ EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+}
+
// Test that binding a PBO to ARRAY_BUFFER works as expected.
TEST_P(ReadPixelsPBOTest, ArrayBufferTarget)
{
@@ -128,13 +174,10 @@
glBindBuffer(GL_ARRAY_BUFFER, mPBO);
GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT);
- unsigned char *dataPtr = static_cast<unsigned char *>(mappedPtr);
+ GLColor *dataColor = static_cast<GLColor *>(mappedPtr);
EXPECT_GL_NO_ERROR();
- EXPECT_EQ(255, dataPtr[0]);
- EXPECT_EQ(0, dataPtr[1]);
- EXPECT_EQ(0, dataPtr[2]);
- EXPECT_EQ(255, dataPtr[3]);
+ EXPECT_EQ(GLColor::red, dataColor[0]);
glUnmapBuffer(GL_ARRAY_BUFFER);
EXPECT_GL_NO_ERROR();
@@ -166,21 +209,15 @@
// Read 16x16 region from green backbuffer to PBO at offset 16
glReadPixels(0, 0, 16, 16, GL_RGBA, GL_UNSIGNED_BYTE, reinterpret_cast<GLvoid*>(16));
- GLvoid * mappedPtr = glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, 32, GL_MAP_READ_BIT);
- unsigned char *dataPtr = static_cast<unsigned char *>(mappedPtr);
+ GLvoid *mappedPtr = glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, 32, GL_MAP_READ_BIT);
+ GLColor *dataColor = static_cast<GLColor *>(mappedPtr);
EXPECT_GL_NO_ERROR();
// Test pixel 0 is red (existing data)
- EXPECT_EQ(255, dataPtr[0]);
- EXPECT_EQ(0, dataPtr[1]);
- EXPECT_EQ(0, dataPtr[2]);
- EXPECT_EQ(255, dataPtr[3]);
+ EXPECT_EQ(GLColor::red, dataColor[0]);
// Test pixel 16 is green (new data)
- EXPECT_EQ(0, dataPtr[16 * 4 + 0]);
- EXPECT_EQ(255, dataPtr[16 * 4 + 1]);
- EXPECT_EQ(0, dataPtr[16 * 4 + 2]);
- EXPECT_EQ(255, dataPtr[16 * 4 + 3]);
+ EXPECT_EQ(GLColor::green, dataColor[16]);
glUnmapBuffer(GL_PIXEL_PACK_BUFFER);
EXPECT_GL_NO_ERROR();
@@ -202,14 +239,11 @@
glBindBuffer(GL_ARRAY_BUFFER, mPBO);
glBufferSubData(GL_ARRAY_BUFFER, 0, 4, data);
- GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT);
- unsigned char *dataPtr = static_cast<unsigned char *>(mappedPtr);
+ GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT);
+ GLColor *dataColor = static_cast<GLColor *>(mappedPtr);
EXPECT_GL_NO_ERROR();
- EXPECT_EQ(1, dataPtr[0]);
- EXPECT_EQ(2, dataPtr[1]);
- EXPECT_EQ(3, dataPtr[2]);
- EXPECT_EQ(4, dataPtr[3]);
+ EXPECT_EQ(GLColor(1, 2, 3, 4), dataColor[0]);
glUnmapBuffer(GL_ARRAY_BUFFER);
EXPECT_GL_NO_ERROR();
@@ -238,19 +272,12 @@
glBindBuffer(GL_ARRAY_BUFFER, mPBO);
glBufferSubData(GL_ARRAY_BUFFER, 16, 4, data);
- GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT);
- unsigned char *dataPtr = static_cast<unsigned char *>(mappedPtr);
+ GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT);
+ GLColor *dataColor = static_cast<GLColor *>(mappedPtr);
EXPECT_GL_NO_ERROR();
- EXPECT_EQ(255, dataPtr[0]);
- EXPECT_EQ(0, dataPtr[1]);
- EXPECT_EQ(0, dataPtr[2]);
- EXPECT_EQ(255, dataPtr[3]);
-
- EXPECT_EQ(1, dataPtr[16]);
- EXPECT_EQ(2, dataPtr[17]);
- EXPECT_EQ(3, dataPtr[18]);
- EXPECT_EQ(4, dataPtr[19]);
+ EXPECT_EQ(GLColor::red, dataColor[0]);
+ EXPECT_EQ(GLColor(1, 2, 3, 4), dataColor[4]);
glUnmapBuffer(GL_ARRAY_BUFFER);
EXPECT_GL_NO_ERROR();
@@ -304,10 +331,9 @@
// Test that we can draw with PBO data.
TEST_P(ReadPixelsPBODrawTest, DrawWithPBO)
{
- unsigned char data[4] = { 1, 2, 3, 4 };
-
+ GLColor color(1, 2, 3, 4);
glBindTexture(GL_TEXTURE_2D, mTexture);
- glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, data);
+ glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, &color);
EXPECT_GL_NO_ERROR();
glBindFramebuffer(GL_READ_FRAMEBUFFER, mFBO);
@@ -344,14 +370,11 @@
glDrawArrays(GL_POINTS, 0, 1);
EXPECT_GL_NO_ERROR();
- memset(data, 0, 4);
- glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, data);
+ color = GLColor(0, 0, 0, 0);
+ glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, &color);
EXPECT_GL_NO_ERROR();
- EXPECT_EQ(1, data[0]);
- EXPECT_EQ(2, data[1]);
- EXPECT_EQ(3, data[2]);
- EXPECT_EQ(4, data[3]);
+ EXPECT_EQ(GLColor(1, 2, 3, 4), color);
}
class ReadPixelsMultisampleTest : public ReadPixelsTest