[Mesa-dev] [PATCH] mesa/uniform_query: Don't write to *params if there is an error

[Brian Paul]

On 12/07/2012 04:59 PM, Matt Turner wrote:
> On Fri, Dec 7, 2012 at 3:49 PM, Brian Paul<brianp at vmware.com>  wrote:
>> On 12/07/2012 04:23 PM, Matt Turner wrote:
>>>
>>> The GL 3.1 and ES 3.0 specs say of glGetActiveUniformsiv:
>>>      "If an error occurs, nothing will be written to params."
>>>
>>> So save a copy of params and restore it on error. The alternative is
>>> making a pass through the array to check for errors before a second pass
>>> that actually writes *params, but that seems like optimizing bad
>>> programs at the expense of good ones. Actually, this patch sort of does
>>> too.
>>>
>>> Fixes es3conform's getactiveuniformsiv_for_nonexistent_uniform_indices
>>> test.
>>> ---
>>>    src/mesa/main/uniform_query.cpp |   13 +++++++++++--
>>>    1 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> Are there security implications of malloc'ing based on uniformCount?
>>>
>>> diff --git a/src/mesa/main/uniform_query.cpp
>>> b/src/mesa/main/uniform_query.cpp
>>> index cbdd39e..57ddf68 100644
>>> --- a/src/mesa/main/uniform_query.cpp
>>> +++ b/src/mesa/main/uniform_query.cpp
>>> @@ -84,6 +84,7 @@ _mesa_GetActiveUniformsiv(GLuint program,
>>>       GET_CURRENT_CONTEXT(ctx);
>>>       struct gl_shader_program *shProg;
>>>       GLsizei i;
>>> +   GLint *saved_params;
>>>
>>>       shProg = _mesa_lookup_shader_program_err(ctx, program,
>>> "glGetActiveUniform");
>>>       if (!shProg)
>>> @@ -95,13 +96,16 @@ _mesa_GetActiveUniformsiv(GLuint program,
>>>          return;
>>>       }
>>>
>>> +   saved_params = (GLint *) malloc(uniformCount * sizeof(GLint));
>>> +   memcpy(saved_params, params, uniformCount * sizeof(GLint));
>>> +
>>>       for (i = 0; i<   uniformCount; i++) {
>>>          GLuint index = uniformIndices[i];
>>>          const struct gl_uniform_storage *uni
>>> =&shProg->UniformStorage[index];
>>>
>>>
>>>          if (index>= shProg->NumUserUniformStorage) {
>>>           _mesa_error(ctx, GL_INVALID_VALUE,
>>> "glGetActiveUniformsiv(index)");
>>> -        return;
>>> +        goto error;
>>>          }
>>>
>>>          switch (pname) {
>>> @@ -142,9 +146,14 @@ _mesa_GetActiveUniformsiv(GLuint program,
>>>
>>>          default:
>>>           _mesa_error(ctx, GL_INVALID_ENUM,
>>> "glGetActiveUniformsiv(pname)");
>>> -        return;
>>> +        goto error;
>>>          }
>>>       }
>>> +   free(saved_params);
>>> +   return;
>>> +error:
>>> +   memcpy(params, saved_params, uniformCount * sizeof(GLint));
>>> +   free(saved_params);
>>>    }
>>>
>>>    static bool
>>
>>
>>
>> Another approach would be to move the index checking up into a new loop
>> before the existing loop:
>>
>> at 97:
>>
>>
>>     for (i = 0; i<  uniformCount; i++) {
>>        if (uniformIndices[i]>= shProg->NumUserUniformStorage) {
>>           _mesa_error(ctx, GL_INVALID_VALUE, "glGetActiveUniformsiv(index)");
>>           return;
>>        }
>>     }
>>
>> That seems cleaner to me.  What do you think?
>>
>> -Brian
>
> Yeah, that's the alternative I mentioned in the commit summary.

Oop, sorry, I didn't read that closely enough.


> The
> only complication is that you have to check that the pnames are valid
> too.

Are you sure?  On the first loop iteration we'll detect if pname is 
invalid and return before anything is written to params.

-Brian