Skip to contentSkip to conversation

Fix (potential) memory leak in init_route_list()

PATCH New Steffan Karger 3 participants 6 messages Nov 5, 2015 – Nov 10, 2015

lore
Share & tools
lore

Steffan Karger

author
#0 lore raw

PATCH 1/1 Fix (potential) memory leak in init_route_list()

init_route() can allocate memory in netlist, but fail in many more ways than just fail to allocate. Thus, always check and clean up netlist if needed, instead of just when init_route() succeeds.

This fix is for master only. The release/2.3 branch cleans up netlist immediately, and needs a different patch for a similar problem.

Found using coverity.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/route.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
src/openvpn/route.c +2 -1
index 096e3bc..bf377d8 100644
@@ -642,7 +642,6 @@ init_route_list (struct route_list *rl,
 	else
 	  {
             struct addrinfo* curele;
-            gc_addspecial(netlist, &gc_freeaddrinfo_callback, &gc);
             for (curele	= netlist; curele; curele = curele->ai_next)
 	      {
                 struct route_ipv4 *new;

@@ -653,6 +652,8 @@ init_route_list (struct route_list *rl,
                 rl->routes = new;
 	      }
 	  }
+	if (netlist)
+	  gc_addspecial(netlist, &gc_freeaddrinfo_callback, &gc);
       }
   }
Signature · 1 line
2.5.0
author
#1 lore raw

PATCH 1/1 Fix (potential) memory leak in init_route_list()

init_route() can allocate memory in netlist, but fail in many more ways than just fail to allocate. Thus, always check and clean up netlist if needed, instead of just when init_route() succeeds.

This fix is for master only. The release/2.3 branch cleans up netlist immediately, and needs a different patch for a similar problem.

Found using coverity.

v2: initialize netlist to NULL

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/route.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
src/openvpn/route.c +3 -2
index 096e3bc..58b16c0 100644
@@ -634,7 +634,7 @@ init_route_list (struct route_list *rl,
     struct route_option *ro;
     for (ro = opt->routes; ro; ro = ro->next)
       {
-        struct addrinfo* netlist;
+        struct addrinfo* netlist = NULL;
 	struct route_ipv4 r;
 
 	if (!init_route (&r, &netlist, ro, rl))

@@ -642,7 +642,6 @@ init_route_list (struct route_list *rl,
 	else
 	  {
             struct addrinfo* curele;
-            gc_addspecial(netlist, &gc_freeaddrinfo_callback, &gc);
             for (curele	= netlist; curele; curele = curele->ai_next)
 	      {
                 struct route_ipv4 *new;

@@ -653,6 +652,8 @@ init_route_list (struct route_list *rl,
                 rl->routes = new;
 	      }
 	  }
+	if (netlist)
+	  gc_addspecial(netlist, &gc_freeaddrinfo_callback, &gc);
       }
   }
Signature · 1 line
2.5.0
#2 lore raw

Hi,

Quoted 10 lines · Steffan Karger

init_route() can allocate memory in netlist, but fail in many more ways than just fail to allocate. Thus, always check and clean up netlist if needed, instead of just when init_route() succeeds.

This fix is for master only. The release/2.3 branch cleans up netlist immediately, and needs a different patch for a similar problem.

Found using coverity.

v2: initialize netlist to NULL

I'm fine with the patch as is (so "ACK"), but I'm wondering about something.

Quoted 9 lines
src/openvpn/route.c +0 -1
@@ -642,7 +642,6 @@ init_route_list (struct route_list *rl,
 	else
 	  {
             struct addrinfo* curele;
-            gc_addspecial(netlist, &gc_freeaddrinfo_callback, &gc);
             for (curele	= netlist; curele; curele = curele->ai_next)
 	      {
                 struct route_ipv4 *new;

src/openvpn/route.c init_route_list (struct route_list *rl, · line 658

Quoted 7 lines
src/openvpn/route.c +2 -0
@@ -653,6 +652,8 @@ init_route_list (struct route_list *rl,
                 rl->routes = new;
 	      }
 	  }
+	if (netlist)
+	  gc_addspecial(netlist, &gc_freeaddrinfo_callback, &gc);
       }
   }

I can see why we do this gc_addspecial() dance in socket.c (because the elements returned by getaddrinfo() are being pointed at, not copied).

This usage here seems unneeded, as the code will walk netlist, copy out the IPv4 addresses into a separate (gc-allocated) struct route_ipv4, and never use parts of netlist again - so freeaddrinfo() *should* work, and make the code slightly less complex.

Arne, this came from you in commit e719a053534 (and I ACKed it) - could you revisit this particular use and check whether it's truly needed?

thanks,

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#3 lore raw
Quoted 4 lines

I can see why we do this gc_addspecial() dance in socket.c (because the elements returned by getaddrinfo() are being pointed at, not copied).

Quoted 8 lines

This usage here seems unneeded, as the code will walk netlist, copy out the IPv4 addresses into a separate (gc-allocated) struct route_ipv4, and never use parts of netlist again - so freeaddrinfo() *should* work, and make the code slightly less complex.

Arne, this came from you in commit e719a053534 (and I ACKed it) - could you revisit this particular use and check whether it's truly needed?

I don't think it is really needed. It is more consistency and we try to use gc functions instead of free/malloc.

Arne

#4 lore raw

Hi,

Quoted 8 lines · Arne Schwabe
Quoted 8 lines

I can see why we do this gc_addspecial() dance in socket.c (because the elements returned by getaddrinfo() are being pointed at, not copied).

This usage here seems unneeded, as the code will walk netlist, copy out the IPv4 addresses into a separate (gc-allocated) struct route_ipv4, and never use parts of netlist again - so freeaddrinfo() *should* work, and make the code slightly less complex.

Quoted 5 lines
Quoted 2 lines

Arne, this came from you in commit e719a053534 (and I ACKed it) - could you revisit this particular use and check whether it's truly needed?

I don't think it is really needed. It is more consistency and we try to use gc functions instead of free/malloc.

Uh, I'm not sure I buy the "consistency" argument here - today we call freeaddrinfo() in 9 places (not counting buffer.h), and gc_addspecial() in 2 places... so I'd more call for "simplicity wins", that is, if there is no reason to delay calling freeaddrinfo(), we shouldn't do so - the memory in use is not related to a gc...

Do you agree?

(For the time being I have merged Steffan's patch to fix the memory leak, because it's a well-defined and correct change on its own - but I think this one still needs changing :-))

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#5 lore raw
In reply to Steffan Karger

Your patch has been applied to the master branch.

commit 3671bd185a914dc1d91c8a967e1c3a14dedacd32

Author: Steffan Karger
Date:   Fri Nov 6 08:42:39 2015 +0100
     Fix (potential) memory leak in init_route_list()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10443
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering