pre-ipv6-only clean up patchset

PATCH Superseded Antonio Quartulli 2 participants 12 messages Jun 13, 2018

lore

Share & tools

lore

Antonio Quartulli

author

2018-06-13 08:12 UTC #0 lore raw

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

Antonio Quartulli

author

2018-06-13 08:12 UTC #1 lore raw

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.

Signed-off-by Antonio Quartulli <[email protected]...>

---
 src/openvpn/tun.c | 8 ++++++++
 1 file changed, 8 insertions(+)

src/openvpn/tun.c +8 -0

index 3eb0f78a..466d4d42 100644
@@ -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

Signature · 1 line

2.17.1

Gert Doering

2018-06-13 11:06 UTC #2 lore raw

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

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 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...

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
index 3eb0f78a..466d4d42 100644

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]...

Antonio Quartulli

author

2018-06-13 11:41 UTC #3 lore raw

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

Hi,

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 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...
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
index 3eb0f78a..466d4d42 100644
@@ -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 -0
         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 :-)

Quoted 3 lines

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

Quoted 7 lines

(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,

Signature · 1 line

Antonio Quartulli

Antonio Quartulli

author

2018-06-13 08:12 UTC #4 lore raw

PATCH 2/3 close_tun: always pass a valid tt pointer

In reply to Antonio Quartulli

From: 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.

Signed-off-by Antonio Quartulli <[email protected]...>

---
 src/openvpn/init.c |   7 +-
 src/openvpn/tun.c  | 434 ++++++++++++++++++++++-----------------------
 2 files changed, 220 insertions(+), 221 deletions(-)

src/openvpn/init.c +5 -2

index 36c1a4c4..b748357d 100644
@@ -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

Antonio Quartulli

author

2018-06-13 08:20 UTC #5 lore raw

Re: [Openvpn-devel] [PATCH 2/3] close_tun: always pass a valid tt pointer

Please ignore this patch. It was a just idling there when I did the git send-email.

Cheers,

Signature · 1 line

Antonio Quartulli

Antonio Quartulli

author

2018-06-13 08:12 UTC #6 lore raw

PATCH 2/3 tun: always pass a valid tt pointer

In reply to 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.

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 <[email protected]...>

---
 src/openvpn/init.c |   7 +-
 src/openvpn/tun.c  | 434 ++++++++++++++++++++++-----------------------
 2 files changed, 220 insertions(+), 221 deletions(-)

src/openvpn/init.c +5 -2

index 36c1a4c4..b748357d 100644
@@ -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

Gert Doering

2018-06-13 11:28 UTC #7 lore raw

Re: [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer

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.

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]...

Antonio Quartulli

author

2018-06-13 11:38 UTC #8 lore raw

Re: [Openvpn-devel] [PATCH 2/3] tun: always pass a valid tt pointer

Hi,

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

Quoted 2 lines

gert

ACK for the rest of the comments.

Cheers,

Signature · 1 line

Antonio Quartulli

Antonio Quartulli

author

2018-06-13 08:12 UTC #9 lore raw

PATCH 3/3 tun: get rid of tt->did_ifconfig member

In reply to Antonio Quartulli

From: 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).

Signed-off-by Antonio Quartulli <[email protected]...>

---
 src/openvpn/tun.c | 12 +-----------
 src/openvpn/tun.h |  1 -
 2 files changed, 1 insertion(+), 12 deletions(-)

src/openvpn/tun.c +1 ~~-11~~

index 932c9d2d..36429204 100644
@@ -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

index 6c57ad0d..9495d602 100644
@@ -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 */

Signature · 1 line

2.17.1

Gert Doering

2018-06-13 11:29 UTC #10 lore raw

Re: [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member

Hi,

Quoted 4 lines · Antonio Quartulli

From: 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.

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]...

Antonio Quartulli

author

2018-06-13 11:42 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH 3/3] tun: get rid of tt->did_ifconfig member

Quoted 9 lines · Gert Doering

Hi,
Quoted 4 lines · Antonio Quartulli
From: 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.
                  ^
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