[Piglit] [PATCH 2/2] texturing/s3rc-errors: double check GL_TEXTURE parameters and alloc

[Ian Romanick]

On 02/05/2014 10:30 PM, Daniel Kurtz wrote:
> If the GL does not actually support one of the compression
> formats in s3tc_formats, it may silently choose a different actual
> internalformat.
> 
> In such a case, the resulting compressed image size may be larger,
> leading to memory corruption when the bigger-than-expected image is read back
> with glGetCompressedTexImage() into a buffer that was allocated to the
> expected, smaller, size.
> 
> Instead, first confirm the texture has really been compressed, that the
> format is as expected, and that the size matches our pre-computed expected
> size.  If any of these fail, set pass to false and spit an error, but
> keep going, using the actual format and size returned by the GL.
> 
> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
> ---
>  tests/texturing/s3tc-errors.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/texturing/s3tc-errors.c b/tests/texturing/s3tc-errors.c
> index 710625c..bfe8053 100644
> --- a/tests/texturing/s3tc-errors.c
> +++ b/tests/texturing/s3tc-errors.c
> @@ -125,14 +125,17 @@ check_gl_error2_(GLenum err1, GLenum err2, int line)
>  
>  
>  static bool
> -test_format(int width, int height, GLfloat *image, GLenum format)
> +test_format(int width, int height, GLfloat *image, GLenum requested_format)
>  {
> -	GLubyte *compressed_image =
> -		malloc(piglit_compressed_image_size(format, width, height));
> +	GLubyte *compressed_image;
>  	GLenum format2;
>  	int x, y, w, h;
>  	GLuint tex;
>  	bool pass = true;
> +	GLuint expected_size;
> +	bool is_compressed;
> +	GLuint compressed_size;
> +	GLenum format;
>  
>  	glPixelStorei(GL_UNPACK_SKIP_PIXELS, 0);
>  	glPixelStorei(GL_UNPACK_SKIP_ROWS, 0);
> @@ -143,12 +146,43 @@ test_format(int width, int height, GLfloat *image, GLenum format)
>  	glBindTexture(GL_TEXTURE_2D, tex);
>  	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
>  	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> -	glTexImage2D(GL_TEXTURE_2D, 0, format, width, height, 0,
> +	glTexImage2D(GL_TEXTURE_2D, 0, requested_format, width, height, 0,
>  		     GL_RGBA, GL_FLOAT, image);
>  
>  	pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
>  	pass = check_rendering(width, height) && pass;
>  
> +	glGetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_COMPRESSED,
> +			&is_compressed);
> +	glGetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_INTERNAL_FORMAT,
> +			&format);
> +	glGetTexLevelParameteriv(GL_TEXTURE_2D, 0,
> +			GL_TEXTURE_COMPRESSED_IMAGE_SIZE, &compressed_size);
> +
> +	pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> +
> +	if (!is_compressed)
> +		printf("Image was not compressed\n");
> +	pass = is_compressed && pass;

It's better to do each of these blocks as:

    if (!is_compressed) {
        printf("Image was not compressed\n");
        pass = false;
    }

With each of the three cases of that fixed,

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

(One other suggestion below.)

> +
> +	if (format != requested_format)
> +		printf("Internal Format mismatch. Found: 0x%04x Expected: 0x%04x\n",
> +				format, requested_format);
> +	pass = (format == requested_format) && pass;
> +
> +	expected_size = piglit_compressed_image_size(requested_format, width,
> +			height);
> +
> +	if (compressed_size != expected_size)
> +		printf("Compressed image size mismatch. Found: %u Expected: %u\n",
> +				compressed_size, expected_size);
> +	pass = (compressed_size == expected_size) && pass;
> +
> +	/* Use GL_TEXTURE_COMPRESSED_IMAGE_SIZE even if it wasn't what we
> +	 * expected to avoid corruption due to under-allocated buffer.
> +	 */
> +	compressed_image = malloc(compressed_size);
> +

If we're going to all this effort, we should probably go all the way. A
follow-on patch should check that glGetCompressedTexImage doesn't
over-run the buffer.

>  	/* Read back the compressed image data */
>  	glGetCompressedTexImage(GL_TEXTURE_2D, 0, compressed_image);
>  
>