[Mesa-dev] [PATCH] mesa: don't crash in KHR_no_error uniform variants when location == -1

[Samuel Pitoiset]

On 05/14/2017 01:20 PM, Timothy Arceri wrote:
> On 14/05/17 01:39, Samuel Pitoiset wrote:
>> Mmh, this can still crash if location is < -1 or greater than the 
>> number of uniforms. How about:
> 
> Sorry my commit message went missing it should have something like:
> 
> "-1 is allowed by the spec so that inactive uniforms don't cause the app 
> to throw an error."
> 
> In other words we need to not crash for that when running with no error 
> enabled, but any of the paths that would normally hit an error we are 
> free to crash on and need not waste any cpu checking for.

Okay, if the spec says this, then it's fine by me. Can you also add a 
spec comment directly in the code to avoid confusion with the errors path?

With that addressed, this patch is:

Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

> 
>>
>> struct gl_uniform_storage *uni = NULL;
>> if (_mesa_is_no_error_enabled(ctx)) {
>>    if (location >= 0 && location < shProg->NumUniformRemapTable)
>>      uni = shProg->UniformRemapTable[location];
>>    if (!uni)
>>      return;
>>    ...
>> } else {
>>    ...
>> }
>>
>> On 05/13/2017 08:31 AM, Timothy Arceri wrote:
>>> ---
>>>   src/mesa/main/uniform_query.cpp | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/src/mesa/main/uniform_query.cpp 
>>> b/src/mesa/main/uniform_query.cpp
>>> index 0e02a76..5f38aa5 100644
>>> --- a/src/mesa/main/uniform_query.cpp
>>> +++ b/src/mesa/main/uniform_query.cpp
>>> @@ -904,20 +904,23 @@ validate_uniform(GLint location, GLsizei count, 
>>> const GLvoid *values,
>>>   extern "C" void
>>>   _mesa_uniform(GLint location, GLsizei count, const GLvoid *values,
>>>                 struct gl_context *ctx, struct gl_shader_program 
>>> *shProg,
>>>                 enum glsl_base_type basicType, unsigned src_components)
>>>   {
>>>      unsigned offset;
>>>      int size_mul = glsl_base_type_is_64bit(basicType) ? 2 : 1;
>>>      struct gl_uniform_storage *uni;
>>>      if (_mesa_is_no_error_enabled(ctx)) {
>>> +      if (location == -1)
>>> +         return;
>>> +
>>>         uni = shProg->UniformRemapTable[location];
>>>         /* The array index specified by the uniform location is just the
>>>          * uniform location minus the base location of of the uniform.
>>>          */
>>>         assert(uni->array_elements > 0 || location == 
>>> (int)uni->remap_location);
>>>         offset = location - uni->remap_location;
>>>      } else {
>>>         uni = validate_uniform(location, count, values, &offset, ctx, 
>>> shProg,
>>>                                basicType, src_components);
>>>