[Mesa-dev] [PATCH] glsl: Fix memory leak in builtin_builder::_image_prototype.

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> On Fri, 2014-10-03 at 13:27 +0300, Francisco Jerez wrote:
>> Kenneth Graunke <kenneth at whitecape.org> writes:
>> 
>> > On Thursday, October 02, 2014 11:28:35 AM Ian Romanick wrote:
>> >> Since this is Curro's code, I'm CC'ing me.
>> >> 
>> >> On 10/01/2014 03:12 AM, Iago Toral Quiroga wrote:
>> >> > in_var calls the ir_variable constructor, which dups the variable name.
>> >> > ---
>> >> >  src/glsl/builtin_functions.cpp | 8 +++++---
>> >> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >> > 
>> >> > diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp
>> >> > index 5a024cb..7d61fcc 100644
>> >> > --- a/src/glsl/builtin_functions.cpp
>> >> > +++ b/src/glsl/builtin_functions.cpp
>> >> > @@ -4465,9 +4465,11 @@ builtin_builder::_image_prototype(const glsl_type *image_type,
>> >> >        sig->parameters.push_tail(in_var(glsl_type::int_type, "sample"));
>> >> >  
>> >> >     /* Data arguments. */
>> >> > -   for (unsigned i = 0; i < num_arguments; ++i)
>> >> > -      sig->parameters.push_tail(in_var(data_type,
>> >> > -                                       ralloc_asprintf(NULL, "arg%d", i)));
>> >> > +   for (unsigned i = 0; i < num_arguments; ++i) {
>> >> > +      char *arg_name = ralloc_asprintf(NULL, "arg%d", i);
>> >> > +      sig->parameters.push_tail(in_var(data_type, arg_name));
>> >> > +      ralloc_free(arg_name);
>> >> > +   }
>> >> 
>> >> Using a NULL memory context is generally bad... precisely because it
>> >> often leads to memory leaks.
>> >> 
>> >> There are a couple ways to fix this.  Since all of the image functions
>> >> have a limited number of parameters, we could either:
>> >> 
>> >>  - Have a fixed size buffer that we snprintf to.
>> >> 
>> >>  - Have a table of all the parameter names.
>> >> 
>> >>  - Since this is the function prototype, I don't think we need names for
>> >> the parameters at all.  Just pass NULL?
>> >
>> > Does anything even use the names?  I don't think anything does...at which point, why not just call them all "arg" and be done with it?
>> >
>> 
>> Aren't the names useful for debugging and error reporting?  But sure,
>> you're right that "arg1" isn't a lot more meaningful than "arg".
>
> Francisco: so I understand that you would be okay with just passing
> "arg" for all these parameters as Kenneth suggested? I can send the
> patch for this if we agree to do this.
>

*Shrug*, if we just call them all "arg", isn't it going to be a little
confusing when (if) we start emitting errors that reference the
parameter names?  You patch is fine as-is IMHO.

> Iago
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141006/09ccdf0c/attachment.sig>