diff --git a/src/libANGLE/formatutils.cpp b/src/libANGLE/formatutils.cpp index 780f2f7ef..4222d726c 100644 --- a/src/libANGLE/formatutils.cpp +++ b/src/libANGLE/formatutils.cpp @@ -939,6 +939,32 @@ gl::ErrorOrResult InternalFormat::computeSkipBytes(GLuint rowPitch, return skipBytes.ValueOrDie(); } +gl::ErrorOrResult InternalFormat::computePackSize(GLenum formatType, + const gl::Extents &size, + const gl::PixelPackState &pack) const +{ + ASSERT(!compressed); + + if (size.height == 0) + { + return 0; + } + + CheckedNumeric rowPitch; + ANGLE_TRY_RESULT(computeRowPitch(formatType, size.width, pack.alignment, pack.rowLength), + rowPitch); + + CheckedNumeric heightMinusOne = size.height - 1; + CheckedNumeric bytes = computePixelBytes(formatType); + + CheckedNumeric totalSize = heightMinusOne * rowPitch; + totalSize += size.width * bytes; + + ANGLE_TRY_CHECKED_MATH(totalSize); + + return totalSize.ValueOrDie(); +} + gl::ErrorOrResult InternalFormat::computeUnpackSize( GLenum formatType, const gl::Extents &size, @@ -950,6 +976,11 @@ gl::ErrorOrResult InternalFormat::computeUnpackSize( return computeCompressedImageSize(formatType, size); } + if (size.height == 0 || size.depth == 0) + { + return 0; + } + CheckedNumeric rowPitch; CheckedNumeric depthPitch; ANGLE_TRY_RESULT(computeRowPitch(formatType, size.width, unpack.alignment, unpack.rowLength), @@ -971,6 +1002,26 @@ gl::ErrorOrResult InternalFormat::computeUnpackSize( return totalSize.ValueOrDie(); } +gl::ErrorOrResult InternalFormat::computePackEndByte(GLenum formatType, + const gl::Extents &size, + const gl::PixelPackState &pack) const +{ + GLuint rowPitch; + CheckedNumeric checkedSkipBytes; + CheckedNumeric 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 endByte = checkedCopyBytes + checkedSkipBytes; + + ANGLE_TRY_CHECKED_MATH(endByte); + return endByte.ValueOrDie(); +} + gl::ErrorOrResult 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 b5dc6a42c..0bd3ae61d 100644 --- 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 computePackSize(GLenum formatType, + const gl::Extents &size, + const gl::PixelPackState &pack) const; gl::ErrorOrResult computeUnpackSize(GLenum formatType, const gl::Extents &size, const gl::PixelUnpackState &unpack) const; + gl::ErrorOrResult computePackEndByte(GLenum formatType, + const gl::Extents &size, + const gl::PixelPackState &pack) const; gl::ErrorOrResult computeUnpackEndByte(GLenum formatType, const gl::Extents &size, const gl::PixelUnpackState &unpack, diff --git a/src/libANGLE/validationES.cpp b/src/libANGLE/validationES.cpp index 3ab4f6bff..2d3474a9d 100644 --- 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 checkedEndByte(endByteOrErr.getResult()); + CheckedNumeric checkedOffset(reinterpret_cast(pixels)); + checkedEndByte += checkedOffset; + + if (checkedEndByte.ValueOrDie() > static_cast(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 checkedOutputPitch(outputPitchOrErr.getResult()); - auto checkedRequiredSize = checkedOutputPitch * height; - if (!checkedRequiredSize.IsValid()) + if (endByteOrErr.getResult() > static_cast(bufSize)) { - context->handleError(Error(GL_INVALID_OPERATION, "Unsigned multiplication overflow.")); - return false; - } - - // sized query sanity check - if (checkedRequiredSize.ValueOrDie() > static_cast(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 c89156363..782646f7d 100644 --- 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,7 +513,8 @@ 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; } } @@ -521,7 +522,7 @@ bool ValidateES3TexImageParametersBase(Context *context, // ...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 2a9fe8e5a..47d2e3dba 100644 --- a/src/tests/gl_tests/ReadPixelsTest.cpp +++ b/src/tests/gl_tests/ReadPixelsTest.cpp @@ -32,7 +32,7 @@ class ReadPixelsTest : public ANGLETest } }; -// 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 @@ TEST_P(ReadPixelsTest, OutOfBounds) GLsizei pixelsWidth = 32; GLsizei pixelsHeight = 32; GLint offset = 16; - std::vector pixels((pixelsWidth + offset) * (pixelsHeight + offset) * 4); + std::vector 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(pixel[0]); - unsigned int g = static_cast(pixel[1]); - unsigned int b = static_cast(pixel[2]); - unsigned int a = static_cast(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 @@ class ReadPixelsPBOTest : public ReadPixelsTest 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 @@ class ReadPixelsPBOTest : public ReadPixelsTest 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(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 @@ TEST_P(ReadPixelsPBOTest, ArrayBufferTarget) glBindBuffer(GL_ARRAY_BUFFER, mPBO); GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT); - unsigned char *dataPtr = static_cast(mappedPtr); + GLColor *dataColor = static_cast(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 @@ TEST_P(ReadPixelsPBOTest, ExistingDataPreserved) // Read 16x16 region from green backbuffer to PBO at offset 16 glReadPixels(0, 0, 16, 16, GL_RGBA, GL_UNSIGNED_BYTE, reinterpret_cast(16)); - GLvoid * mappedPtr = glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, 32, GL_MAP_READ_BIT); - unsigned char *dataPtr = static_cast(mappedPtr); + GLvoid *mappedPtr = glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, 32, GL_MAP_READ_BIT); + GLColor *dataColor = static_cast(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 @@ TEST_P(ReadPixelsPBOTest, SubDataPreservesContents) 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(mappedPtr); + GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT); + GLColor *dataColor = static_cast(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 @@ TEST_P(ReadPixelsPBOTest, SubDataOffsetPreservesContents) 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(mappedPtr); + GLvoid *mappedPtr = glMapBufferRange(GL_ARRAY_BUFFER, 0, 32, GL_MAP_READ_BIT); + GLColor *dataColor = static_cast(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 @@ class ReadPixelsPBODrawTest : public ReadPixelsPBOTest // 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 @@ TEST_P(ReadPixelsPBODrawTest, DrawWithPBO) 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