[Mesa-dev] [PATCH] mesa: add bounds checking for uniform array access

[Frank Henigman]

Should have mentioned it does pass piglit, but now that I look I don't see
any tests generating an out-of-bounds array index.  I also made my own test
program which does go out of bounds, and I'll submit that for inclusion in
piglit.

On Fri, Nov 2, 2012 at 4:51 PM, Ian Romanick <idr at freedesktop.org> wrote:

> On 11/02/2012 01:12 PM, Frank Henigman wrote:
>
>> validate_uniform_parameters now checks that the array index is
>> valid.  This means if an index is out of bounds, glGetUniform* now
>> fails with GL_INVALID_OPERATION, as it should.
>> _mesa_uniform and _mesa_uniform_matrix also call
>> validate_uniform_parameters so the bounds checks there became
>> redundant and were removed.
>>
>> The test in glGetUniformLocation is modified to check array bounds
>> so it now returns GL_INVALID_INDEX (-1) if you ask for the location
>> of a non-existent array element, as it should.
>>
>
> Do we have piglit tests for either of these cases?
>
> Do all of the existing piglit tests that pass still pass with this change?
>  It seems like they should...
>
>
>  ---
>>   src/mesa/main/uniform_query.**cpp |   26 +++++++++++++-------------
>>   1 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/mesa/main/uniform_query.**cpp
>> b/src/mesa/main/uniform_query.**cpp
>> index bddb8f9..66a2399 100644
>> --- a/src/mesa/main/uniform_query.**cpp
>> +++ b/src/mesa/main/uniform_query.**cpp
>> @@ -237,11 +237,14 @@ validate_uniform_parameters(**struct gl_context
>> *ctx,
>>         return false;
>>      }
>>
>> -   /* This case should be impossible.  The implication is that a call
>> like
>> -    * glGetUniformLocation(prog, "foo[8]") was successful but "foo" is
>> not an
>> -    * array.
>> +   /* If the uniform is an array, check that array_index is in bounds.
>> +    * If not an array, check that array_index is zero.
>> +    * array_index is unsigned so no need to check for less than zero.
>>       */
>> -   if (*array_index != 0 && shProg->UniformStorage[*loc].**array_elements
>> == 0) {
>> +   unsigned limit = shProg->UniformStorage[*loc].**array_elements;
>> +   if (limit == 0)
>> +      limit = 1;
>> +   if (*array_index >= limit) {
>>         _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)",
>>                   caller, location);
>>         return false;
>> @@ -728,9 +731,6 @@ _mesa_uniform(struct gl_context *ctx, struct
>> gl_shader_program *shProg,
>>       * will have already generated an error.
>>       */
>>      if (uni->array_elements != 0) {
>> -      if (offset >= uni->array_elements)
>> -        return;
>> -
>>         count = MIN2(count, (int) (uni->array_elements - offset));
>>      }
>>
>> @@ -885,9 +885,6 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct
>> gl_shader_program *shProg,
>>       * will have already generated an error.
>>       */
>>      if (uni->array_elements != 0) {
>> -      if (offset >= uni->array_elements)
>> -        return;
>> -
>>         count = MIN2(count, (int) (uni->array_elements - offset));
>>      }
>>
>> @@ -1021,10 +1018,13 @@ _mesa_get_uniform_location(**struct gl_context
>> *ctx,
>>      if (!found)
>>         return GL_INVALID_INDEX;
>>
>> -   /* Since array_elements is 0 for non-arrays, this causes look-ups of
>> 'a[0]'
>> -    * to (correctly) fail if 'a' is not an array.
>> +   /* If the uniform is an array, fail if the index is out of bounds.
>> +    * (A negative index is caught above.)  This also fails if the uniform
>> +    * is not an array, but the user is trying to index it, because
>> +    * array_elements is zero and offset >= 0.
>>       */
>> -   if (array_lookup && shProg->UniformStorage[**location].array_elements
>> == 0) {
>> +   if (array_lookup
>> +        && offset >= shProg->UniformStorage[**location].array_elements)
>> {
>>         return GL_INVALID_INDEX;
>>      }
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20121105/7e30dca5/attachment.html>