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

[Frank Henigman]

New piglit test:
http://lists.freedesktop.org/archives/piglit/2012-November/003690.html

On Mon, Nov 5, 2012 at 12:04 PM, Frank Henigman <fjhenigman at google.com>wrote:

> 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/826bd085/attachment.html>