diff mbox series

[Openvpn-devel,1/3] tun: ensure gc and argv are always freed

Message ID 20180613081218.1834-2-a@unstable.cc
State Changes Requested
Headers show
Series pre-ipv6-only clean up patchset | expand

Commit Message

Antonio Quartulli June 12, 2018, 10:12 p.m. UTC 
From: Antonio Quartulli <antonio@openvpn.net>

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.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 src/openvpn/tun.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Gert Doering June 13, 2018, 1:06 a.m. UTC | #1 
Hi,

On Wed, Jun 13, 2018 at 04:12:15PM +0800, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@openvpn.net>
> 
> 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@openvpn.net>
> ---
>  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.

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

(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 :-)

gert
Antonio Quartulli June 13, 2018, 1:41 a.m. UTC | #2 
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@openvpn.net>
>>
>> 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@openvpn.net>
>> ---
>>  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,
diff mbox series

Patch

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);
     }
 }
 
@@ -2700,6 +2702,8 @@  close_tun(struct tuntap *tt)
         openvpn_execve_check(&argv, NULL, 0, "NetBSD 'destroy tun interface' failed (non-critical)");
 
         free(tt);
+        argv_reset(&argv);
+        gc_free(&gc);
     }
 }
 
@@ -2837,6 +2841,7 @@  close_tun(struct tuntap *tt)
         openvpn_execve_check(&argv, NULL, 0, "FreeBSD 'destroy tun interface' failed (non-critical)");
 
         free(tt);
+        argv_reset(&argv);
     }
 }
 
@@ -3308,6 +3313,7 @@  open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
         env_set_add( es, "ODMDIR=/etc/objrepos" );
         openvpn_execve_check(&argv, es, S_FATAL, "AIX 'create tun interface' failed");
         env_set_destroy(es);
+        argv_reset(&argv);
     }
     else
     {
@@ -3362,6 +3368,8 @@  close_tun(struct tuntap *tt)
 
     free(tt);
     env_set_destroy(es);
+    argv_reset(&argv);
+    gc_free(gc);
 }
 
 int