[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] [patch 1/5] iptables: fix invalid free



On Wed, Mar 21, 2007 at 10:31:41AM -0400, Daniel Veillard wrote:
> On Wed, Mar 21, 2007 at 01:55:35PM +0000, Mark McLoughlin wrote:
> >   - I think the use case is a little different - generally in libvirt, 
> >     we're only allocating very small chunks where the CPU hit for 
> >     initialisation would be negligible and would never show up on a
> >     profile. I'd prefer to take the minor hit of zero-initialising
> >     most/all memory for programming ease.
> > 
> >   - If our wrappers always zero-initialise, we don't need the 
> >     "initialise to -1 when debugging" thing.
> > 
> >   - If we rely on calloc() zero-initialising in our wrappers, we give 
> >     opportunity for libc to optimise where it knows the memory is 
> >     already initialised - e.g. where it's mmap()ing the memory 
> >     from /dev/zero
> 
>   okay, okay, let's use calloc() for libvirt, but then there is a number of
> places where I probably used memset() for zeroing, they should all be cleaned
> up.

  Patch enclosed, it also fixes a couple of places in virsh.c where 
malloc() and calloc() were called directly instead of using the virsh
checking functions.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/
Index: qemud/iptables.c
===================================================================
RCS file: /data/cvs/libxen/qemud/iptables.c,v
retrieving revision 1.4
diff -u -p -r1.4 iptables.c
--- qemud/iptables.c	13 Mar 2007 22:43:22 -0000	1.4
+++ qemud/iptables.c	22 Mar 2007 16:57:11 -0000
@@ -270,11 +270,9 @@ iptRulesNew(const char *table,
 {
     iptRules *rules;
 
-    if (!(rules = (iptRules *)malloc(sizeof (iptRules))))
+    if (!(rules = (iptRules *)calloc(1, sizeof (iptRules))))
         return NULL;
 
-    memset (rules, 0, sizeof (iptRules));
-
     if (!(rules->table = strdup(table)))
         goto error;
 
@@ -353,11 +351,9 @@ iptablesAddRemoveChain(iptRules *rules, 
         2 + /*   --table foo     */
         2;  /*   --new-chain bar */
 
-    if (!(argv = (char **)malloc(sizeof(char *) * (n+1))))
+    if (!(argv = (char **)calloc(1, sizeof(char *) * (n + 1))))
         goto error;
 
-    memset(argv, 0, sizeof(char *) * (n + 1));
-
     n = 0;
 
     if (!(argv[n++] = strdup(IPTABLES_PATH)))
@@ -413,14 +409,12 @@ iptablesAddRemoveRule(iptRules *rules, i
 
     va_end(args);
 
-    if (!(argv = (char **)malloc(sizeof(char *) * (n + 1))))
+    if (!(argv = (char **)calloc(1, sizeof(char *) * (n + 1))))
         goto error;
 
     if (!(rule = (char *)malloc(rulelen)))
         goto error;
 
-    memset(argv, 0, sizeof(char *) * (n + 1));
-
     n = 0;
 
     if (!(argv[n++] = strdup(IPTABLES_PATH)))
Index: qemud/qemud.c
===================================================================
RCS file: /data/cvs/libxen/qemud/qemud.c,v
retrieving revision 1.33
diff -u -p -r1.33 qemud.c
--- qemud/qemud.c	13 Mar 2007 22:43:22 -0000	1.33
+++ qemud/qemud.c	22 Mar 2007 16:57:11 -0000
@@ -1132,11 +1132,9 @@ qemudBuildDnsmasqArgv(struct qemud_serve
         (2 * network->def->nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */
         1;  /* NULL */
 
-    if (!(*argv = malloc(len * sizeof(char *))))
+    if (!(*argv = calloc(1, len * sizeof(char *))))
         goto no_memory;
 
-    memset(*argv, 0, len * sizeof(char *));
-
 #define APPEND_ARG(v, n, s) do {     \
         if (!((v)[(n)] = strdup(s))) \
             goto no_memory;          \
Index: src/conf.c
===================================================================
RCS file: /data/cvs/libxen/src/conf.c,v
retrieving revision 1.8
diff -u -p -r1.8 conf.c
--- src/conf.c	9 Mar 2007 20:47:12 -0000	1.8
+++ src/conf.c	22 Mar 2007 16:57:11 -0000
@@ -151,13 +151,11 @@ __virConfNew(void)
 {
     virConfPtr ret;
 
-    ret = (virConfPtr) malloc(sizeof(virConf));
+    ret = (virConfPtr) calloc(1, sizeof(virConf));
     if (ret == NULL) {
         virConfError(NULL, VIR_ERR_NO_MEMORY, _("allocating configuration"), 0);
         return(NULL);
     }
-    memset(ret, 0, sizeof(virConf));
-
     ret->filename = NULL;
 
     return(ret);
@@ -202,13 +200,12 @@ virConfAddEntry(virConfPtr conf, char *n
     if ((comm == NULL) && (name == NULL))
         return(NULL);
     
-    ret = (virConfEntryPtr) malloc(sizeof(virConfEntry));
+    ret = (virConfEntryPtr) calloc(1, sizeof(virConfEntry));
     if (ret == NULL) {
         virConfError(NULL, VIR_ERR_NO_MEMORY, _("allocating configuration"), 0);
         return(NULL);
     }
 
-    memset(ret, 0, sizeof(virConfEntry));
     ret->name = name;
     ret->value = value;
     ret->comment = comm;
@@ -486,14 +483,13 @@ virConfParseValue(virConfParserCtxtPtr c
 	             ctxt->line);
 	return(NULL);
     }
-    ret = (virConfValuePtr) malloc(sizeof(virConfValue));
+    ret = (virConfValuePtr) calloc(1, sizeof(virConfValue));
     if (ret == NULL) {
         virConfError(NULL, VIR_ERR_NO_MEMORY, _("allocating configuration"), 0);
 	if (str != NULL)
 	    free(str);
         return(NULL);
     }
-    memset(ret, 0, sizeof(virConfValue));
     ret->type = type;
     ret->l = l;
     ret->str = str;
Index: src/hash.c
===================================================================
RCS file: /data/cvs/libxen/src/hash.c,v
retrieving revision 1.18
diff -u -p -r1.18 hash.c
--- src/hash.c	6 Mar 2007 22:06:14 -0000	1.18
+++ src/hash.c	22 Mar 2007 16:57:11 -0000
@@ -89,9 +89,8 @@ virHashCreate(int size)
     if (table) {
         table->size = size;
         table->nbElems = 0;
-        table->table = malloc(size * sizeof(virHashEntry));
+        table->table = calloc(1, size * sizeof(virHashEntry));
         if (table->table) {
-            memset(table->table, 0, size * sizeof(virHashEntry));
             return (table);
         }
         free(table);
@@ -132,12 +131,11 @@ virHashGrow(virHashTablePtr table, int s
     if (oldtable == NULL)
         return (-1);
 
-    table->table = malloc(size * sizeof(virHashEntry));
+    table->table = calloc(1, size * sizeof(virHashEntry));
     if (table->table == NULL) {
         table->table = oldtable;
         return (-1);
     }
-    memset(table->table, 0, size * sizeof(virHashEntry));
     table->size = size;
 
     /*  If the two loops are merged, there would be situations where
@@ -661,12 +659,11 @@ virConnectPtr
 virGetConnect(void) {
     virConnectPtr ret;
 
-    ret = (virConnectPtr) malloc(sizeof(virConnect));
+    ret = (virConnectPtr) calloc(1, sizeof(virConnect));
     if (ret == NULL) {
         virHashError(NULL, VIR_ERR_NO_MEMORY, _("allocating connection"));
         goto failed;
     }
-    memset(ret, 0, sizeof(virConnect));
     ret->magic = VIR_CONNECT_MAGIC;
     ret->nb_drivers = 0;
     ret->handle = -1;
@@ -767,12 +764,11 @@ virGetDomain(virConnectPtr conn, const c
     /*
      * not found, allocate a new one
      */
-    ret = (virDomainPtr) malloc(sizeof(virDomain));
+    ret = (virDomainPtr) calloc(1, sizeof(virDomain));
     if (ret == NULL) {
         virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
 	goto error;
     }
-    memset(ret, 0, sizeof(virDomain));
     ret->name = strdup(name);
     if (ret->name == NULL) {
         virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
@@ -950,12 +946,11 @@ virGetNetwork(virConnectPtr conn, const 
     /*
      * not found, allocate a new one
      */
-    ret = (virNetworkPtr) malloc(sizeof(virNetwork));
+    ret = (virNetworkPtr) calloc(1, sizeof(virNetwork));
     if (ret == NULL) {
         virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
 	goto error;
     }
-    memset(ret, 0, sizeof(virNetwork));
     ret->name = strdup(name);
     if (ret->name == NULL) {
         virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
Index: src/libvirt.c
===================================================================
RCS file: /data/cvs/libxen/src/libvirt.c,v
retrieving revision 1.64
diff -u -p -r1.64 libvirt.c
--- src/libvirt.c	19 Mar 2007 10:15:52 -0000	1.64
+++ src/libvirt.c	22 Mar 2007 16:57:11 -0000
@@ -40,7 +40,6 @@
  * TODO:
  * - use lock to protect against concurrent accesses ?
  * - use reference counting to garantee coherent pointer state ?
- * - memory wrappers for malloc/free ?
  */
 
 static virDriverPtr virDriverTab[MAX_DRIVERS];
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libxen/src/virsh.c,v
retrieving revision 1.68
diff -u -p -r1.68 virsh.c
--- src/virsh.c	22 Mar 2007 10:27:55 -0000	1.68
+++ src/virsh.c	22 Mar 2007 16:57:11 -0000
@@ -1247,9 +1247,9 @@ cmdVcpuinfo(vshControl * ctl, vshCmd * c
         return FALSE;
     }
 
-    cpuinfo = malloc(sizeof(virVcpuInfo)*info.nrVirtCpu);
+    cpuinfo = vshMalloc(ctl, sizeof(virVcpuInfo)*info.nrVirtCpu);
     cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
-    cpumap = malloc(info.nrVirtCpu * cpumaplen);
+    cpumap = vshMalloc(ctl, info.nrVirtCpu * cpumaplen);
 
     if ((ncpus = virDomainGetVcpus(dom,
                                    cpuinfo, info.nrVirtCpu,
@@ -1350,8 +1350,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm
     }
 
     cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
-    cpumap = malloc(cpumaplen);
-    memset(cpumap, 0, cpumaplen);
+    cpumap = vshCalloc(ctl, 1, cpumaplen);
 
     do {
         unsigned int cpu = atoi(cpulist);
Index: src/xm_internal.c
===================================================================
RCS file: /data/cvs/libxen/src/xm_internal.c,v
retrieving revision 1.21
diff -u -p -r1.21 xm_internal.c
--- src/xm_internal.c	16 Mar 2007 15:03:21 -0000	1.21
+++ src/xm_internal.c	22 Mar 2007 16:57:11 -0000
@@ -2113,11 +2113,10 @@ virDomainPtr xenXMDomainDefineXML(virCon
         goto error;
     }
 
-    if (!(entry = malloc(sizeof(xenXMConfCache)))) {
+    if (!(entry = calloc(1, sizeof(xenXMConfCache)))) {
         xenXMError(conn, VIR_ERR_NO_MEMORY, "config");
         goto error;
     }
-    memset(entry, 0, sizeof(xenXMConfCache));
 
     if ((entry->refreshedAt = time(NULL)) == ((time_t)-1)) {
         xenXMError(conn, VIR_ERR_INTERNAL_ERROR, "unable to get current time");

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]