pre-ipv6-only clean up patchset
cover pre-ipv6-only clean up patchset
This patchset is the result of some of Gert's comment on the IPv6-only patchset.
For this reason, this patchset is expected to be applied *before* applying future versions (v3...) of the IPv6-only patchset.
This patchset is mostly about restyling the code a bit and getting rid of unused members. Hence, there is no new feature being introduced.
Patch 1 is fixing potential memory leaks resulting from not releasing some argv and gc local variables.
Cheers,
p.s. buildbot is currently unusable due to the ongoing maintenance on the OpenVPN servers and therefor eonly local tests have been performed.
Antonio Quartulli (3):
tun: ensure gc and argv are always freed
tun: always pass a valid tt pointer
tun: get rid of tt->did_ifconfig member
src/openvpn/init.c | 7 +- src/openvpn/tun.c | 442 ++++++++++++++++++++++----------------------- src/openvpn/tun.h | 1 - 3 files changed, 223 insertions(+), 227 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/3 tun: ensure gc and argv are always freed
From: Antonio Quartulli <[email protected]...>
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.
--- src/openvpn/tun.c | 8 ++++++++ 1 file changed, 8 insertions(+)
src/openvpn/tun.c +8 -0
@@ -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 tunenv_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
Signature · 1 line
2.17.1
PATCH 2/3 close_tun: always pass a valid tt pointer
In reply to Antonio QuartulliFrom: Antonio Quartulli <[email protected]...>
This patch is a small "logic restyle" which basically moves the check against "!tt" outside of the various close_tun() functions and replaces it with an ASSERT.
This way the check if done only once and the function can rely on the assumption that "tt" is always valid.
This change is mainly to improve the code style inside the close_tun() implementations by removing one level of indentation.
No functional change is expected.
--- src/openvpn/init.c | 7 +- src/openvpn/tun.c | 434 ++++++++++++++++++++++----------------------- 2 files changed, 220 insertions(+), 221 deletions(-)
src/openvpn/init.c +5 -2
@@ -1847,8 +1847,11 @@ static void do_close_tun_simple(struct context *c) {msg (D_CLOSE ,"Closing TUN/TAP interface" ); -close_tun (c->c1.tuntap); - c->c1.tuntap =NULL ; +if (c->c1.tuntap) + { +close_tun (c->c1.tuntap); + c->c1.tuntap =NULL ; + } c->c1.tuntap_owned = false;#if P2MP CLEAR (c->c1.pulled_options_digest_save);
src/openvpn/tun.c +215 -219
590 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH 2/3 tun: always pass a valid tt pointer
In reply to Antonio QuartulliFrom: Antonio Quartulli <[email protected]...>
This patch is a small "logic restyle" which basically moves the check for "tt != NULL" outside of the various close_tun() implementations and replaces it with an ASSERT.
This way the check is done only once and the function can rely on the assumption that "tt" is always valid.
This change is mainly to improve the code style inside close_tun() implementations by removing one level of indentation.
No functional change is present.
--- src/openvpn/init.c | 7 +- src/openvpn/tun.c | 434 ++++++++++++++++++++++----------------------- 2 files changed, 220 insertions(+), 221 deletions(-)
src/openvpn/init.c +5 -2
@@ -1847,8 +1847,11 @@ static void do_close_tun_simple(struct context *c) {msg (D_CLOSE ,"Closing TUN/TAP interface" ); -close_tun (c->c1.tuntap); - c->c1.tuntap =NULL ; +if (c->c1.tuntap) + { +close_tun (c->c1.tuntap); + c->c1.tuntap =NULL ; + } c->c1.tuntap_owned = false;#if P2MP CLEAR (c->c1.pulled_options_digest_save);
src/openvpn/tun.c +215 -219
590 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH 3/3 tun: get rid of tt->did_ifconfig member
In reply to Antonio QuartulliFrom: Antonio Quartulli <[email protected]...>
tt->did_ifconfig is currently only read by the Linux platform, but it is currently iuselessly set also by every other system.
The Linux platform does not actually even need this member and can directly rely on tt->did_ifconfig_setup.
For the reasons above, remove the tt->did_ifconfig at all and use tt->did_ifconfig_setup where needed (close_tun() on Linux).
--- src/openvpn/tun.c | 12 +----------- src/openvpn/tun.h | 1 - 2 files changed, 1 insertion(+), 12 deletions(-)
src/openvpn/tun.c +1 -11
@@ -987,7 +987,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv); openvpn_execve_check(&argv, es, S_FATAL, "Linux ip -6 addr add failed"); } - tt->did_ifconfig = true;#else /* ifdef ENABLE_IPROUTE */ if (tun) {
@@ -1026,7 +1025,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv); openvpn_execve_check(&argv, es, S_FATAL, "Linux ifconfig inet6 failed"); } - tt->did_ifconfig = true;#endif /*ENABLE_IPROUTE*/ #elif defined(TARGET_ANDROID)
@@ -1186,8 +1184,6 @@ do_ifconfig(struct tuntap *tt, add_route(&r, tt, 0, NULL, es); } - tt->did_ifconfig = true; -#elif defined(TARGET_OPENBSD)in_addr_t remote_end;/* for "virtual" subnet topology */
@@ -1265,7 +1261,6 @@ do_ifconfig(struct tuntap *tt,/* and, hooray, we explicitely need to add a route... */ add_route_connected_v6_net(tt, es); } - tt->did_ifconfig = true;#elif defined(TARGET_NETBSD)
@@ -1327,7 +1322,6 @@ do_ifconfig(struct tuntap *tt,/* and, hooray, we explicitely need to add a route... */ add_route_connected_v6_net(tt, es); } - tt->did_ifconfig = true;#elif defined(TARGET_DARWIN)/*
@@ -1384,7 +1378,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv);openvpn_execve_check (&argv, es,S_FATAL ,"Mac OS X ifconfig failed" ); - tt->did_ifconfig = true;/* Add a network route for the local tun interface */ if (!tun && tt->topology ==TOP_SUBNET )
@@ -1457,7 +1450,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv);openvpn_execve_check (&argv, es,S_FATAL ,"FreeBSD ifconfig failed" ); - tt->did_ifconfig = true;/* Add a network route for the local tun interface */ if (!tun && tt->topology ==TOP_SUBNET )
@@ -1507,7 +1499,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv);openvpn_execve_check (&argv, aix_es,S_FATAL ,"AIX ifconfig failed" ); - tt->did_ifconfig = true;if (do_ipv6) {
@@ -1545,7 +1536,6 @@ do_ifconfig(struct tuntap *tt,break ; } - tt->did_ifconfig = true; }if (do_ipv6)
@@ -2104,7 +2094,7 @@ close_tun(struct tuntap *tt) {ASSERT (tt); -if (tt->type !=DEV_TYPE_NULL && tt->did_ifconfig) +if (tt->type !=DEV_TYPE_NULL && tt->did_ifconfig_setup) {struct argv argv =argv_new ();struct gc_arena gc =gc_new ();
src/openvpn/tun.h +0 -1
@@ -138,7 +138,6 @@ struct tuntapbool did_ifconfig_setup;bool did_ifconfig_ipv6_setup; -bool did_ifconfig;bool persistent_if;/* if existed before, keep on program end */
Signature · 1 line
2.17.1
Re: [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer
In reply to Antonio QuartulliPlease ignore this patch. It was a just idling there when I did the git send-email.
Cheers,
Signature · 1 line
Antonio Quartulli
Re: [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed
In reply to Antonio QuartulliHi,
Quoted 10 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
Everytime a argv object is initialized with
argv_new(), it has to be released withargv_reset()once not needed anymore.The same holds for
gc_arenaobjects initialized withgc_new()that have to be released withgc_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...
Quoted 10 lines
Signed-off-by Antonio Quartulli <[email protected]...>--- src/openvpn/tun.c | 8 ++++++++ 1 file changed, 8 insertions(+)src/openvpn/tun.c +0
-0
src/openvpn/tun.c close_tun(struct tuntap *tt) · line 2621
Quoted 7 lines
src/openvpn/tun.c +2
-0@@ -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
Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany [email protected]...Re: [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer
In reply to Antonio QuartulliHi,
Quoted 6 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
This patch is a small "logic restyle" which basically moves the check for "tt != NULL" outside of the various
close_tun()implementations and replaces it with an ASSERT.
Quoted 7 lines
This way the check is done only once and the function can rely on the assumption that "tt" is always valid.
This change is mainly to improve the code style inside
close_tun()implementations by removing one level of indentation.No functional change is present.
Mostly-ACK-but :-)
The changes look good. It won't apply after the gc_arena-fixed 1/3 of this series, so we need a new one.
I've applied the patch and checked with "git diff -w", and it does what it says on the lid...
Not sure the ASSERT(tt) is needed in solaris_error_close() - tt can never be NULL there (because all the caller functions are working with an active tt). So I'd tend to leave that out, it's not doing something useful.
In those close_tun() instances where you removed the
else if (tt)
{
}
part - some of them had a comment ("/* close and destroy */"). Maybe keep the comment?
As in
if (tt->persistent_if) /* keep pre-existing if around */
{
close_tun_generic(tt);
free(tt);
return;
}
/* close and destroy */ <<<<< this one
struct argv argv = argv_new();gert
Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany [email protected]...Re: [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member
In reply to Antonio QuartulliHi,
Quoted 4 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
tt->did_ifconfigis currently only read by the Linux platform, but it is currently iuselessly set also by every other system.
^
If you're respinning v2 of this set, there is a small typo here :-)
Otherwise, ACK. Thanks.
gert
Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany [email protected]...Re: [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer
In reply to Gert DoeringHi,
Quoted 22 lines · Gert Doering
Hi,
Quoted 6 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
This patch is a small "logic restyle" which basically moves the check for "tt != NULL" outside of the various
close_tun()implementations and replaces it with an ASSERT.Quoted 7 lines
This way the check is done only once and the function can rely on the assumption that "tt" is always valid.
This change is mainly to improve the code style inside
close_tun()implementations by removing one level of indentation.No functional change is present.
Mostly-ACK-but :-)
The changes look good. It won't apply after the gc_arena-fixed 1/3 of this series, so we need a new one.
Quoted 7 lines
I've applied the patch and checked with "git diff -w", and it does what it says on the lid...
Not sure the ASSERT(tt) is needed in
solaris_error_close()- tt can never be NULL there (because all the caller functions are working with an active tt). So I'd tend to leave that out, it's not doing something useful.
It's implicitly assumed, so I thought it would be helpful to make it explicit (mostly to warn other devs to forget about that).
But I'll remove it if you think it's superfluous.
Quoted 24 lines
In those
close_tun()instances where you removed theelse if (tt)
{}
part - some of them had a comment ("/* close and destroy */"). Maybe keep the comment?
As in
if (tt->persistent_if) /* keep pre-existing if around */ { close_tun_generic(tt); free(tt); return; } /* close and destroy */ <<<<< this one struct argv argv = argv_new();
Quoted 2 lines
gert
ACK for the rest of the comments.
Cheers,
Signature · 1 line
Antonio Quartulli
Re: [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed
In reply to Gert DoeringHi,
Quoted 31 lines · Gert Doering
Hi,
Quoted 10 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
Everytime a argv object is initialized with
argv_new(), it has to be released withargv_reset()once not needed anymore.The same holds for
gc_arenaobjects initialized withgc_new()that have to be released withgc_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...
Quoted 12 lines
Signed-off-by Antonio Quartulli <[email protected]...>--- src/openvpn/tun.c | 8 ++++++++ 1 file changed, 8 insertions(+)src/openvpn/tun.c +0
-0@@ -2615,6 +2615,8 @@ close_tun(struct tuntap *tt)openvpn_execve_check (&argv,NULL ,0 ,"OpenBSD 'destroy tun interface' failed (non-critical)" );
Quoted 10 lines
src/openvpn/tun.c close_tun(struct tuntap *tt) · line 2621
Quoted 5 lines
src/openvpn/tun.c +2
-0free (tt); +argv_reset (&argv); +gc_free (&gc); } }While this is technically correct, the
gc_arenais not used at all in thisclose_tun()instance (OPENBSD). So, please do not add a cleanup call, but just remove the "structgc_arenagc =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 :-)
Quoted 3 lines
The same holds for the NETBSD and AIX variants - gc/argv initialized, but only argv used. So, please remove
gc_arenainstead :-)
ok
Quoted 7 lines
(Side note: the FREEBSD
close_tun()variant is mostly the same, but does not have agc_arena... seems someone already stumbled across that one and fixed it)ACK on the remaining AIX
argv_reset()- oops, that was me :-)
Thanks,
Signature · 1 line
Antonio Quartulli
Re: [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member
In reply to Gert DoeringQuoted 9 lines · Gert Doering
Hi,
Quoted 4 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
tt->did_ifconfigis currently only read by the Linux platform, but it is currently iuselessly set also by every other system.^If you're respinning v2 of this set, there is a small typo here :-)
Thanks for spotting this.
Quoted 3 lines
Otherwise, ACK. Thanks.
will resend the entire patchset as v2.
Cheers,
Signature · 1 line
Antonio Quartulli