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

[Brian Paul]

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