[Mesa-dev] [PATCH 1/5] mesa: Have validate_uniform_parameters return the gl_uniform_storage pointer

[Erik Faye-Lund]

On Thu, Sep 11, 2014 at 1:27 PM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
> On Sat, Aug 2, 2014 at 4:09 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
>> index 609d94b..7b089fa 100644
>> --- a/src/mesa/main/uniform_query.cpp
>> +++ b/src/mesa/main/uniform_query.cpp
>> @@ -266,30 +265,32 @@ validate_uniform_parameters(struct gl_context *ctx,
>>      */
>>     if (shProg->UniformRemapTable[location] ==
>>         INACTIVE_UNIFORM_EXPLICIT_LOCATION)
>> -      return false;
>> +      return NULL;
>>
>> -   _mesa_uniform_split_location_offset(shProg, location, loc, array_index);
>> +   unsigned loc;
>> +   _mesa_uniform_split_location_offset(shProg, location, &loc, array_index);
>> +   struct gl_uniform_storage *const uni = &shProg->UniformStorage[loc];
>>
>> -   if (shProg->UniformStorage[*loc].array_elements == 0 && count > 1) {
>> +   if (uni->array_elements == 0 && count > 1) {
>
> I'm getting a NULL-pointer deference here when running piglit's
> spec/ARB_explicit_uniform_location/arb_explicit_uniform_location-use-of-unused-loc
> on yesterday's master. A quick git-log doesn't seem to contain a fix
> in today's master either.
>
> Perhaps something like this is in order?
>
> -   if (uni->array_elements == 0 && count > 1) {
> +   if (uni && uni->array_elements == 0 && count > 1) {

Actually, that just moves the issue a few lines, as line 282 also
dereferences 'uni'. Earlying out by checking for NULL in uni seems
more appropriate, but if I do so without setting GL_INVALID_OEPRATION,
the piglit test fails.

Doing

---8<---
diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
index 4cd2bca..4483948 100644
--- a/src/mesa/main/uniform_query.cpp
+++ b/src/mesa/main/uniform_query.cpp
@@ -268,6 +268,12 @@ validate_uniform_parameters(struct gl_context *ctx,
       return NULL;

    struct gl_uniform_storage *const uni = shProg->UniformRemapTable[location];
+   if (!uni) {
+       _mesa_error(ctx, GL_INVALID_OPERATION,
+                 "%s(nonexistent uniform, location=%d)",
+                 caller, location);
+      return NULL;
+   }

    if (uni->array_elements == 0 && count > 1) {
       _mesa_error(ctx, GL_INVALID_OPERATION,
---8<---

...seems to fix both the crash and the test. Looking a bit more
closely at the test, it seems somewhat appropriate.