* [Openvpn-devel] [PATCH 0/3] pre-ipv6-only clean up patchset
@ 2018-06-13 8:12 Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed Antonio Quartulli
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 8:12 UTC (permalink / raw)
To: openvpn-devel; +Cc: Antonio Quartulli <a@
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(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed
2018-06-13 8:12 [Openvpn-devel] [PATCH 0/3] pre-ipv6-only clean up patchset Antonio Quartulli
@ 2018-06-13 8:12 ` Antonio Quartulli
2018-06-13 11:06 ` Gert Doering
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer Antonio Quartulli
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 8:12 UTC (permalink / raw)
To: openvpn-devel; +Cc: Antonio Quartulli <antonio@
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.
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);
}
}
@@ -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
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer
2018-06-13 8:12 [Openvpn-devel] [PATCH 0/3] pre-ipv6-only clean up patchset Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed Antonio Quartulli
@ 2018-06-13 8:12 ` Antonio Quartulli
2018-06-13 8:20 ` Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] tun: " Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member Antonio Quartulli
3 siblings, 1 reply; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 8:12 UTC (permalink / raw)
To: openvpn-devel; +Cc: Antonio Quartulli <antonio@
From: Antonio Quartulli <antonio@...515...>
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.
Signed-off-by: Antonio Quartulli <antonio@...515...>
---
src/openvpn/init.c | 7 +-
src/openvpn/tun.c | 434 ++++++++++++++++++++++-----------------------
2 files changed, 220 insertions(+), 221 deletions(-)
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 36c1a4c4..b748357d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -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);
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 466d4d42..932c9d2d 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1900,11 +1900,10 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- close_tun_generic(tt);
- free(tt);
- }
+ ASSERT(tt);
+
+ close_tun_generic(tt);
+ free(tt);
}
int
@@ -2103,77 +2102,76 @@ tuncfg(const char *dev, const char *dev_type, const char *dev_node, int persist_
void
close_tun(struct tuntap *tt)
{
- if (tt)
+ ASSERT(tt);
+
+ if (tt->type != DEV_TYPE_NULL && tt->did_ifconfig)
{
- if (tt->type != DEV_TYPE_NULL && tt->did_ifconfig)
- {
- struct argv argv = argv_new();
- struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
+ struct gc_arena gc = gc_new();
#ifdef ENABLE_IPROUTE
- if (is_tun_p2p(tt))
- {
- argv_printf(&argv,
- "%s addr del dev %s local %s peer %s",
- iproute_path,
- tt->actual_name,
- print_in_addr_t(tt->local, 0, &gc),
- print_in_addr_t(tt->remote_netmask, 0, &gc)
- );
- }
- else
- {
- argv_printf(&argv,
- "%s addr del dev %s %s/%d",
- iproute_path,
- tt->actual_name,
- print_in_addr_t(tt->local, 0, &gc),
- netmask_to_netbits2(tt->remote_netmask)
- );
- }
-#else /* ifdef ENABLE_IPROUTE */
+ if (is_tun_p2p(tt))
+ {
argv_printf(&argv,
- "%s %s 0.0.0.0",
- IFCONFIG_PATH,
- tt->actual_name
- );
+ "%s addr del dev %s local %s peer %s",
+ iproute_path,
+ tt->actual_name,
+ print_in_addr_t(tt->local, 0, &gc),
+ print_in_addr_t(tt->remote_netmask, 0, &gc)
+ );
+ }
+ else
+ {
+ argv_printf(&argv,
+ "%s addr del dev %s %s/%d",
+ iproute_path,
+ tt->actual_name,
+ print_in_addr_t(tt->local, 0, &gc),
+ netmask_to_netbits2(tt->remote_netmask)
+ );
+ }
+#else /* ifdef ENABLE_IPROUTE */
+ argv_printf(&argv,
+ "%s %s 0.0.0.0",
+ IFCONFIG_PATH,
+ tt->actual_name
+ );
#endif /* ifdef ENABLE_IPROUTE */
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "Linux ip addr del failed");
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "Linux ip addr del failed");
- if (tt->did_ifconfig_ipv6_setup)
- {
- const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+ if (tt->did_ifconfig_ipv6_setup)
+ {
+ const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
#ifdef ENABLE_IPROUTE
- argv_printf(&argv, "%s -6 addr del %s/%d dev %s",
- iproute_path,
- ifconfig_ipv6_local,
- tt->netbits_ipv6,
- tt->actual_name
- );
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed");
+ argv_printf(&argv, "%s -6 addr del %s/%d dev %s",
+ iproute_path,
+ ifconfig_ipv6_local,
+ tt->netbits_ipv6,
+ tt->actual_name
+ );
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed");
#else /* ifdef ENABLE_IPROUTE */
- argv_printf(&argv,
- "%s %s del %s/%d",
- IFCONFIG_PATH,
- tt->actual_name,
- ifconfig_ipv6_local,
- tt->netbits_ipv6
- );
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "Linux ifconfig inet6 del failed");
+ argv_printf(&argv,
+ "%s %s del %s/%d",
+ IFCONFIG_PATH,
+ tt->actual_name,
+ ifconfig_ipv6_local,
+ tt->netbits_ipv6
+ );
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "Linux ifconfig inet6 del failed");
#endif
- }
-
- argv_reset(&argv);
- gc_free(&gc);
}
- close_tun_generic(tt);
- free(tt);
+
+ argv_reset(&argv);
+ gc_free(&gc);
}
+ close_tun_generic(tt);
+ free(tt);
}
int
@@ -2491,18 +2489,17 @@ solaris_close_tun(struct tuntap *tt)
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- solaris_close_tun(tt);
+ ASSERT(tt);
- if (tt->actual_name)
- {
- free(tt->actual_name);
- }
+ solaris_close_tun(tt);
- clear_tuntap(tt);
- free(tt);
+ if (tt->actual_name)
+ {
+ free(tt->actual_name);
}
+
+ clear_tuntap(tt);
+ free(tt);
}
static void
@@ -2511,6 +2508,8 @@ solaris_error_close(struct tuntap *tt, const struct env_set *es,
{
struct argv argv = argv_new();
+ ASSERT(tt);
+
if (unplumb_inet6)
{
argv_printf( &argv, "%s %s inet6 unplumb",
@@ -2591,33 +2590,34 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
/* only *TAP* devices need destroying, tun devices auto-self-destruct
*/
- if (tt && (tt->type == DEV_TYPE_TUN || tt->persistent_if ) )
+ if (tt->type == DEV_TYPE_TUN || tt->persistent_if)
{
close_tun_generic(tt);
free(tt);
+ return;
}
- else if (tt)
- {
- struct gc_arena gc = gc_new();
- struct argv argv = argv_new();
- /* setup command, close tun dev (clears tt->actual_name!), run command
- */
+ struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s %s destroy",
- IFCONFIG_PATH, tt->actual_name);
+ /* setup command, close tun dev (clears tt->actual_name!), run command
+ */
- close_tun_generic(tt);
+ argv_printf(&argv, "%s %s destroy",
+ IFCONFIG_PATH, tt->actual_name);
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "OpenBSD 'destroy tun interface' failed (non-critical)");
+ close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- gc_free(&gc);
- }
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "OpenBSD 'destroy tun interface' failed (non-critical)");
+
+ free(tt);
+ argv_reset(&argv);
+ gc_free(&gc);
}
int
@@ -2678,33 +2678,34 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
/* only tun devices need destroying, tap devices auto-self-destruct
*/
- if (tt && ( tt->type != DEV_TYPE_TUN || tt->persistent_if ) )
+ if (tt->type != DEV_TYPE_TUN || tt->persistent_if)
{
close_tun_generic(tt);
free(tt);
+ return;
}
- else if (tt)
- {
- struct gc_arena gc = gc_new();
- struct argv argv = argv_new();
- /* setup command, close tun dev (clears tt->actual_name!), run command
- */
+ struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s %s destroy",
- IFCONFIG_PATH, tt->actual_name);
+ /* setup command, close tun dev (clears tt->actual_name!), run command
+ */
- close_tun_generic(tt);
+ argv_printf(&argv, "%s %s destroy",
+ IFCONFIG_PATH, tt->actual_name);
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "NetBSD 'destroy tun interface' failed (non-critical)");
+ close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- gc_free(&gc);
- }
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "NetBSD 'destroy tun interface' failed (non-critical)");
+
+ free(tt);
+ argv_reset(&argv);
+ gc_free(&gc);
}
static inline int
@@ -2820,29 +2821,31 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt && tt->persistent_if) /* keep pre-existing if around */
+ ASSERT(tt);
+
+ if (tt->persistent_if) /* keep pre-existing if around */
{
close_tun_generic(tt);
free(tt);
+ return;
}
- else if (tt) /* close and destroy */
- {
- struct argv argv = argv_new();
- /* setup command, close tun dev (clears tt->actual_name!), run command
- */
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s %s destroy",
- IFCONFIG_PATH, tt->actual_name);
+ /* setup command, close tun dev (clears tt->actual_name!), run command
+ */
- close_tun_generic(tt);
+ argv_printf(&argv, "%s %s destroy",
+ IFCONFIG_PATH, tt->actual_name);
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "FreeBSD 'destroy tun interface' failed (non-critical)");
+ close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- }
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0,
+ "FreeBSD 'destroy tun interface' failed (non-critical)");
+
+ free(tt);
+ argv_reset(&argv);
}
int
@@ -2933,11 +2936,10 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- close_tun_generic(tt);
- free(tt);
- }
+ ASSERT(tt);
+
+ close_tun_generic(tt);
+ free(tt);
}
int
@@ -3190,27 +3192,26 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- struct gc_arena gc = gc_new();
- struct argv argv = argv_new();
+ ASSERT(tt);
- if (tt->did_ifconfig_ipv6_setup)
- {
- const char *ifconfig_ipv6_local =
- print_in6_addr(tt->local_ipv6, 0, &gc);
+ struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s delete -inet6 %s",
- ROUTE_PATH, ifconfig_ipv6_local );
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "MacOS X 'remove inet6 route' failed (non-critical)");
- }
+ if (tt->did_ifconfig_ipv6_setup)
+ {
+ const char *ifconfig_ipv6_local =
+ print_in6_addr(tt->local_ipv6, 0, &gc);
- close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- gc_free(&gc);
+ argv_printf(&argv, "%s delete -inet6 %s",
+ ROUTE_PATH, ifconfig_ipv6_local );
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "MacOS X 'remove inet6 route' failed (non-critical)");
}
+
+ close_tun_generic(tt);
+ free(tt);
+ argv_reset(&argv);
+ gc_free(&gc);
}
int
@@ -3339,15 +3340,12 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
struct gc_arena gc = gc_new();
struct argv argv = argv_new();
struct env_set *es = env_set_create(NULL);
- if (!tt)
- {
- return;
- }
-
/* persistent devices need IP address unconfig, others need destroyal
*/
if (tt->persistent_if)
@@ -6187,104 +6185,103 @@ tun_show_debug(struct tuntap *tt)
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
struct gc_arena gc = gc_new();
- if (tt)
+ if (tt->did_ifconfig_ipv6_setup)
{
- if (tt->did_ifconfig_ipv6_setup)
- {
- /* remove route pointing to interface */
- delete_route_connected_v6_net(tt, NULL);
+ /* remove route pointing to interface */
+ delete_route_connected_v6_net(tt, NULL);
- if (tt->options.msg_channel)
+ if (tt->options.msg_channel)
+ {
+ do_address_service(false, AF_INET6, tt);
+ if (tt->options.dns6_len > 0)
{
- do_address_service(false, AF_INET6, tt);
- if (tt->options.dns6_len > 0)
- {
- do_dns6_service(false, tt);
- }
+ do_dns6_service(false, tt);
}
- else
- {
- const char *ifconfig_ipv6_local;
- struct argv argv = argv_new();
+ }
+ else
+ {
+ const char *ifconfig_ipv6_local;
+ struct argv argv = argv_new();
- /* "store=active" is needed in Windows 8(.1) to delete the
- * address we added (pointed out by Cedric Tabary).
- */
+ /* "store=active" is needed in Windows 8(.1) to delete the
+ * address we added (pointed out by Cedric Tabary).
+ */
- /* netsh interface ipv6 delete address \"%s\" %s */
- ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+ /* netsh interface ipv6 delete address \"%s\" %s */
+ ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+ argv_printf(&argv,
+ "%s%sc interface ipv6 delete address %s %s store=active",
+ get_win_sys_path(),
+ NETSH_PATH_SUFFIX,
+ tt->actual_name,
+ ifconfig_ipv6_local);
+
+ netsh_command(&argv, 1, M_WARN);
+
+ /* delete ipv6 dns servers if any were set */
+ if (tt->options.dns6_len > 0)
+ {
argv_printf(&argv,
- "%s%sc interface ipv6 delete address %s %s store=active",
+ "%s%sc interface ipv6 delete dns %s all",
get_win_sys_path(),
NETSH_PATH_SUFFIX,
- tt->actual_name,
- ifconfig_ipv6_local);
-
+ tt->actual_name);
netsh_command(&argv, 1, M_WARN);
-
- /* delete ipv6 dns servers if any were set */
- if (tt->options.dns6_len > 0)
- {
- argv_printf(&argv,
- "%s%sc interface ipv6 delete dns %s all",
- get_win_sys_path(),
- NETSH_PATH_SUFFIX,
- tt->actual_name);
- netsh_command(&argv, 1, M_WARN);
- }
- argv_reset(&argv);
}
+ argv_reset(&argv);
}
+ }
#if 1
- if (tt->ipapi_context_defined)
+ if (tt->ipapi_context_defined)
+ {
+ DWORD status;
+ if ((status = DeleteIPAddress(tt->ipapi_context)) != NO_ERROR)
{
- DWORD status;
- if ((status = DeleteIPAddress(tt->ipapi_context)) != NO_ERROR)
- {
- msg(M_WARN, "Warning: DeleteIPAddress[%u] failed on TAP-Windows adapter, status=%u : %s",
- (unsigned int)tt->ipapi_context,
- (unsigned int)status,
- strerror_win32(status, &gc));
- }
+ msg(M_WARN, "Warning: DeleteIPAddress[%u] failed on TAP-Windows adapter, status=%u : %s",
+ (unsigned int)tt->ipapi_context,
+ (unsigned int)status,
+ strerror_win32(status, &gc));
}
+ }
#endif
- dhcp_release(tt);
+ dhcp_release(tt);
- if (tt->hand != NULL)
+ if (tt->hand != NULL)
+ {
+ dmsg(D_WIN32_IO_LOW, "Attempting CancelIO on TAP-Windows adapter");
+ if (!CancelIo(tt->hand))
{
- dmsg(D_WIN32_IO_LOW, "Attempting CancelIO on TAP-Windows adapter");
- if (!CancelIo(tt->hand))
- {
- msg(M_WARN | M_ERRNO, "Warning: CancelIO failed on TAP-Windows adapter");
- }
+ msg(M_WARN | M_ERRNO, "Warning: CancelIO failed on TAP-Windows adapter");
}
+ }
- dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped read event on TAP-Windows adapter");
- overlapped_io_close(&tt->reads);
-
- dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped write event on TAP-Windows adapter");
- overlapped_io_close(&tt->writes);
+ dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped read event on TAP-Windows adapter");
+ overlapped_io_close(&tt->reads);
- if (tt->hand != NULL)
- {
- dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on TAP-Windows adapter");
- if (!CloseHandle(tt->hand))
- {
- msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on TAP-Windows adapter");
- }
- }
+ dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped write event on TAP-Windows adapter");
+ overlapped_io_close(&tt->writes);
- if (tt->actual_name)
+ if (tt->hand != NULL)
+ {
+ dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on TAP-Windows adapter");
+ if (!CloseHandle(tt->hand))
{
- free(tt->actual_name);
+ msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on TAP-Windows adapter");
}
+ }
- clear_tuntap(tt);
- free(tt);
+ if (tt->actual_name)
+ {
+ free(tt->actual_name);
}
+
+ clear_tuntap(tt);
+ free(tt);
gc_free(&gc);
}
@@ -6363,11 +6360,10 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- close_tun_generic(tt);
- free(tt);
- }
+ ASSERT(tt);
+
+ close_tun_generic(tt);
+ free(tt);
}
int
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer
2018-06-13 8:12 [Openvpn-devel] [PATCH 0/3] pre-ipv6-only clean up patchset Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer Antonio Quartulli
@ 2018-06-13 8:12 ` Antonio Quartulli
2018-06-13 11:28 ` Gert Doering
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member Antonio Quartulli
3 siblings, 1 reply; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 8:12 UTC (permalink / raw)
To: openvpn-devel; +Cc: Antonio Quartulli <antonio@
From: Antonio Quartulli <antonio@...515...>
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.
Signed-off-by: Antonio Quartulli <antonio@...515...>
---
src/openvpn/init.c | 7 +-
src/openvpn/tun.c | 434 ++++++++++++++++++++++-----------------------
2 files changed, 220 insertions(+), 221 deletions(-)
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 36c1a4c4..b748357d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -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);
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 466d4d42..932c9d2d 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1900,11 +1900,10 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- close_tun_generic(tt);
- free(tt);
- }
+ ASSERT(tt);
+
+ close_tun_generic(tt);
+ free(tt);
}
int
@@ -2103,77 +2102,76 @@ tuncfg(const char *dev, const char *dev_type, const char *dev_node, int persist_
void
close_tun(struct tuntap *tt)
{
- if (tt)
+ ASSERT(tt);
+
+ if (tt->type != DEV_TYPE_NULL && tt->did_ifconfig)
{
- if (tt->type != DEV_TYPE_NULL && tt->did_ifconfig)
- {
- struct argv argv = argv_new();
- struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
+ struct gc_arena gc = gc_new();
#ifdef ENABLE_IPROUTE
- if (is_tun_p2p(tt))
- {
- argv_printf(&argv,
- "%s addr del dev %s local %s peer %s",
- iproute_path,
- tt->actual_name,
- print_in_addr_t(tt->local, 0, &gc),
- print_in_addr_t(tt->remote_netmask, 0, &gc)
- );
- }
- else
- {
- argv_printf(&argv,
- "%s addr del dev %s %s/%d",
- iproute_path,
- tt->actual_name,
- print_in_addr_t(tt->local, 0, &gc),
- netmask_to_netbits2(tt->remote_netmask)
- );
- }
-#else /* ifdef ENABLE_IPROUTE */
+ if (is_tun_p2p(tt))
+ {
argv_printf(&argv,
- "%s %s 0.0.0.0",
- IFCONFIG_PATH,
- tt->actual_name
- );
+ "%s addr del dev %s local %s peer %s",
+ iproute_path,
+ tt->actual_name,
+ print_in_addr_t(tt->local, 0, &gc),
+ print_in_addr_t(tt->remote_netmask, 0, &gc)
+ );
+ }
+ else
+ {
+ argv_printf(&argv,
+ "%s addr del dev %s %s/%d",
+ iproute_path,
+ tt->actual_name,
+ print_in_addr_t(tt->local, 0, &gc),
+ netmask_to_netbits2(tt->remote_netmask)
+ );
+ }
+#else /* ifdef ENABLE_IPROUTE */
+ argv_printf(&argv,
+ "%s %s 0.0.0.0",
+ IFCONFIG_PATH,
+ tt->actual_name
+ );
#endif /* ifdef ENABLE_IPROUTE */
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "Linux ip addr del failed");
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "Linux ip addr del failed");
- if (tt->did_ifconfig_ipv6_setup)
- {
- const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+ if (tt->did_ifconfig_ipv6_setup)
+ {
+ const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
#ifdef ENABLE_IPROUTE
- argv_printf(&argv, "%s -6 addr del %s/%d dev %s",
- iproute_path,
- ifconfig_ipv6_local,
- tt->netbits_ipv6,
- tt->actual_name
- );
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed");
+ argv_printf(&argv, "%s -6 addr del %s/%d dev %s",
+ iproute_path,
+ ifconfig_ipv6_local,
+ tt->netbits_ipv6,
+ tt->actual_name
+ );
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed");
#else /* ifdef ENABLE_IPROUTE */
- argv_printf(&argv,
- "%s %s del %s/%d",
- IFCONFIG_PATH,
- tt->actual_name,
- ifconfig_ipv6_local,
- tt->netbits_ipv6
- );
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "Linux ifconfig inet6 del failed");
+ argv_printf(&argv,
+ "%s %s del %s/%d",
+ IFCONFIG_PATH,
+ tt->actual_name,
+ ifconfig_ipv6_local,
+ tt->netbits_ipv6
+ );
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "Linux ifconfig inet6 del failed");
#endif
- }
-
- argv_reset(&argv);
- gc_free(&gc);
}
- close_tun_generic(tt);
- free(tt);
+
+ argv_reset(&argv);
+ gc_free(&gc);
}
+ close_tun_generic(tt);
+ free(tt);
}
int
@@ -2491,18 +2489,17 @@ solaris_close_tun(struct tuntap *tt)
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- solaris_close_tun(tt);
+ ASSERT(tt);
- if (tt->actual_name)
- {
- free(tt->actual_name);
- }
+ solaris_close_tun(tt);
- clear_tuntap(tt);
- free(tt);
+ if (tt->actual_name)
+ {
+ free(tt->actual_name);
}
+
+ clear_tuntap(tt);
+ free(tt);
}
static void
@@ -2511,6 +2508,8 @@ solaris_error_close(struct tuntap *tt, const struct env_set *es,
{
struct argv argv = argv_new();
+ ASSERT(tt);
+
if (unplumb_inet6)
{
argv_printf( &argv, "%s %s inet6 unplumb",
@@ -2591,33 +2590,34 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
/* only *TAP* devices need destroying, tun devices auto-self-destruct
*/
- if (tt && (tt->type == DEV_TYPE_TUN || tt->persistent_if ) )
+ if (tt->type == DEV_TYPE_TUN || tt->persistent_if)
{
close_tun_generic(tt);
free(tt);
+ return;
}
- else if (tt)
- {
- struct gc_arena gc = gc_new();
- struct argv argv = argv_new();
- /* setup command, close tun dev (clears tt->actual_name!), run command
- */
+ struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s %s destroy",
- IFCONFIG_PATH, tt->actual_name);
+ /* setup command, close tun dev (clears tt->actual_name!), run command
+ */
- close_tun_generic(tt);
+ argv_printf(&argv, "%s %s destroy",
+ IFCONFIG_PATH, tt->actual_name);
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "OpenBSD 'destroy tun interface' failed (non-critical)");
+ close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- gc_free(&gc);
- }
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "OpenBSD 'destroy tun interface' failed (non-critical)");
+
+ free(tt);
+ argv_reset(&argv);
+ gc_free(&gc);
}
int
@@ -2678,33 +2678,34 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
/* only tun devices need destroying, tap devices auto-self-destruct
*/
- if (tt && ( tt->type != DEV_TYPE_TUN || tt->persistent_if ) )
+ if (tt->type != DEV_TYPE_TUN || tt->persistent_if)
{
close_tun_generic(tt);
free(tt);
+ return;
}
- else if (tt)
- {
- struct gc_arena gc = gc_new();
- struct argv argv = argv_new();
- /* setup command, close tun dev (clears tt->actual_name!), run command
- */
+ struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s %s destroy",
- IFCONFIG_PATH, tt->actual_name);
+ /* setup command, close tun dev (clears tt->actual_name!), run command
+ */
- close_tun_generic(tt);
+ argv_printf(&argv, "%s %s destroy",
+ IFCONFIG_PATH, tt->actual_name);
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "NetBSD 'destroy tun interface' failed (non-critical)");
+ close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- gc_free(&gc);
- }
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "NetBSD 'destroy tun interface' failed (non-critical)");
+
+ free(tt);
+ argv_reset(&argv);
+ gc_free(&gc);
}
static inline int
@@ -2820,29 +2821,31 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt && tt->persistent_if) /* keep pre-existing if around */
+ ASSERT(tt);
+
+ if (tt->persistent_if) /* keep pre-existing if around */
{
close_tun_generic(tt);
free(tt);
+ return;
}
- else if (tt) /* close and destroy */
- {
- struct argv argv = argv_new();
- /* setup command, close tun dev (clears tt->actual_name!), run command
- */
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s %s destroy",
- IFCONFIG_PATH, tt->actual_name);
+ /* setup command, close tun dev (clears tt->actual_name!), run command
+ */
- close_tun_generic(tt);
+ argv_printf(&argv, "%s %s destroy",
+ IFCONFIG_PATH, tt->actual_name);
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "FreeBSD 'destroy tun interface' failed (non-critical)");
+ close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- }
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0,
+ "FreeBSD 'destroy tun interface' failed (non-critical)");
+
+ free(tt);
+ argv_reset(&argv);
}
int
@@ -2933,11 +2936,10 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- close_tun_generic(tt);
- free(tt);
- }
+ ASSERT(tt);
+
+ close_tun_generic(tt);
+ free(tt);
}
int
@@ -3190,27 +3192,26 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- struct gc_arena gc = gc_new();
- struct argv argv = argv_new();
+ ASSERT(tt);
- if (tt->did_ifconfig_ipv6_setup)
- {
- const char *ifconfig_ipv6_local =
- print_in6_addr(tt->local_ipv6, 0, &gc);
+ struct gc_arena gc = gc_new();
+ struct argv argv = argv_new();
- argv_printf(&argv, "%s delete -inet6 %s",
- ROUTE_PATH, ifconfig_ipv6_local );
- argv_msg(M_INFO, &argv);
- openvpn_execve_check(&argv, NULL, 0, "MacOS X 'remove inet6 route' failed (non-critical)");
- }
+ if (tt->did_ifconfig_ipv6_setup)
+ {
+ const char *ifconfig_ipv6_local =
+ print_in6_addr(tt->local_ipv6, 0, &gc);
- close_tun_generic(tt);
- free(tt);
- argv_reset(&argv);
- gc_free(&gc);
+ argv_printf(&argv, "%s delete -inet6 %s",
+ ROUTE_PATH, ifconfig_ipv6_local );
+ argv_msg(M_INFO, &argv);
+ openvpn_execve_check(&argv, NULL, 0, "MacOS X 'remove inet6 route' failed (non-critical)");
}
+
+ close_tun_generic(tt);
+ free(tt);
+ argv_reset(&argv);
+ gc_free(&gc);
}
int
@@ -3339,15 +3340,12 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
struct gc_arena gc = gc_new();
struct argv argv = argv_new();
struct env_set *es = env_set_create(NULL);
- if (!tt)
- {
- return;
- }
-
/* persistent devices need IP address unconfig, others need destroyal
*/
if (tt->persistent_if)
@@ -6187,104 +6185,103 @@ tun_show_debug(struct tuntap *tt)
void
close_tun(struct tuntap *tt)
{
+ ASSERT(tt);
+
struct gc_arena gc = gc_new();
- if (tt)
+ if (tt->did_ifconfig_ipv6_setup)
{
- if (tt->did_ifconfig_ipv6_setup)
- {
- /* remove route pointing to interface */
- delete_route_connected_v6_net(tt, NULL);
+ /* remove route pointing to interface */
+ delete_route_connected_v6_net(tt, NULL);
- if (tt->options.msg_channel)
+ if (tt->options.msg_channel)
+ {
+ do_address_service(false, AF_INET6, tt);
+ if (tt->options.dns6_len > 0)
{
- do_address_service(false, AF_INET6, tt);
- if (tt->options.dns6_len > 0)
- {
- do_dns6_service(false, tt);
- }
+ do_dns6_service(false, tt);
}
- else
- {
- const char *ifconfig_ipv6_local;
- struct argv argv = argv_new();
+ }
+ else
+ {
+ const char *ifconfig_ipv6_local;
+ struct argv argv = argv_new();
- /* "store=active" is needed in Windows 8(.1) to delete the
- * address we added (pointed out by Cedric Tabary).
- */
+ /* "store=active" is needed in Windows 8(.1) to delete the
+ * address we added (pointed out by Cedric Tabary).
+ */
- /* netsh interface ipv6 delete address \"%s\" %s */
- ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+ /* netsh interface ipv6 delete address \"%s\" %s */
+ ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+ argv_printf(&argv,
+ "%s%sc interface ipv6 delete address %s %s store=active",
+ get_win_sys_path(),
+ NETSH_PATH_SUFFIX,
+ tt->actual_name,
+ ifconfig_ipv6_local);
+
+ netsh_command(&argv, 1, M_WARN);
+
+ /* delete ipv6 dns servers if any were set */
+ if (tt->options.dns6_len > 0)
+ {
argv_printf(&argv,
- "%s%sc interface ipv6 delete address %s %s store=active",
+ "%s%sc interface ipv6 delete dns %s all",
get_win_sys_path(),
NETSH_PATH_SUFFIX,
- tt->actual_name,
- ifconfig_ipv6_local);
-
+ tt->actual_name);
netsh_command(&argv, 1, M_WARN);
-
- /* delete ipv6 dns servers if any were set */
- if (tt->options.dns6_len > 0)
- {
- argv_printf(&argv,
- "%s%sc interface ipv6 delete dns %s all",
- get_win_sys_path(),
- NETSH_PATH_SUFFIX,
- tt->actual_name);
- netsh_command(&argv, 1, M_WARN);
- }
- argv_reset(&argv);
}
+ argv_reset(&argv);
}
+ }
#if 1
- if (tt->ipapi_context_defined)
+ if (tt->ipapi_context_defined)
+ {
+ DWORD status;
+ if ((status = DeleteIPAddress(tt->ipapi_context)) != NO_ERROR)
{
- DWORD status;
- if ((status = DeleteIPAddress(tt->ipapi_context)) != NO_ERROR)
- {
- msg(M_WARN, "Warning: DeleteIPAddress[%u] failed on TAP-Windows adapter, status=%u : %s",
- (unsigned int)tt->ipapi_context,
- (unsigned int)status,
- strerror_win32(status, &gc));
- }
+ msg(M_WARN, "Warning: DeleteIPAddress[%u] failed on TAP-Windows adapter, status=%u : %s",
+ (unsigned int)tt->ipapi_context,
+ (unsigned int)status,
+ strerror_win32(status, &gc));
}
+ }
#endif
- dhcp_release(tt);
+ dhcp_release(tt);
- if (tt->hand != NULL)
+ if (tt->hand != NULL)
+ {
+ dmsg(D_WIN32_IO_LOW, "Attempting CancelIO on TAP-Windows adapter");
+ if (!CancelIo(tt->hand))
{
- dmsg(D_WIN32_IO_LOW, "Attempting CancelIO on TAP-Windows adapter");
- if (!CancelIo(tt->hand))
- {
- msg(M_WARN | M_ERRNO, "Warning: CancelIO failed on TAP-Windows adapter");
- }
+ msg(M_WARN | M_ERRNO, "Warning: CancelIO failed on TAP-Windows adapter");
}
+ }
- dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped read event on TAP-Windows adapter");
- overlapped_io_close(&tt->reads);
-
- dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped write event on TAP-Windows adapter");
- overlapped_io_close(&tt->writes);
+ dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped read event on TAP-Windows adapter");
+ overlapped_io_close(&tt->reads);
- if (tt->hand != NULL)
- {
- dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on TAP-Windows adapter");
- if (!CloseHandle(tt->hand))
- {
- msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on TAP-Windows adapter");
- }
- }
+ dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped write event on TAP-Windows adapter");
+ overlapped_io_close(&tt->writes);
- if (tt->actual_name)
+ if (tt->hand != NULL)
+ {
+ dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on TAP-Windows adapter");
+ if (!CloseHandle(tt->hand))
{
- free(tt->actual_name);
+ msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on TAP-Windows adapter");
}
+ }
- clear_tuntap(tt);
- free(tt);
+ if (tt->actual_name)
+ {
+ free(tt->actual_name);
}
+
+ clear_tuntap(tt);
+ free(tt);
gc_free(&gc);
}
@@ -6363,11 +6360,10 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
void
close_tun(struct tuntap *tt)
{
- if (tt)
- {
- close_tun_generic(tt);
- free(tt);
- }
+ ASSERT(tt);
+
+ close_tun_generic(tt);
+ free(tt);
}
int
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member
2018-06-13 8:12 [Openvpn-devel] [PATCH 0/3] pre-ipv6-only clean up patchset Antonio Quartulli
` (2 preceding siblings ...)
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] tun: " Antonio Quartulli
@ 2018-06-13 8:12 ` Antonio Quartulli
2018-06-13 11:29 ` Gert Doering
3 siblings, 1 reply; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 8:12 UTC (permalink / raw)
To: openvpn-devel; +Cc: Antonio Quartulli <antonio@
From: Antonio Quartulli <antonio@...515...>
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).
Signed-off-by: Antonio Quartulli <antonio@...515...>
---
src/openvpn/tun.c | 12 +-----------
src/openvpn/tun.h | 1 -
2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 932c9d2d..36429204 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -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();
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index 6c57ad0d..9495d602 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -138,7 +138,6 @@ struct tuntap
bool did_ifconfig_setup;
bool did_ifconfig_ipv6_setup;
- bool did_ifconfig;
bool persistent_if; /* if existed before, keep on program end */
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer Antonio Quartulli
@ 2018-06-13 8:20 ` Antonio Quartulli
0 siblings, 0 replies; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 8:20 UTC (permalink / raw)
To: openvpn-devel; +Cc: Antonio Quartulli <antonio@
[-- Attachment #1.1: Type: text/plain, Size: 127 bytes --]
Please ignore this patch. It was a just idling there when I did the git
send-email.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed Antonio Quartulli
@ 2018-06-13 11:06 ` Gert Doering
2018-06-13 11:41 ` Antonio Quartulli
0 siblings, 1 reply; 12+ messages in thread
From: Gert Doering @ 2018-06-13 11:06 UTC (permalink / raw)
To: Antonio Quartulli <a@; +Cc: openvpn-devel, Antonio Quartulli <antonio@
[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]
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.
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
--
"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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] tun: " Antonio Quartulli
@ 2018-06-13 11:28 ` Gert Doering
2018-06-13 11:38 ` Antonio Quartulli
0 siblings, 1 reply; 12+ messages in thread
From: Gert Doering @ 2018-06-13 11:28 UTC (permalink / raw)
To: Antonio Quartulli <a@; +Cc: openvpn-devel, Antonio Quartulli <antonio@
[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]
Hi,
On Wed, Jun 13, 2018 at 04:12:17PM +0800, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@...515...>
>
> 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.
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
--
"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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member Antonio Quartulli
@ 2018-06-13 11:29 ` Gert Doering
2018-06-13 11:42 ` Antonio Quartulli
0 siblings, 1 reply; 12+ messages in thread
From: Gert Doering @ 2018-06-13 11:29 UTC (permalink / raw)
To: Antonio Quartulli <a@; +Cc: openvpn-devel, Antonio Quartulli <antonio@
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
Hi,
On Wed, Jun 13, 2018 at 04:12:18PM +0800, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@...515...>
>
> tt->did_ifconfig is 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
--
"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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer
2018-06-13 11:28 ` Gert Doering
@ 2018-06-13 11:38 ` Antonio Quartulli
0 siblings, 0 replies; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 11:38 UTC (permalink / raw)
Cc: openvpn-devel
[-- Attachment #1.1: Type: text/plain, Size: 1906 bytes --]
Hi,
On 13/06/18 19:28, Gert Doering wrote:
> Hi,
>
> On Wed, Jun 13, 2018 at 04:12:17PM +0800, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@...515...>
>>
>> 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.
>
> 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.
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.
>
>
> 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
>
ACK for the rest of the comments.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed
2018-06-13 11:06 ` Gert Doering
@ 2018-06-13 11:41 ` Antonio Quartulli
0 siblings, 0 replies; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 11:41 UTC (permalink / raw)
Cc: openvpn-devel
[-- Attachment #1.1: Type: text/plain, Size: 2043 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member
2018-06-13 11:29 ` Gert Doering
@ 2018-06-13 11:42 ` Antonio Quartulli
0 siblings, 0 replies; 12+ messages in thread
From: Antonio Quartulli @ 2018-06-13 11:42 UTC (permalink / raw)
Cc: openvpn-devel
[-- Attachment #1.1: Type: text/plain, Size: 553 bytes --]
On 13/06/18 19:29, Gert Doering wrote:
> Hi,
>
> On Wed, Jun 13, 2018 at 04:12:18PM +0800, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@...515...>
>>
>> tt->did_ifconfig is 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.
>
> Otherwise, ACK. Thanks.
>
will resend the entire patchset as v2.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-06-13 11:42 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-13 8:12 [Openvpn-devel] [PATCH 0/3] pre-ipv6-only clean up patchset Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 1/3] tun: ensure gc and argv are always freed Antonio Quartulli
2018-06-13 11:06 ` Gert Doering
2018-06-13 11:41 ` Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer Antonio Quartulli
2018-06-13 8:20 ` Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 2/3] tun: " Antonio Quartulli
2018-06-13 11:28 ` Gert Doering
2018-06-13 11:38 ` Antonio Quartulli
2018-06-13 8:12 ` [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member Antonio Quartulli
2018-06-13 11:29 ` Gert Doering
2018-06-13 11:42 ` Antonio Quartulli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.