Browse code

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.

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 <davids@redhat.com>
Acked-by: Gert Doering <gert@greenie.muc.de>

David Sommerseth authored on 2012/02/06 08:30:47
Showing 6 changed files
... ...
@@ -54,11 +54,21 @@ alloc_buf_debug (size_t size, const char *file, int line)
54 54
 alloc_buf (size_t size)
55 55
 #endif
56 56
 {
57
+  struct buffer buf;
58
+
59
+  if (!buf_size_valid (size))
60
+    buf_size_error (size);
61
+  buf.capacity = (int)size;
62
+  buf.offset = 0;
63
+  buf.len = 0;
57 64
 #ifdef DMALLOC
58
-  return alloc_buf_gc_debug (size, NULL, file, line);
65
+  buf.data = openvpn_dmalloc (file, line, size);
59 66
 #else
60
-  return alloc_buf_gc (size, NULL);
67
+  buf.data = calloc (1, size);
61 68
 #endif
69
+  check_malloc_return(buf.data);
70
+
71
+  return buf;
62 72
 }
63 73
 
64 74
 struct buffer
... ...
@@ -515,11 +525,25 @@ string_alloc (const char *str, struct gc_arena *gc)
515 515
       const int n = strlen (str) + 1;
516 516
       char *ret;
517 517
 
518
+      if (gc) {
519
+#ifdef DMALLOC
520
+        ret = (char *) gc_malloc_debug (n, false, gc, file, line);
521
+#else
522
+        ret = (char *) gc_malloc (n, false, gc);
523
+#endif
524
+      } else {
525
+        /* If there are no garbage collector available, it's expected
526
+         * that the caller cleans up afterwards.  This is coherent with the
527
+         * earlier behaviour when gc_malloc() would be called with gc == NULL
528
+         */
518 529
 #ifdef DMALLOC
519
-      ret = (char *) gc_malloc_debug (n, false, gc, file, line);
530
+        ret = openvpn_dmalloc (file, line, n);
531
+        memset(ret, 0, n);
520 532
 #else
521
-      ret = (char *) gc_malloc (n, false, gc);
533
+        ret = calloc(1, n);
522 534
 #endif
535
+        check_malloc_return(ret);
536
+      }
523 537
       memcpy (ret, str, n);
524 538
       return ret;
525 539
     }
... ...
@@ -3012,7 +3012,7 @@ do_close_ifconfig_pool_persist (struct context *c)
3012 3012
 static void
3013 3013
 do_inherit_env (struct context *c, const struct env_set *src)
3014 3014
 {
3015
-  c->c2.es = env_set_create (NULL);
3015
+  c->c2.es = env_set_create (&c->c2.gc);
3016 3016
   c->c2.es_owned = true;
3017 3017
   env_set_inherit (c->c2.es, src);
3018 3018
 }
... ...
@@ -164,7 +164,7 @@ main (int argc, char *argv[])
164 164
 	  gc_init (&c.gc);
165 165
 
166 166
 	  /* initialize environmental variable store */
167
-	  c.es = env_set_create (NULL);
167
+	  c.es = env_set_create (&c.gc);
168 168
 #ifdef WIN32
169 169
 	  set_win_sys_path_via_env (c.es);
170 170
 #endif
... ...
@@ -4291,7 +4291,7 @@ add_option (struct options *options,
4291 4291
     {
4292 4292
       unsigned int netbits;
4293 4293
       char * ipv6_local;
4294
-	
4294
+
4295 4295
       VERIFY_PERMISSION (OPT_P_UP);
4296 4296
       if ( get_ipv6_addr( p[1], NULL, &netbits, &ipv6_local, msglevel ) &&
4297 4297
            ipv6_addr_safe( p[2] ) )
... ...
@@ -4301,6 +4301,11 @@ add_option (struct options *options,
4301 4301
 	      msg( msglevel, "ifconfig-ipv6: /netbits must be between 64 and 124, not '/%d'", netbits );
4302 4302
 	      goto err;
4303 4303
 	    }
4304
+
4305
+          if (options->ifconfig_ipv6_local)
4306
+            /* explicitly ignoring this is a const char */
4307
+            free ((char *) options->ifconfig_ipv6_local);
4308
+
4304 4309
 	  options->ifconfig_ipv6_local = ipv6_local;
4305 4310
 	  options->ifconfig_ipv6_netbits = netbits;
4306 4311
 	  options->ifconfig_ipv6_remote = p[2];
... ...
@@ -566,7 +566,7 @@ pf_init_context (struct context *c)
566 566
         if (plugin_call (c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
567 567
           {
568 568
             event_timeout_init (&c->c2.pf.reload, 1, now);
569
-            c->c2.pf.filename = string_alloc (pf_file, NULL);
569
+            c->c2.pf.filename = string_alloc (pf_file, &c->c2.gc);
570 570
             c->c2.pf.enabled = true;
571 571
 #ifdef ENABLE_DEBUG
572 572
             if (check_debug_level (D_PF_DEBUG))
... ...
@@ -83,6 +83,7 @@ set_common_name (struct tls_session *session, const char *common_name)
83 83
     }
84 84
   if (common_name)
85 85
     {
86
+      /* FIXME: Last alloc will never be freed */
86 87
       session->common_name = string_alloc (common_name, NULL);
87 88
 #ifdef ENABLE_PF
88 89
       {
... ...
@@ -703,6 +704,7 @@ man_def_auth_set_client_reason (struct tls_multi *multi, const char *client_reas
703 703
       multi->client_reason = NULL;
704 704
     }
705 705
   if (client_reason && strlen (client_reason))
706
+    /* FIXME: Last alloc will never be freed */
706 707
     multi->client_reason = string_alloc (client_reason, NULL);
707 708
 }
708 709