diff mbox series

[Openvpn-devel] Add calls to nvlist_destroy to avoid leaks

Message ID fd473a5552ce4f91a95abf8c8b91df4e@stormshield.eu
State Changes Requested
Headers show
Series [Openvpn-devel] Add calls to nvlist_destroy to avoid leaks | expand

Commit Message

Rémi FARAULT Oct. 30, 2024, 10:31 a.m. UTC

Comments

Gert Doering Nov. 4, 2024, 4:13 p.m. UTC | #1 
Hi,

so, I had the ACK from Kristof in GH#636 and was going to apply it,
but "this patch does not compile"

On Wed, Oct 30, 2024 at 10:31:57AM +0000, Rémi FARAULT via Openvpn-devel wrote:
>      if (remoteaddr)
>      {
> -        nvlist_add_nvlist(nvl, "remote", sockaddr_to_nvlist(remoteaddr));
> +        remote_nvl = sockaddr_to_nvlist(remoteaddr);
> +        nvlist_add_nvlist(nvl, "remote", remove_nvl);
>      }

There is an "remove" here, not "remote"...

Can you please fix & send a v2 ("git send-email -v2")?

While at it, please add all the stuff that's in the GH MR to the commit
message ;-)  - I'm happy to do that for "infrequent contributors", but
project rules disallow me to change actual code.  So when doing the
new version, you can just add:

----------------------------------------------------
    Add calls to nvlist_destroy to avoid leaks
    
    Some memory leaks were detected by valgrind on the openvpn daemon, using
    DCO mode on a FreeBSD platform.  The leaks are caused by missing
    nvlist_destroy calls in the file dco_freebsd.c.
    
    Calls to nvlist_destroy were added, sometimes using local variables to
    store nvlist pointers temporarly.  A valgrind run on the updated daemon
    confirmed that  the leaks were gone.
    
    Github: OpenVPN/openvpn#636
    Signed-off-by: R<A9>mi Farault <remi.farault@stormshield.eu>
----------------------------------------------------

(sorry for the name mangling, I do my best to make the commit UTF8
correct, but to/from e-mail tends to be... interesting)

gert
diff mbox series

Patch

From 67100f57634b1b59c2ec8c294fccabda8fb7d893 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Farault?= <remi.farault@stormshield.eu>
Date: Tue, 29 Oct 2024 12:06:35 +0100
Subject: [PATCH v1] Add calls to nvlist_destroy to avoid leaks

---
 src/openvpn/dco_freebsd.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index c92e42a1..55f0ab1d 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -78,7 +78,7 @@  dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd,
              struct in_addr *vpn_ipv4, struct in6_addr *vpn_ipv6)
 {
     struct ifdrv drv;
-    nvlist_t *nvl;
+    nvlist_t *nvl, *local_nvl, *remote_nvl;
     int ret;
 
     nvl = nvlist_create(0);
@@ -87,12 +87,14 @@  dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd,
 
     if (localaddr)
     {
-        nvlist_add_nvlist(nvl, "local", sockaddr_to_nvlist(localaddr));
+        local_nvl = sockaddr_to_nvlist(localaddr);
+        nvlist_add_nvlist(nvl, "local", local_nvl);
     }
 
     if (remoteaddr)
     {
-        nvlist_add_nvlist(nvl, "remote", sockaddr_to_nvlist(remoteaddr));
+        remote_nvl = sockaddr_to_nvlist(remoteaddr);
+        nvlist_add_nvlist(nvl, "remote", remove_nvl);
     }
 
     if (vpn_ipv4)
@@ -121,6 +123,14 @@  dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd,
     }
 
     free(drv.ifd_data);
+    if (localaddr)
+    {
+        nvlist_destroy(local_nvl);
+    }
+    if (remoteaddr)
+    {
+        nvlist_destroy(remote_nvl);
+    }
     nvlist_destroy(nvl);
 
     return ret;
@@ -418,7 +428,7 @@  dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid,
             const char *ciphername)
 {
     struct ifdrv drv;
-    nvlist_t *nvl;
+    nvlist_t *nvl, *encrypt_nvl, *decrypt_nvl;
     int ret;
 
     msg(D_DCO_DEBUG, "%s: slot %d, key-id %d, peer-id %d, cipher %s",
@@ -430,10 +440,11 @@  dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid,
     nvlist_add_number(nvl, "keyid", keyid);
     nvlist_add_number(nvl, "peerid", peerid);
 
-    nvlist_add_nvlist(nvl, "encrypt",
-                      key_to_nvlist(encrypt_key, encrypt_iv, ciphername));
-    nvlist_add_nvlist(nvl, "decrypt",
-                      key_to_nvlist(decrypt_key, decrypt_iv, ciphername));
+    encrypt_nvl = key_to_nvlist(encrypt_key, encrypt_iv, ciphername);
+    decrypt_nvl = key_to_nvlist(decrypt_key, decrypt_iv, ciphername);
+
+    nvlist_add_nvlist(nvl, "encrypt", encrypt_nvl);
+    nvlist_add_nvlist(nvl, "decrypt", decrypt_nvl);
 
     CLEAR(drv);
     snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
@@ -451,6 +462,8 @@  dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid,
     }
 
     free(drv.ifd_data);
+    nvlist_destroy(encrypt_nvl);
+    nvlist_destroy(decrypt_nvl);
     nvlist_destroy(nvl);
 
     return ret;
@@ -750,6 +763,7 @@  retry:
     if (!nvlist_exists_nvlist_array(nvl, "peers"))
     {
         /* no peers */
+        nvlist_destroy(nvl);
         return 0;
     }
 
@@ -762,6 +776,7 @@  retry:
         dco_update_peer_stat(m, peerid, nvlist_get_nvlist(peer, "bytes"));
     }
 
+    nvlist_destroy(nvl);
     return 0;
 }
 
-- 
2.25.1