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

[Nanley Chery]

On Tue, Jul 28, 2015 at 1:23 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 07/27/2015 02:13 PM, Nanley Chery wrote:
>> From: Nanley Chery <nanley.g.chery at 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 at intel.com>
>> Cc: Ian Romanick <ian.d.romanick at 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);
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150728/37edf2d3/attachment-0001.html>