[1/2] oes_compressed_paletted_texture: prevent accessing buffer out of bounds

Submitted by Nanley Chery on July 27, 2015, 9:13 p.m.

Details

Message ID 1438031594-15534-1-git-send-email-nanleychery@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Nanley Chery July 27, 2015, 9:13 p.m.
From: Nanley Chery <nanley.g.chery@intel.com>

The buffer variable is too small to accomodate running the
[Compressed]TexImage commands with the GL_PALETTE8_RGBA8_OES format
(which needs 1024 bytes instead of the 768 allocated). Rely on the GL
behavior of allocating unspecified memory when a NULL pointer is passed.

Signed-off-by: Nanley Chery <nanley.g.chery@intel.com>
Cc: Ian Romanick <ian.d.romanick@intel.com>
---
 .../oes_compressed_paletted_texture-api.c                     | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c b/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
index 13b0bba..4d9f5b7 100644
--- a/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
+++ b/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
@@ -63,7 +63,6 @@  piglit_display(void)
 void
 piglit_init(int argc, char **argv)
 {
-	GLubyte buffer[512 + (16 * 16)];
 	GLuint tex;
 	GLsizei size;
 	unsigned i;
@@ -98,7 +97,7 @@  piglit_init(int argc, char **argv)
 	for (i = 0; i < ARRAY_SIZE(t); i++) {
 		glTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
 			     16, 16, 0,
-			     t[i].internal_format, t[i].type, buffer);
+			     t[i].internal_format, t[i].type, NULL);
 #if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL_ES2)
 		{
 			GLenum error = glGetError();
@@ -131,13 +130,13 @@  piglit_init(int argc, char **argv)
 		 */
 		glCompressedTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
 					  16, 16, 0,
-					  size + t[i].palette_size - 1, buffer);
+					  size + t[i].palette_size - 1, NULL);
 		if (!piglit_check_gl_error(GL_INVALID_VALUE))
 			piglit_report_result(PIGLIT_FAIL);
 
 		glCompressedTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
 					  16, 16, 0,
-					  size + t[i].palette_size, buffer);
+					  size + t[i].palette_size, NULL);
 		if (!piglit_check_gl_error(GL_NO_ERROR))
 			piglit_report_result(PIGLIT_FAIL);
 
@@ -155,7 +154,7 @@  piglit_init(int argc, char **argv)
 		size = (8 * 8) >> t[i].shift;
 		glCompressedTexImage2D(GL_TEXTURE_2D, 1, t[i].internal_format,
 				       8, 8, 0,
-				       size + t[i].palette_size, buffer);
+				       size + t[i].palette_size, NULL);
 		if (!piglit_check_gl_error(GL_INVALID_VALUE))
 			piglit_report_result(PIGLIT_FAIL);
 
@@ -184,7 +183,7 @@  piglit_init(int argc, char **argv)
 		size = (17 * 17) >> t[i].shift;
 		glCompressedTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
 				       16, 16, 1,
-				       size + t[i].palette_size, buffer);
+				       size + t[i].palette_size, NULL);
 #if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL_ES2)
 		if (!piglit_check_gl_error(GL_INVALID_VALUE))
 			piglit_report_result(PIGLIT_FAIL);

Comments

On 07/27/2015 02:13 PM, Nanley Chery wrote:
> From: Nanley Chery <nanley.g.chery@intel.com>
> 
> The buffer variable is too small to accomodate running the
> [Compressed]TexImage commands with the GL_PALETTE8_RGBA8_OES format
> (which needs 1024 bytes instead of the 768 allocated). Rely on the GL
               ^^^^
You mean (32 bits * 256 entries) + (16 * 16) = 1280, right? :)

> behavior of allocating unspecified memory when a NULL pointer is passed.

I'm a little bit skeptical about this.

    *   Should glCompressedTexSubImage2D be allowed for modifying paletted
        texture data.

        RESOLVED:  No, this would then require implementations that do not
        support paletted formats internally to also store the palette
        per texture.  This can be a memory overhead on platforms that are
        memory constrained.

I have a feeling that some implementations would generate an error for
this even though the spec does not call for one.

It also shouldn't matter for most (all?) of these cases.  These are all
trying to get the implementation to generate an error based on other
parameters to the functions.  The error should be generated before
dereferencing the pointer.

I think it's better to just increase the size of the buffer to (4 *
256) + (16 * 16).

> Signed-off-by: Nanley Chery <nanley.g.chery@intel.com>
> Cc: Ian Romanick <ian.d.romanick@intel.com>
> ---
>  .../oes_compressed_paletted_texture-api.c                     | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c b/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
> index 13b0bba..4d9f5b7 100644
> --- a/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
> +++ b/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
> @@ -63,7 +63,6 @@ piglit_display(void)
>  void
>  piglit_init(int argc, char **argv)
>  {
> -	GLubyte buffer[512 + (16 * 16)];
>  	GLuint tex;
>  	GLsizei size;
>  	unsigned i;
> @@ -98,7 +97,7 @@ piglit_init(int argc, char **argv)
>  	for (i = 0; i < ARRAY_SIZE(t); i++) {
>  		glTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
>  			     16, 16, 0,
> -			     t[i].internal_format, t[i].type, buffer);
> +			     t[i].internal_format, t[i].type, NULL);
>  #if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL_ES2)
>  		{
>  			GLenum error = glGetError();
> @@ -131,13 +130,13 @@ piglit_init(int argc, char **argv)
>  		 */
>  		glCompressedTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
>  					  16, 16, 0,
> -					  size + t[i].palette_size - 1, buffer);
> +					  size + t[i].palette_size - 1, NULL);
>  		if (!piglit_check_gl_error(GL_INVALID_VALUE))
>  			piglit_report_result(PIGLIT_FAIL);
>  
>  		glCompressedTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
>  					  16, 16, 0,
> -					  size + t[i].palette_size, buffer);
> +					  size + t[i].palette_size, NULL);
>  		if (!piglit_check_gl_error(GL_NO_ERROR))
>  			piglit_report_result(PIGLIT_FAIL);
>  
> @@ -155,7 +154,7 @@ piglit_init(int argc, char **argv)
>  		size = (8 * 8) >> t[i].shift;
>  		glCompressedTexImage2D(GL_TEXTURE_2D, 1, t[i].internal_format,
>  				       8, 8, 0,
> -				       size + t[i].palette_size, buffer);
> +				       size + t[i].palette_size, NULL);
>  		if (!piglit_check_gl_error(GL_INVALID_VALUE))
>  			piglit_report_result(PIGLIT_FAIL);
>  
> @@ -184,7 +183,7 @@ piglit_init(int argc, char **argv)
>  		size = (17 * 17) >> t[i].shift;
>  		glCompressedTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
>  				       16, 16, 1,
> -				       size + t[i].palette_size, buffer);
> +				       size + t[i].palette_size, NULL);
>  #if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL_ES2)
>  		if (!piglit_check_gl_error(GL_INVALID_VALUE))
>  			piglit_report_result(PIGLIT_FAIL);
>
On Tue, Jul 28, 2015 at 1:23 PM, Ian Romanick <idr@freedesktop.org> wrote:
> On 07/27/2015 02:13 PM, Nanley Chery wrote:
>> From: Nanley Chery <nanley.g.chery@intel.com>
>>
>> The buffer variable is too small to accomodate running the
>> [Compressed]TexImage commands with the GL_PALETTE8_RGBA8_OES format
>> (which needs 1024 bytes instead of the 768 allocated). Rely on the GL
>                ^^^^
> You mean (32 bits * 256 entries) + (16 * 16) = 1280, right? :)
>

Whoops. Yes, I did.

>> behavior of allocating unspecified memory when a NULL pointer is passed.
>
> I'm a little bit skeptical about this.
>
>     *   Should glCompressedTexSubImage2D be allowed for modifying paletted
>         texture data.
>
>         RESOLVED:  No, this would then require implementations that do not
>         support paletted formats internally to also store the palette
>         per texture.  This can be a memory overhead on platforms that are
>         memory constrained.
>
> I have a feeling that some implementations would generate an error for
> this even though the spec does not call for one.
>

OpenGL 4.5 Core:


If the data argument of TexImage1D, TexImage2D, or TexImage3D is NULL ,
and the pixel unpack buffer object is zero, a one-, two-, or
three-dimensional tex-
ture image is created with the specified target, level, internalformat,
border, width,
height, and depth, but with unspecified image contents. In this case no
pixel values
are accessed in client memory, and no pixel processing is performed.

[...]

If the data argument of CompressedTexImage1D, CompressedTexImage2D,
or CompressedTexImage3D is NULL , and the pixel unpack buffer object is
zero,
a texture image with unspecified image contents is created, just as when a
NULL
pointer is passed to TexImage1D, TexImage2D, or TexImage3D.


GLES 1.0 says about the same thing about TexImage but not about
CompressedTexImage. Perhaps this could be why some implementations would
emit errors?

> It also shouldn't matter for most (all?) of these cases.  These are all
> trying to get the implementation to generate an error based on other
> parameters to the functions.  The error should be generated before
> dereferencing the pointer.
>

I agree.

> I think it's better to just increase the size of the buffer to (4 *
> 256) + (16 * 16).
>
>> Signed-off-by: Nanley Chery <nanley.g.chery@intel.com>
>> Cc: Ian Romanick <ian.d.romanick@intel.com>
>> ---
>>  .../oes_compressed_paletted_texture-api.c                     | 11
+++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git
a/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
b/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
>> index 13b0bba..4d9f5b7 100644
>> ---
a/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
>> +++
b/tests/spec/oes_compressed_paletted_texture/oes_compressed_paletted_texture-api.c
>> @@ -63,7 +63,6 @@ piglit_display(void)
>>  void
>>  piglit_init(int argc, char **argv)
>>  {
>> -     GLubyte buffer[512 + (16 * 16)];
>>       GLuint tex;
>>       GLsizei size;
>>       unsigned i;
>> @@ -98,7 +97,7 @@ piglit_init(int argc, char **argv)
>>       for (i = 0; i < ARRAY_SIZE(t); i++) {
>>               glTexImage2D(GL_TEXTURE_2D, 0, t[i].internal_format,
>>                            16, 16, 0,
>> -                          t[i].internal_format, t[i].type, buffer);
>> +                          t[i].internal_format, t[i].type, NULL);
>>  #if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL_ES2)
>>               {
>>                       GLenum error = glGetError();
>> @@ -131,13 +130,13 @@ piglit_init(int argc, char **argv)
>>                */
>>               glCompressedTexImage2D(GL_TEXTURE_2D, 0,
t[i].internal_format,
>>                                         16, 16, 0,
>> -                                       size + t[i].palette_size - 1,
buffer);
>> +                                       size + t[i].palette_size - 1,
NULL);
>>               if (!piglit_check_gl_error(GL_INVALID_VALUE))
>>                       piglit_report_result(PIGLIT_FAIL);
>>
>>               glCompressedTexImage2D(GL_TEXTURE_2D, 0,
t[i].internal_format,
>>                                         16, 16, 0,
>> -                                       size + t[i].palette_size,
buffer);
>> +                                       size + t[i].palette_size, NULL);
>>               if (!piglit_check_gl_error(GL_NO_ERROR))
>>                       piglit_report_result(PIGLIT_FAIL);
>>
>> @@ -155,7 +154,7 @@ piglit_init(int argc, char **argv)
>>               size = (8 * 8) >> t[i].shift;
>>               glCompressedTexImage2D(GL_TEXTURE_2D, 1,
t[i].internal_format,
>>                                      8, 8, 0,
>> -                                    size + t[i].palette_size, buffer);
>> +                                    size + t[i].palette_size, NULL);
>>               if (!piglit_check_gl_error(GL_INVALID_VALUE))
>>                       piglit_report_result(PIGLIT_FAIL);
>>
>> @@ -184,7 +183,7 @@ piglit_init(int argc, char **argv)
>>               size = (17 * 17) >> t[i].shift;
>>               glCompressedTexImage2D(GL_TEXTURE_2D, 0,
t[i].internal_format,
>>                                      16, 16, 1,
>> -                                    size + t[i].palette_size, buffer);
>> +                                    size + t[i].palette_size, NULL);
>>  #if defined(PIGLIT_USE_OPENGL_ES1) || defined(PIGLIT_USE_OPENGL_ES2)
>>               if (!piglit_check_gl_error(GL_INVALID_VALUE))
>>                       piglit_report_result(PIGLIT_FAIL);
>>
>