Hi,

On 13/06/18 19:06, Gert Doering wrote:
> Hi,
> 
> On Wed, Jun 13, 2018 at 04:12:15PM +0800, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@...515...>
>>
>> Everytime a argv object is initialized with argv_new(), it has
>> to be released with argv_reset() once not needed anymore.
>>
>> The same holds for gc_arena objects initialized with gc_new() that
>> have to be released with gc_free().
>>
>> Ensure both kind of objects are always properly released to avoid
>> memory leaks.
> 
> This is good housekeeping.  Still, I need to NAK this, because I never
> agree to anything on the first go ;-)
> 
> Specifically...
> 
>> Signed-off-by: Antonio Quartulli <antonio@...515...>
>> ---
>>  src/openvpn/tun.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
>> index 3eb0f78a..466d4d42 100644
>> --- a/src/openvpn/tun.c
>> +++ b/src/openvpn/tun.c
>> @@ -2615,6 +2615,8 @@ close_tun(struct tuntap *tt)
>>          openvpn_execve_check(&argv, NULL, 0, "OpenBSD 'destroy tun interface' failed (non-critical)");
>>  
>>          free(tt);
>> +        argv_reset(&argv);
>> +        gc_free(&gc);
>>      }
>>  }
> 
> While this is technically correct, the gc_arena is not used at all in 
> this close_tun() instance (OPENBSD).  So, please do not add a cleanup call, 
> but just remove the "struct gc_arena gc = gc_new()" call instead.
> 

Oh, right! I was so focused on the gc_new/gc_free pattern that I did not
realize it was not used at all. Well, I assumed it was there for a
reason :-)

> The same holds for the NETBSD and AIX variants - gc/argv initialized, but 
> only argv used.  So, please remove gc_arena instead :-)
> 

ok

> (Side note: the FREEBSD close_tun() variant is mostly the same, but does
> not have a gc_arena ... seems someone already stumbled across that one
> and fixed it)
> 
> 
> ACK on the remaining AIX argv_reset() - oops, that was me :-)
> 

Thanks,


-- 
Antonio Quartulli