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

[Matt Turner]

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. The
only complication is that you have to check that the pnames are valid
too.

That diff actually looks okay. I'll send it out.