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