Re: [PATCH] BUG/MINOR: vars: Fix memory leak in vars_check_arg

2019-05-12 Thread Willy Tarreau 
On Sun, May 12, 2019 at 06:01:26PM +0200, Tim Düsterhus wrote:
> Willy,
> 
> Am 11.05.19 um 05:53 schrieb Willy Tarreau:
> >> diff --git a/src/vars.c b/src/vars.c
> >> index 477a14632..d32310270 100644
> >> --- a/src/vars.c
> >> +++ b/src/vars.c
> >> @@ -510,6 +510,7 @@ int vars_check_arg(struct arg *arg, char **err)
> >> err);
> >>if (!name)
> >>return 0;
> >> +  free(arg->data.str.area);
> > 
> > Here I'll add "arg->data.str.area=NULL". It significantly simplifies 
> > debugging
> > sessions to avoid leaving pointers to freed areas in various structs.
> 
> I believe it is not strictly necessary here, because the pointer is
> overridden directly below - when writing to the scope member of the var
> struct (otherwise valgrind would not have complained!) - but I agree
> that this generally is good style.

Absolutely, it was really not for correctness but to remove doubts over
the long term. When the code evolves, some paths that granted certain
conditions do not grant them anymore and this is when such cases start
to create doubts.

Cheers,
Willy

Re: [PATCH] BUG/MINOR: vars: Fix memory leak in vars_check_arg

2019-05-12 Thread Tim Düsterhus 
Willy,

Am 11.05.19 um 05:53 schrieb Willy Tarreau:
>> diff --git a/src/vars.c b/src/vars.c
>> index 477a14632..d32310270 100644
>> --- a/src/vars.c
>> +++ b/src/vars.c
>> @@ -510,6 +510,7 @@ int vars_check_arg(struct arg *arg, char **err)
>>   err);
>>  if (!name)
>>  return 0;
>> +free(arg->data.str.area);
> 
> Here I'll add "arg->data.str.area=NULL". It significantly simplifies debugging
> sessions to avoid leaving pointers to freed areas in various structs.

I believe it is not strictly necessary here, because the pointer is
overridden directly below - when writing to the scope member of the var
struct (otherwise valgrind would not have complained!) - but I agree
that this generally is good style.

Thanks
Tim Düsterhus

Re: [PATCH] BUG/MINOR: vars: Fix memory leak in vars_check_arg

2019-05-10 Thread Willy Tarreau 
On Fri, May 10, 2019 at 05:50:50PM +0200, Tim Duesterhus wrote:
> vars_check_arg previously leaked the string containing the variable
> name:
(...)

Thanks Tim! I'm going to apply a minor change :

> diff --git a/src/vars.c b/src/vars.c
> index 477a14632..d32310270 100644
> --- a/src/vars.c
> +++ b/src/vars.c
> @@ -510,6 +510,7 @@ int vars_check_arg(struct arg *arg, char **err)
>err);
>   if (!name)
>   return 0;
> + free(arg->data.str.area);

Here I'll add "arg->data.str.area=NULL". It significantly simplifies debugging
sessions to avoid leaving pointers to freed areas in various structs.

Thanks!
Willy