Fix assert() situations where gc_malloc() is called without a gc_arena object

PATCH Superseded David Sommerseth 3 participants 4 messages Feb 7, 2012 – Feb 8, 2012

Share & tools

David Sommerseth

author

2012-02-07 10:58 UTC #0 lore raw

PATCH 1/1 Fix assert() situations where gc_malloc() is called without a gc_arena object

In commit bee92b479414d12035b0422f81ac5fcfe14fa645 the gc_malloc() was hardened to always require a gc_arena object for garbage collection. Some places in the code expected the old behaviour of a normal malloc() in these cases, that is a memory allocation without garbage collection.

This old behaviour is partly restored by allowing string_alloc() to do a non-gc based allocation if no gc_arena object is available. In addition some other places string_alloc() will now be called with a gc_arena pointer where such an object is available.

The alloc_buf() function has also been refactored to not use gc_malloc() at all.

Signed-off-by David Sommerseth <[email protected]...>

---
 buffer.c     |   25 ++++++++++++++++++++++---
 init.c       |    2 +-
 openvpn.c    |    2 +-
 options.c    |    2 ++
 pf.c         |    2 +-
 ssl_verify.c |    2 ++
 6 files changed, 29 insertions(+), 6 deletions(-)

buffer.c +22 -3

index c39bbcb..f9335f0 100644
@@ -54,11 +54,21 @@ alloc_buf_debug (size_t size, const char *file, int line)
 alloc_buf (size_t size)
 #endif
 {
+  struct buffer buf;
+
+  if (!buf_size_valid (size))
+    buf_size_error (size);
+  buf.capacity = (int)size;
+  buf.offset = 0;
+  buf.len = 0;
 #ifdef DMALLOC
-  return alloc_buf_gc_debug (size, NULL, file, line);
+  buf.data = openvpn_dmalloc (file, line, size);
 #else
-  return alloc_buf_gc (size, NULL);
+  buf.data = calloc (1, size);
 #endif
+  check_malloc_return(buf.data);
+
+  return buf;
 }
 
 struct buffer

@@ -518,7 +528,16 @@ string_alloc (const char *str, struct gc_arena *gc)
 #ifdef DMALLOC
       ret = (char *) gc_malloc_debug (n, false, gc, file, line);
 #else
-      ret = (char *) gc_malloc (n, false, gc);
+      if (gc) {
+        ret = (char *) gc_malloc (n, false, gc);
+      } else {
+        /* If there are no garbage collector available, it's expected
+         * that the caller cleans up afterwards.  This is coherent with the
+         * earlier behaviour when gc_malloc() would be called with gc == NULL
+         */
+        ret = calloc(1, n);
+        check_malloc_return(ret);
+      }
 #endif
       memcpy (ret, str, n);
       return ret;

init.c +1 -1

index 525f441..f0c3693 100644
@@ -3012,7 +3012,7 @@ do_close_ifconfig_pool_persist (struct context *c)
 static void
 do_inherit_env (struct context *c, const struct env_set *src)
 {
-  c->c2.es = env_set_create (NULL);
+  c->c2.es = env_set_create (&c->c2.gc);
   c->c2.es_owned = true;
   env_set_inherit (c->c2.es, src);
 }

openvpn.c +1 -1

index f5f2bce..84289d2 100644
@@ -164,7 +164,7 @@ main (int argc, char *argv[])
 	  gc_init (&c.gc);
 
 	  /* initialize environmental variable store */
-	  c.es = env_set_create (NULL);
+	  c.es = env_set_create (&c.gc);
 #ifdef WIN32
 	  set_win_sys_path_via_env (c.es);
 #endif

options.c +2 -0

index 6b8ae22..bec11e5 100644
@@ -4301,6 +4301,8 @@ add_option (struct options *options,
 	      msg( msglevel, "ifconfig-ipv6: /netbits must be between 64 and 124, not '/%d'", netbits );
 	      goto err;
 	    }
+          /* FIXME: Check if ipv6_local might miss a free().  It might leak
+           * via get_ipv6_addr() -> string_alloc() ? */
 	  options->ifconfig_ipv6_local = ipv6_local;
 	  options->ifconfig_ipv6_netbits = netbits;
 	  options->ifconfig_ipv6_remote = p[2];

pf.c +1 -1

index 6b4cba4..79915fa 100644
@@ -566,7 +566,7 @@ pf_init_context (struct context *c)
         if (plugin_call (c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
           {
             event_timeout_init (&c->c2.pf.reload, 1, now);
-            c->c2.pf.filename = string_alloc (pf_file, NULL);
+            c->c2.pf.filename = string_alloc (pf_file, &c->c2.gc);
             c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
             if (check_debug_level (D_PF_DEBUG))

ssl_verify.c +2 -0

index e45f149..37d4982 100644
@@ -83,6 +83,7 @@ set_common_name (struct tls_session *session, const char *common_name)
     }
   if (common_name)
     {
+      /* FIXME: Last alloc will never be freed */
       session->common_name = string_alloc (common_name, NULL);
 #ifdef ENABLE_PF
       {

@@ -703,6 +704,7 @@ man_def_auth_set_client_reason (struct tls_multi *multi, const char *client_reas
       multi->client_reason = NULL;
     }
   if (client_reason && strlen (client_reason))
+    /* FIXME: Last alloc will never be freed */
     multi->client_reason = string_alloc (client_reason, NULL);
 }

Signature · 1 line

1.7.4.4

David Sommerseth

author

2012-02-07 13:31 UTC #1 lore raw

PATCH v2 1/1 Fix assert() situations where gc_malloc() is called without a gc_arena object

The alloc_buf() function has also been refactored to not use gc_malloc() at all.

v2: - removes a memleak when --ifconfig-ipv6 is used several times
- makes string_alloc() behave properly if DMALLOC is enabled

Signed-off-by David Sommerseth <[email protected]...>

---
 buffer.c     |   32 ++++++++++++++++++++++++++++----
 init.c       |    2 +-
 openvpn.c    |    2 +-
 options.c    |    7 ++++++-
 pf.c         |    2 +-
 ssl_verify.c |    2 ++
 6 files changed, 39 insertions(+), 8 deletions(-)

buffer.c +28 -4

index c39bbcb..fca6a90 100644
@@ -54,11 +54,21 @@ alloc_buf_debug (size_t size, const char *file, int line)
 alloc_buf (size_t size)
 #endif
 {
+  struct buffer buf;
+
+  if (!buf_size_valid (size))
+    buf_size_error (size);
+  buf.capacity = (int)size;
+  buf.offset = 0;
+  buf.len = 0;
 #ifdef DMALLOC
-  return alloc_buf_gc_debug (size, NULL, file, line);
+  buf.data = openvpn_dmalloc (file, line, size);
 #else
-  return alloc_buf_gc (size, NULL);
+  buf.data = calloc (1, size);
 #endif
+  check_malloc_return(buf.data);
+
+  return buf;
 }
 
 struct buffer

@@ -515,11 +525,25 @@ string_alloc (const char *str, struct gc_arena *gc)
       const int n = strlen (str) + 1;
       char *ret;
 
+      if (gc) {
+#ifdef DMALLOC
+        ret = (char *) gc_malloc_debug (n, false, gc, file, line);
+#else
+        ret = (char *) gc_malloc (n, false, gc);
+#endif
+      } else {
+        /* If there are no garbage collector available, it's expected
+         * that the caller cleans up afterwards.  This is coherent with the
+         * earlier behaviour when gc_malloc() would be called with gc == NULL
+         */
 #ifdef DMALLOC
-      ret = (char *) gc_malloc_debug (n, false, gc, file, line);
+        ret = openvpn_dmalloc (file, line, n);
+        memset(ret, 0, n);
 #else
-      ret = (char *) gc_malloc (n, false, gc);
+        ret = calloc(1, n);
 #endif
+        check_malloc_return(ret);
+      }
       memcpy (ret, str, n);
       return ret;
     }

init.c +1 -1

index 525f441..f0c3693 100644
@@ -3012,7 +3012,7 @@ do_close_ifconfig_pool_persist (struct context *c)
 static void
 do_inherit_env (struct context *c, const struct env_set *src)
 {
-  c->c2.es = env_set_create (NULL);
+  c->c2.es = env_set_create (&c->c2.gc);
   c->c2.es_owned = true;
   env_set_inherit (c->c2.es, src);
 }

openvpn.c +1 -1

index f5f2bce..84289d2 100644
@@ -164,7 +164,7 @@ main (int argc, char *argv[])
 	  gc_init (&c.gc);
 
 	  /* initialize environmental variable store */
-	  c.es = env_set_create (NULL);
+	  c.es = env_set_create (&c.gc);
 #ifdef WIN32
 	  set_win_sys_path_via_env (c.es);
 #endif

options.c +6 -1

index 6b8ae22..a0b3431 100644
@@ -4291,7 +4291,7 @@ add_option (struct options *options,
     {
       unsigned int netbits;
       char * ipv6_local;
-	
+
       VERIFY_PERMISSION (OPT_P_UP);
       if ( get_ipv6_addr( p[1], NULL, &netbits, &ipv6_local, msglevel ) &&
            ipv6_addr_safe( p[2] ) )

@@ -4301,6 +4301,11 @@ add_option (struct options *options,
 	      msg( msglevel, "ifconfig-ipv6: /netbits must be between 64 and 124, not '/%d'", netbits );
 	      goto err;
 	    }
+
+          if (options->ifconfig_ipv6_local)
+            /* explicitly ignoring this is a const char */
+            free ((char *) options->ifconfig_ipv6_local);
+
 	  options->ifconfig_ipv6_local = ipv6_local;
 	  options->ifconfig_ipv6_netbits = netbits;
 	  options->ifconfig_ipv6_remote = p[2];

pf.c +1 -1

index 6b4cba4..79915fa 100644
@@ -566,7 +566,7 @@ pf_init_context (struct context *c)
         if (plugin_call (c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
           {
             event_timeout_init (&c->c2.pf.reload, 1, now);
-            c->c2.pf.filename = string_alloc (pf_file, NULL);
+            c->c2.pf.filename = string_alloc (pf_file, &c->c2.gc);
             c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
             if (check_debug_level (D_PF_DEBUG))

ssl_verify.c +2 -0

index e45f149..37d4982 100644
@@ -83,6 +83,7 @@ set_common_name (struct tls_session *session, const char *common_name)
     }
   if (common_name)
     {
+      /* FIXME: Last alloc will never be freed */
       session->common_name = string_alloc (common_name, NULL);
 #ifdef ENABLE_PF
       {

@@ -703,6 +704,7 @@ man_def_auth_set_client_reason (struct tls_multi *multi, const char *client_reas
       multi->client_reason = NULL;
     }
   if (client_reason && strlen (client_reason))
+    /* FIXME: Last alloc will never be freed */
     multi->client_reason = string_alloc (client_reason, NULL);
 }

Signature · 1 line

1.7.4.4

Samuli Seppänen

2012-02-08 13:00 UTC #2 lore raw

I give this one a feature-ACK as it seems to fix the buffer.c line 313 issue:

<http://thread.gmane.org/gmane.network.openvpn.devel/5346>

All the best,

Signature · 5 lines

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Quoted 166 lines

The alloc_buf() function has also been refactored to not use gc_malloc() at all.

v2: - removes a memleak when --ifconfig-ipv6 is used several times
- makes string_alloc() behave properly if DMALLOC is enabled

Signed-off-by David Sommerseth <[email protected]...>

---
 buffer.c     |   32 ++++++++++++++++++++++++++++----
 init.c       |    2 +-
 openvpn.c    |    2 +-
 options.c    |    7 ++++++-
 pf.c         |    2 +-
 ssl_verify.c |    2 ++
 6 files changed, 39 insertions(+), 8 deletions(-)

buffer.c +28 -4

index c39bbcb..fca6a90 100644
@@ -54,11 +54,21 @@ alloc_buf_debug (size_t size, const char *file, int line)
 alloc_buf (size_t size)
 #endif
 {
+  struct buffer buf;
+
+  if (!buf_size_valid (size))
+    buf_size_error (size);
+  buf.capacity = (int)size;
+  buf.offset = 0;
+  buf.len = 0;
 #ifdef DMALLOC
-  return alloc_buf_gc_debug (size, NULL, file, line);
+  buf.data = openvpn_dmalloc (file, line, size);
 #else
-  return alloc_buf_gc (size, NULL);
+  buf.data = calloc (1, size);
 #endif
+  check_malloc_return(buf.data);
+
+  return buf;
 }
 
 struct buffer

@@ -515,11 +525,25 @@ string_alloc (const char *str, struct gc_arena *gc)
       const int n = strlen (str) + 1;
       char *ret;
 
+      if (gc) {
+#ifdef DMALLOC
+        ret = (char *) gc_malloc_debug (n, false, gc, file, line);
+#else
+        ret = (char *) gc_malloc (n, false, gc);
+#endif
+      } else {
+        /* If there are no garbage collector available, it's expected
+         * that the caller cleans up afterwards.  This is coherent with the
+         * earlier behaviour when gc_malloc() would be called with gc == NULL
+         */
 #ifdef DMALLOC
-      ret = (char *) gc_malloc_debug (n, false, gc, file, line);
+        ret = openvpn_dmalloc (file, line, n);
+        memset(ret, 0, n);
 #else
-      ret = (char *) gc_malloc (n, false, gc);
+        ret = calloc(1, n);
 #endif
+        check_malloc_return(ret);
+      }
       memcpy (ret, str, n);
       return ret;
     }

init.c +1 -1

index 525f441..f0c3693 100644
@@ -3012,7 +3012,7 @@ do_close_ifconfig_pool_persist (struct context *c)
 static void
 do_inherit_env (struct context *c, const struct env_set *src)
 {
-  c->c2.es = env_set_create (NULL);
+  c->c2.es = env_set_create (&c->c2.gc);
   c->c2.es_owned = true;
   env_set_inherit (c->c2.es, src);
 }

openvpn.c +1 -1

index f5f2bce..84289d2 100644
@@ -164,7 +164,7 @@ main (int argc, char *argv[])
 	  gc_init (&c.gc);
 
 	  /* initialize environmental variable store */
-	  c.es = env_set_create (NULL);
+	  c.es = env_set_create (&c.gc);
 #ifdef WIN32
 	  set_win_sys_path_via_env (c.es);
 #endif

options.c +6 -1

index 6b8ae22..a0b3431 100644
@@ -4291,7 +4291,7 @@ add_option (struct options *options,
     {
       unsigned int netbits;
       char * ipv6_local;
-	
+
       VERIFY_PERMISSION (OPT_P_UP);
       if ( get_ipv6_addr( p[1], NULL, &netbits, &ipv6_local, msglevel ) &&
            ipv6_addr_safe( p[2] ) )

@@ -4301,6 +4301,11 @@ add_option (struct options *options,
 	      msg( msglevel, "ifconfig-ipv6: /netbits must be between 64 and 124, not '/%d'", netbits );
 	      goto err;
 	    }
+
+          if (options->ifconfig_ipv6_local)
+            /* explicitly ignoring this is a const char */
+            free ((char *) options->ifconfig_ipv6_local);
+
 	  options->ifconfig_ipv6_local = ipv6_local;
 	  options->ifconfig_ipv6_netbits = netbits;
 	  options->ifconfig_ipv6_remote = p[2];

pf.c +1 -1

index 6b4cba4..79915fa 100644
@@ -566,7 +566,7 @@ pf_init_context (struct context *c)
         if (plugin_call (c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
           {
             event_timeout_init (&c->c2.pf.reload, 1, now);
-            c->c2.pf.filename = string_alloc (pf_file, NULL);
+            c->c2.pf.filename = string_alloc (pf_file, &c->c2.gc);
             c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
             if (check_debug_level (D_PF_DEBUG))

ssl_verify.c +2 -0

index e45f149..37d4982 100644
@@ -83,6 +83,7 @@ set_common_name (struct tls_session *session, const char *common_name)
     }
   if (common_name)
     {
+      /* FIXME: Last alloc will never be freed */
       session->common_name = string_alloc (common_name, NULL);
 #ifdef ENABLE_PF
       {

@@ -703,6 +704,7 @@ man_def_auth_set_client_reason (struct tls_multi *multi, const char *client_reas
       multi->client_reason = NULL;
     }
   if (client_reason && strlen (client_reason))
+    /* FIXME: Last alloc will never be freed */
     multi->client_reason = string_alloc (client_reason, NULL);
 }

David Sommerseth

2012-02-08 16:28 UTC #3 lore raw

In reply to David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 14 lines · David Sommerseth

In commit bee92b479414d12035b0422f81ac5fcfe14fa645 the gc_malloc() was hardened to always require a gc_arena object for garbage collection. Some places in the code expected the old behaviour of a normal malloc() in these cases, that is a memory allocation without garbage collection.
This old behaviour is partly restored by allowing string_alloc() to do a non-gc based allocation if no gc_arena object is available. In addition some other places string_alloc() will now be called with a gc_arena pointer where such an object is available.
The alloc_buf() function has also been refactored to not use gc_malloc() at all.

Quoted 9 lines

v2: - removes a memleak when --ifconfig-ipv6 is used several times - makes string_alloc() behave properly if DMALLOC is enabled
Signed-off-by David Sommerseth <[email protected]...> --- buffer.c
| 32 ++++++++++++++++++++++++++++---- init.c       |    2 +-
openvpn.c |    2 +- options.c    |    7 ++++++- pf.c         |    2
+- ssl_verify.c |    2 ++ 6 files changed, 39 insertions(+), 8 
deletions(-)

Acked by Gert on #openvpn-devel. Applied to master and pushed out to -stable and -testing.

commit dc7be6d078ba106f9b0de12f3e879c3561c3c537

Author: David Sommerseth <[email protected]...>
Date:   Mon Feb 6 00:30:47 2012 +0100

Fix assert() situations where gc_malloc() is called without a
gc_arena object

    Signed-off-by: David Sommerseth <[email protected]...>
    Acked-by: Gert Doering <[email protected]...>

kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8yoqkACgkQDC186MBRfrpwUQCgqMy+nI3jvoIzxJF6j8tOgNRo gdcAnAn9q5OOK4p8z/OJDvRkOkxxRQJs
=kYaj
-----END PGP SIGNATURE-----