[Mesa-dev] [PATCH 5/9] glsl: Fix memcpy size in ir_constant

[Ian Romanick]

On 02/09/2014 01:50 AM, Juha-Pekka Heikkilä wrote:
> The place which cause Klocwork to highlight this is at
> src/mesa/main/ff_fragment_shader.cpp around line 897 where it says:
> 
> ...
>      float const_data[4] = {
>         float(1 << rgb_shift),
>         float(1 << rgb_shift),
>         float(1 << rgb_shift),
>         float(1 << alpha_shift)
>      };
>      shift = new(p->mem_ctx) ir_constant(glsl_type::vec4_type,
>                          (ir_constant_data *)const_data);
> ...
> 
> I don't know if this is the only place for such usage but it looks
> reasonable to me.
> 
> If I fix this place instead of what my patch does Klocwork would tell
> where is next similar usage for this ir_constant constructor, that is
> if such exist.

The ir_constant constructors are used in a lot of places.  I'd rather
not take the extra overhead to work around callers lying about the
parameter type.  If we have to keep fixing these one at a time, I'm okay
with that.  The error case is (most likely) that we read some garbage
data from the stack.  We should fix those, but it's not a cataclysmic
error.

Doing a quick check... 

egrep -r 'new[^ ]* ir_constant[(][^,)]*,[^)]*[)]' src/

shows

src/mesa/drivers/dri/i965/brw_fs_fp.cpp:         ir->coordinate = new(mem_ctx) ir_constant(coordinate_type, &junk_data);
src/glsl/builtin_functions.cpp:   return new(mem_ctx) ir_constant(f, vector_elements);
src/glsl/builtin_functions.cpp:   return new(mem_ctx) ir_constant(i, vector_elements);
src/glsl/builtin_functions.cpp:   return new(mem_ctx) ir_constant(u, vector_elements);
src/glsl/builtin_functions.cpp:   return new(mem_ctx) ir_constant(type, &data);
src/glsl/ir_clone.cpp:      return new(mem_ctx) ir_constant(this->type, &this->value);
src/glsl/opt_constant_propagation.cpp:   *rvalue = new(ralloc_parent(deref)) ir_constant(type, &data);
src/glsl/ir_constant_expression.cpp:   return new(ctx) ir_constant(this->type, &data);
src/glsl/ir_constant_expression.cpp:      return new(ctx) ir_constant(this->type, &data);
src/glsl/ir_constant_expression.cpp:	 return new(ctx) ir_constant(column_type, &data);
src/glsl/ir_constant_expression.cpp:	 return new(ctx) ir_constant(array, component);
src/glsl/opt_algebraic.cpp:	 return new(mem_ctx) ir_constant(ir->type, &data);
src/glsl/ast_to_hir.cpp:   var->constant_value = new(var) ir_constant(glsl_type::ivec3_type, &data);
src/glsl/ast_to_hir.cpp:      new(var) ir_constant(glsl_type::ivec3_type, &data);
src/glsl/lower_instructions.cpp:   ir_constant *sign_mask = new(ir) ir_constant(0x80000000u, vec_elem);
src/glsl/lower_instructions.cpp:   ir_constant *exp_shift = new(ir) ir_constant(23u, vec_elem);
src/glsl/lower_instructions.cpp:   ir_constant *exp_width = new(ir) ir_constant(8u, vec_elem);
src/glsl/lower_instructions.cpp:                                  new(ir) ir_constant(0x1, vec_elem))));
src/glsl/tests/uniform_initializer_utils.cpp:   val = new(mem_ctx) ir_constant(type, &data);
src/glsl/tests/uniform_initializer_utils.cpp:   val = new(mem_ctx) ir_constant(array_type, &values_for_array);
src/glsl/ast_function.cpp:      return new(ctx) ir_constant(constant, component);
src/glsl/ast_function.cpp:      return new(ctx) ir_constant(constructor_type, &actual_parameters);
src/glsl/ast_function.cpp:      return new(ctx) ir_constant(constructor_type, &actual_parameters);
src/glsl/ast_function.cpp:   return new(mem_ctx) ir_constant(constructor_type, parameters);
src/glsl/ast_function.cpp:	 ir_rvalue *rhs = new(ctx) ir_constant(rhs_type, &data);
src/glsl/ast_function.cpp:				new(ctx) ir_constant(rhs_var->type, &zero),
src/glsl/ast_function.cpp:	    ir_rvalue *const rhs = new(ctx) ir_constant(col_type, &ident);
src/glsl/ast_function.cpp:	 return new(ctx) ir_constant(constructor_type, &actual_parameters);
src/glsl/ir_reader.cpp:      return new(mem_ctx) ir_constant(type, &elements);
src/glsl/ir_reader.cpp:   return new(mem_ctx) ir_constant(type, &data);
src/glsl/builtin_variables.cpp:   var->constant_value = new(var) ir_constant(glsl_type::ivec3_type, &data);
src/glsl/builtin_variables.cpp:      new(var) ir_constant(glsl_type::ivec3_type, &data);

Of those, I think we only need to audit:

src/mesa/drivers/dri/i965/brw_fs_fp.cpp:         ir->coordinate = new(mem_ctx) ir_constant(coordinate_type, &junk_data);
src/glsl/builtin_functions.cpp:   return new(mem_ctx) ir_constant(type, &data);
src/glsl/opt_constant_propagation.cpp:   *rvalue = new(ralloc_parent(deref)) ir_constant(type, &data);
src/glsl/ir_constant_expression.cpp:   return new(ctx) ir_constant(this->type, &data);
src/glsl/ir_constant_expression.cpp:      return new(ctx) ir_constant(this->type, &data);
src/glsl/ir_constant_expression.cpp:	 return new(ctx) ir_constant(column_type, &data);
src/glsl/ir_constant_expression.cpp:	 return new(ctx) ir_constant(array, component);
src/glsl/opt_algebraic.cpp:	 return new(mem_ctx) ir_constant(ir->type, &data);
src/glsl/ast_to_hir.cpp:   var->constant_value = new(var) ir_constant(glsl_type::ivec3_type, &data);
src/glsl/ast_to_hir.cpp:      new(var) ir_constant(glsl_type::ivec3_type, &data);
src/glsl/tests/uniform_initializer_utils.cpp:   val = new(mem_ctx) ir_constant(type, &data);
src/glsl/tests/uniform_initializer_utils.cpp:   val = new(mem_ctx) ir_constant(array_type, &values_for_array);
src/glsl/ast_function.cpp:	 ir_rvalue *rhs = new(ctx) ir_constant(rhs_type, &data);
src/glsl/ast_function.cpp:				new(ctx) ir_constant(rhs_var->type, &zero),
src/glsl/ast_function.cpp:	    ir_rvalue *const rhs = new(ctx) ir_constant(col_type, &ident);
src/glsl/ir_reader.cpp:      return new(mem_ctx) ir_constant(type, &elements);
src/glsl/ir_reader.cpp:   return new(mem_ctx) ir_constant(type, &data);
src/glsl/builtin_variables.cpp:   var->constant_value = new(var) ir_constant(glsl_type::ivec3_type, &data);
src/glsl/builtin_variables.cpp:      new(var) ir_constant(glsl_type::ivec3_type, &data);

I skimmed a few of these, and the ones I looked at were fine... none of
them have an explicit cast, so the compiler would complain if they
weren't fine. :)

This search obviously isn't perfect... it misses the case we're
discussing.  I'm sure there's some nice cscope-like tool for C++ that
would find all the callers of that particular constructor.

> /Juha-Pekka
> 
> 
> On Sat, Feb 8, 2014 at 2:30 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 02/07/2014 04:44 AM, Juha-Pekka Heikkila wrote:
>>> ir_constant::ir_constant(const struct glsl_type,
>>> const ir_constant_data *) was copying too much memory.
>>
>> The code looks correct as-is to me.  This copies one ir_constant_data
>> union to another... they're declared as the same type, and they have the
>> same size.  What is the actual error?  Is there some code somewhere
>> that's casting a different type to ir_constant_data* to pass into this
>> constructor?
>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>> ---
>>>  src/glsl/ir.cpp | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
>>> index 1a36bd6..abc5568 100644
>>> --- a/src/glsl/ir.cpp
>>> +++ b/src/glsl/ir.cpp
>>> @@ -622,7 +622,7 @@ ir_constant::ir_constant(const struct glsl_type *type,
>>>
>>>     this->ir_type = ir_type_constant;
>>>     this->type = type;
>>> -   memcpy(& this->value, data, sizeof(this->value));
>>> +   memcpy(& this->value, data, type->std140_size(false));
>>>  }
>>>
>>>  ir_constant::ir_constant(float f, unsigned vector_elements)