2:29 p.m.
After updating the virBuffer APIs to protect against improper usage I have
been thinking about how we might provider safer memory allocation APIs
with protection against common usage errors and compile time validation of
checks for failure.
The standard C library malloc/realloc/calloc/free APIs have just about the
worst possible design, positively encouraging serious application coding
errors. There are at least 7 common problems I can think of at the moment,
perhaps more....
1. malloc() - pass incorrect number of bytes
2. malloc() - fail to check the return value
3. malloc() - use uninitialized memory
4. free() - double free by not setting pointer to NULL
5. realloc() - fail to check the return value
6. realloc() - fail to re-assign the pointer with new address
7. realloc() - accidental overwrite of original pointer with NULL on
failure, leaking memory
Many applications / libraries wrap these in their own functions, but typically
fail to address one or more these problems.
eg glib re-definitions try to address point 2 by having g_malloc() call
abort() on failure, but then re-introduce the problem by adding g_try_malloc()
gpointer g_malloc (gulong n_bytes) G_GNUC_MALLOC;
gpointer g_try_malloc (gulong n_bytes) G_GNUC_MALLOC;
Simiarly it tries address point 6, with the annotation, but this still leaves
open the failure to check for NULL in point 1.
gpointer g_realloc (gpointer mem,
gulong n_bytes) G_GNUC_WARN_UNUSED_RESULT;
gpointer g_try_realloc (gpointer mem,
gulong n_bytes) G_GNUC_WARN_UNUSED_RESULT;
And the g_free wrapper doesn't help with the double free issue at all:
void g_free (gpointer mem);
All 7 of the errors listed early could be avoided and/or checked at compile
time if the APIs used a different calling convention.
1. For any given pointer the compiler knows how many bytes need to be
allocated for a single instance of it. ie sizeof(*(ptr)). Since C
has no data type representing a data type, this cannot be done with
a simple function - it needs a (trivial) macro.
2. The __attribute__((__warn_unused_result__)) annotation causes the
compiler to warn if an application doesn't do something with a return
value. With the standard malloc() API this doesn't help because you
always assign the return value to a pointer. The core problem here
is that the API encodes the error condition as a special case of
the actual data returned. This can be addressed by separating the
two. The return value should simply be bi-state success or fail code
and the return data passed back via an pointer to a pointer parameter.
3. The memory allocated by malloc() is not initialized to zero. For
that a separate calloc() function is provided. This leaves open the
possiblity of an app mistakenly using the wrong variant. Or consider
an app using malloc() and explicitly initializing all struct members.
Some time later a new member is added and now the developer needs to
find all malloc() calls wrt to that struct and explicitly initilize
the new member. It would be safer to always have malloc zero all
memory, even though it has a small performance impact, the net win
in safety is probably worth it.
4. Since free() takes the pointer to be freed directly it cannot reset
the caller's original pointer back to NULL. This can be addressed if
a pointer to the pointer is passed instead. The computational cost
of the often redundant assignment to NULL is negligable given the
safety benefit
5, 6 & 7. As with problem 2, these problems are all caused by the fact
that the error condition is special case of the return data value.
The impact of these is typically far worse because it often results
in a buffer overflow and the complex rules for calling realloc() are
hard to remember.
So the core requirements of a safer API for allocation are:
- Use return values *only* for the success/fail error condition
- Annotate all the return values with __warn_unused_result__
- Pass a pointer to the pointer into all functions
- Define macros around all functions to automatically do the sizeof(*(ptr))
Passing a pointer to a pointer gets a little tedious because of the need
to write '&(foo)' instead of just 'foo', and is actually introduces a
new problem of the caller accidentally passing merely a pointer, instead
of a pointer to a pointer. This is a minor problem though because it will
be identified on the very first run of the code. In addition, since we're
defining macros around every function we can have the macro also convert
from ptr to &(ptr) for us, avoiding this potential error:
So the primary application usage would be via a set of macros:
VIR_ALLOC(ptr)
VIR_ALLOC_N(ptr, count)
VIR_REALLOC(ptr)
VIR_REALLOC_N(ptr, count)
VIR_FREE(ptr)
These call through to the underlying APIs:
int virAlloc(void *, size_t bytes)
int virAllocN(void *, size_t bytes, size_t count)
int virRealloc(void *, size_t bytes)
int virReallocN(void *, size_t bytes, size_t count)
int virFree(void *)
Note that although we're passing a pointer to a pointer into all these, the
first param is still just 'void *' and not 'void **'. This is because
'void *'
is defined to hold any type of pointer, and using 'void **' causes gcc to
complain bitterly about strict aliasing violations. Internally the impls of
these methods will safely cast to 'void **' when deferencing the pointer.
All 4 of Alloc/Realloc functions will have __warn_unused_result__ annotation
so the caller is forced to check the return value for failure, validated at
compile time generating a warning (or fatal compile error with -Werror).
So to wire up the macros to the APIs:
#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
#define VIR_REALLOC(ptr) virRealloc(&(ptr), sizeof(*(ptr)))
#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count))
#define VIR_FREE(ptr) virFree(&(ptr))
In particular notice how we're using the compiler to decide how many bytes
to allocate for each variable here, thus guarenteeing the correct size.
Finally here's an illustration of how these APis would be used in practice.
- Allocating a new variable:
virDomainPtr domain;
if (VIR_ALLOC(domain) < 0) {
__virRaiseError(VIR_ERROR_NO_MEMORY)
return NULL;
}
Looking at the macro, this pass '&domain' into virAlloc() which initalizes
it with address of memory sizeof(*domain) bytes long.
- Allocating an array of domains
virDomainPtr domains;
int ndomains = 10;
if (VIR_ALLOC_N(domains, ndomains) < 0) {
__virRaiseError(VIR_ERROR_NO_MEMORY)
return NULL;
}
- Allocating an array of domain pointers
virDomainPtr *domains;
int ndomains = 10;
if (VIR_ALLOC_N(domains, ndomains) < 0) {
__virRaiseError(VIR_ERROR_NO_MEMORY)
return NULL;
}
- Re-allocate the array of domains to be longer
ndomains = 20
if (VIR_REALLOC_N(domains, ndomains) < 0) {
__virRaiseError(VIR_ERROR_NO_MEMORY)
return NULL;
}
- Free the domain
VIR_FREE(domain);
The attached patch implements these ideas in a memory.h and memory.c file
and updates 'hash.c' to illustrate their usage. If we were to replace
every single instance of malloc/calloc/free/realloc with these calls the
automated builds which run with -Werror would quickly tell us of any places
where we forget to check memory allocations for failure, and we'd make
code like from capabilities.c:
if ((guest->arch.defaultInfo.machines =
calloc(nmachines,
sizeof(*guest->arch.defaultInfo.machines))) == NULL)
return -1;
char **migrateTrans;
if ((migrateTrans = realloc(caps->host.migrateTrans,
sizeof(*migrateTrans) *
(caps->host.nmigrateTrans+1))) == NULL)
return -1;
caps->host.migrateTrans = migrateTrans;
Much less ugly:
if (VIR_ALLOC_N(guest->arch.defaultInfo.machines, nmachines) < 0)
return -1;
if (VIR_REALLOC(migrateTrans, caps->host.nmigrateTrans+1) < 0)
return -1;
So we've addressed all 7 common error scenarios with malloc/free/realloc/caller
and produced easier to read code too. The cleaner realloc() API is particularly
appealing.
This does all seem a little bit too good to be true - I'm wondering why other
apps/libraries don't use this style of allocation functions already ? Perhaps
someone can find nasty flaws in this approach, but hopefuly not...
hash.c | 113 ++++++++++++++++++++++--------------------------
memory.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
memory.h | 95 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 294 insertions(+), 60 deletions(-)
Regards,
Daniel.
Index: memory.h
===================================================================
RCS file: memory.h
diff -N memory.h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ memory.h 27 Apr 2008 19:04:28 -0000
@@ -0,0 +1,95 @@
+/*
+ * memory.c: safer memory allocation
+ *
+ * Copyright (C) 2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+
+#ifndef __VIR_MEMORY_H_
+#define __VIR_MEMORY_H_
+
+#include "internal.h"
+
+/* Don't call these directly - use the macros below */
+int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK;
+int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
+int virRealloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK;
+int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
+void virFree(void *ptrptr);
+
+
+/**
+ * VIR_ALLOC:
+ * @ptr: pointer to hold address of allocated memory
+ *
+ * Allocate sizeof(*ptr) bytes of memory and store
+ * the address of allocated memory in 'ptr'. Fill the
+ * newly allocated memory with zeros.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
+
+/**
+ * VIR_ALLOC_N:
+ * @ptr: pointer to hold address of allocated memory
+ * @count: number of elements to allocate
+ *
+ * Allocate an array of 'count' elements, each sizeof(*ptr)
+ * bytes long and store the address of allocated memory in
+ * 'ptr'. Fill the newly allocated memory with zeros.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
+
+/**
+ * VIR_REALLOC:
+ * @ptr: pointer to hold address of allocated memory
+ *
+ * Re-allocate to sizeof(*ptr) bytes of memory and store
+ * the address of allocated memory in 'ptr'. Fill the
+ * newly allocated memory with zeros
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_REALLOC(ptr) virRealloc(&(ptr), sizeof(*(ptr)))
+
+/**
+ * VIR_REALLOC_N:
+ * @ptr: pointer to hold address of allocated memory
+ * @count: number of elements to allocate
+ *
+ * Re-allocate an array of 'count' elements, each sizeof(*ptr)
+ * bytes long and store the address of allocated memory in
+ * 'ptr'. Fill the newly allocated memory with zeros
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count))
+
+/**
+ * VIR_FREE:
+ * @ptr: pointer holding address to be freed
+ *
+ * Free the memory stored in 'ptr' and update to point
+ * to NULL.
+ */
+#define VIR_FREE(ptr) virFree(&(ptr))
+
+#endif /* __VIR_MEMORY_H_ */
Index: memory.c
===================================================================
RCS file: memory.c
diff -N memory.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ memory.c 27 Apr 2008 19:04:28 -0000
@@ -0,0 +1,146 @@
+/*
+ * memory.c: safer memory allocation
+ *
+ * Copyright (C) 2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <stdlib.h>
+
+#include "memory.h"
+
+/**
+ * virAlloc:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ *
+ * Allocate 'size' bytes of memory. Return the address of the
+ * allocated memory in 'ptrptr'. The newly allocated memory is
+ * filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virAlloc(void *ptrptr, size_t size)
+{
+ if (size == 0) {
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+
+ *(void **)ptrptr = calloc(1, size);
+ if (*(void **)ptrptr == NULL)
+ return -1;
+ return 0;
+}
+
+/**
+ * virAllocN:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ * @count: number of elements to allocate
+ *
+ * Allocate an array of memory 'count' elements long,
+ * each with 'size' bytes. Return the address of the
+ * allocated memory in 'ptrptr'. The newly allocated
+ * memory is filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virAllocN(void *ptrptr, size_t size, size_t count)
+{
+ if (size == 0 || count == 0) {
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+
+ *(void**)ptrptr = calloc(count, size);
+ if (*(void**)ptrptr == NULL)
+ return -1;
+ return 0;
+}
+
+/**
+ * virRealloc:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ *
+ * Resize the block of memory in 'ptrptr' to be 'size' bytes
+ * in length. Update 'ptrptr' with the address of the newly
+ * allocated memory. On failure, 'ptrptr' is not changed and
+ * still points to the original memory block. The newly
+ * allocated memory is filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virRealloc(void *ptrptr, size_t size)
+{
+ void *tmp;
+ if (size == 0) {
+ free(*(void **)ptrptr);
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+
+ tmp = realloc(*(void**)ptrptr, size);
+ if (!tmp)
+ return -1;
+ *(void**)ptrptr = tmp;
+ return 0;
+}
+
+/**
+ * virReallocN:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ * @count: number of elements in array
+ *
+ * Resize the block of memory in 'ptrptr' to be an array of
+ * 'count' elements, each 'size' bytes in length. Update
'ptrptr'
+ * with the address of the newly allocated memory. On failure,
+ * 'ptrptr' is not changed and still points to the original memory
+ * block. The newly allocated memory is filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virReallocN(void *ptrptr, size_t size, size_t count)
+{
+ void *tmp;
+ if (size == 0 || count == 0) {
+ free(*(void **)ptrptr);
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+ tmp = realloc(*(void**)ptrptr, size * count);
+ if (!tmp)
+ return -1;
+ *(void**)ptrptr = tmp;
+ return 0;
+}
+
+/**
+ * virFree:
+ * @ptrptr: pointer to pointer for address of memory to be freed
+ *
+ * Release the chunk of memory in the pointer pointed to by
+ * the 'ptrptr' variable. After release, 'ptrptr' will be
+ * updated to point to NULL.
+ */
+void virFree(void *ptrptr)
+{
+ free(*(void**)ptrptr);
+ *(void**)ptrptr = NULL;
+}
Index: hash.c
===================================================================
RCS file: /data/cvs/libvirt/src/hash.c,v
retrieving revision 1.37
diff -u -p -r1.37 hash.c
--- hash.c 18 Apr 2008 08:33:23 -0000 1.37
+++ hash.c 27 Apr 2008 19:04:28 -0000
@@ -25,6 +25,7 @@
#include <libxml/threads.h>
#include "internal.h"
#include "hash.h"
+#include "memory.h"
#define MAX_HASH_LEN 8
@@ -85,22 +86,22 @@ virHashComputeKey(virHashTablePtr table,
virHashTablePtr
virHashCreate(int size)
{
- virHashTablePtr table;
+ virHashTablePtr table = NULL;
if (size <= 0)
size = 256;
- table = malloc(sizeof(*table));
- if (table) {
- table->size = size;
- table->nbElems = 0;
- table->table = calloc(1, size * sizeof(*(table->table)));
- if (table->table) {
- return (table);
- }
- free(table);
+ if (VIR_ALLOC(table) < 0)
+ return NULL;
+
+ table->size = size;
+ table->nbElems = 0;
+ if (VIR_ALLOC_N(table->table, size) < 0) {
+ VIR_FREE(table);
+ return NULL;
}
- return (NULL);
+
+ return table;
}
/**
@@ -136,8 +137,7 @@ virHashGrow(virHashTablePtr table, int s
if (oldtable == NULL)
return (-1);
- table->table = calloc(1, size * sizeof(*(table->table)));
- if (table->table == NULL) {
+ if (VIR_ALLOC_N(table->table, size) < 0) {
table->table = oldtable;
return (-1);
}
@@ -170,7 +170,7 @@ virHashGrow(virHashTablePtr table, int s
if (table->table[key].valid == 0) {
memcpy(&(table->table[key]), iter, sizeof(virHashEntry));
table->table[key].next = NULL;
- free(iter);
+ VIR_FREE(iter);
} else {
iter->next = table->table[key].next;
table->table[key].next = iter;
@@ -184,7 +184,7 @@ virHashGrow(virHashTablePtr table, int s
}
}
- free(oldtable);
+ VIR_FREE(oldtable);
#ifdef DEBUG_GROW
xmlGenericError(xmlGenericErrorContext,
@@ -225,19 +225,19 @@ virHashFree(virHashTablePtr table, virHa
next = iter->next;
if ((f != NULL) && (iter->payload != NULL))
f(iter->payload, iter->name);
- free(iter->name);
+ VIR_FREE(iter->name);
iter->payload = NULL;
if (!inside_table)
- free(iter);
+ VIR_FREE(iter);
nbElems--;
inside_table = 0;
iter = next;
}
inside_table = 0;
}
- free(table->table);
+ VIR_FREE(table->table);
}
- free(table);
+ VIR_FREE(table);
}
/**
@@ -281,8 +281,7 @@ virHashAddEntry(virHashTablePtr table, c
if (insert == NULL) {
entry = &(table->table[key]);
} else {
- entry = malloc(sizeof(*entry));
- if (entry == NULL)
+ if (VIR_ALLOC(entry) < 0)
return (-1);
}
@@ -354,8 +353,7 @@ virHashUpdateEntry(virHashTablePtr table
if (insert == NULL) {
entry = &(table->table[key]);
} else {
- entry = malloc(sizeof(*entry));
- if (entry == NULL)
+ if (VIR_ALLOC(entry) < 0)
return (-1);
}
@@ -451,10 +449,10 @@ virHashRemoveEntry(virHashTablePtr table
if ((f != NULL) && (entry->payload != NULL))
f(entry->payload, entry->name);
entry->payload = NULL;
- free(entry->name);
+ VIR_FREE(entry->name);
if (prev) {
prev->next = entry->next;
- free(entry);
+ VIR_FREE(entry);
} else {
if (entry->next == NULL) {
entry->valid = 0;
@@ -462,7 +460,7 @@ virHashRemoveEntry(virHashTablePtr table
entry = entry->next;
memcpy(&(table->table[key]), entry,
sizeof(virHashEntry));
- free(entry);
+ VIR_FREE(entry);
}
}
table->nbElems--;
@@ -535,11 +533,11 @@ int virHashRemoveSet(virHashTablePtr tab
if (iter(entry->payload, entry->name, data)) {
count++;
f(entry->payload, entry->name);
- free(entry->name);
+ VIR_FREE(entry->name);
table->nbElems--;
if (prev) {
prev->next = entry->next;
- free(entry);
+ VIR_FREE(entry);
entry = prev;
} else {
if (entry->next == NULL) {
@@ -549,7 +547,7 @@ int virHashRemoveSet(virHashTablePtr tab
entry = entry->next;
memcpy(&(table->table[i]), entry,
sizeof(virHashEntry));
- free(entry);
+ VIR_FREE(entry);
entry = &(table->table[i]);
continue;
}
@@ -689,8 +687,7 @@ virConnectPtr
virGetConnect(void) {
virConnectPtr ret;
- ret = calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(NULL, VIR_ERR_NO_MEMORY, _("allocating connection"));
goto failed;
}
@@ -729,7 +726,7 @@ failed:
virHashFree(ret->storageVols, (virHashDeallocator)
virStorageVolFreeName);
pthread_mutex_destroy(&ret->lock);
- free(ret);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -759,11 +756,11 @@ virReleaseConnect(virConnectPtr conn) {
if (__lastErr.conn == conn)
__lastErr.conn = NULL;
- free(conn->name);
+ VIR_FREE(conn->name);
pthread_mutex_unlock(&conn->lock);
pthread_mutex_destroy(&conn->lock);
- free(conn);
+ VIR_FREE(conn);
}
/**
@@ -824,8 +821,7 @@ __virGetDomain(virConnectPtr conn, const
ret = (virDomainPtr) virHashLookup(conn->domains, name);
/* TODO check the UUID */
if (ret == NULL) {
- ret = (virDomainPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
goto error;
}
@@ -854,8 +850,8 @@ __virGetDomain(virConnectPtr conn, const
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name );
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -888,8 +884,8 @@ virReleaseDomain(virDomainPtr domain) {
__lastErr.dom = NULL;
domain->magic = -1;
domain->id = -1;
- free(domain->name);
- free(domain);
+ VIR_FREE(domain->name);
+ VIR_FREE(domain);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
@@ -962,8 +958,7 @@ __virGetNetwork(virConnectPtr conn, cons
ret = (virNetworkPtr) virHashLookup(conn->networks, name);
/* TODO check the UUID */
if (ret == NULL) {
- ret = (virNetworkPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
goto error;
}
@@ -991,8 +986,8 @@ __virGetNetwork(virConnectPtr conn, cons
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name );
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -1025,8 +1020,8 @@ virReleaseNetwork(virNetworkPtr network)
__lastErr.net = NULL;
network->magic = -1;
- free(network->name);
- free(network);
+ VIR_FREE(network->name);
+ VIR_FREE(network);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
@@ -1100,8 +1095,7 @@ __virGetStoragePool(virConnectPtr conn,
ret = (virStoragePoolPtr) virHashLookup(conn->storagePools, name);
/* TODO check the UUID */
if (ret == NULL) {
- ret = (virStoragePoolPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating storage
pool"));
goto error;
}
@@ -1129,8 +1123,8 @@ __virGetStoragePool(virConnectPtr conn,
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name);
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -1159,8 +1153,8 @@ virReleaseStoragePool(virStoragePoolPtr
_("pool missing from connection hash table"));
pool->magic = -1;
- free(pool->name);
- free(pool);
+ VIR_FREE(pool->name);
+ VIR_FREE(pool);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
@@ -1232,8 +1226,7 @@ __virGetStorageVol(virConnectPtr conn, c
ret = (virStorageVolPtr) virHashLookup(conn->storageVols, key);
if (ret == NULL) {
- ret = (virStorageVolPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating storage
vol"));
goto error;
}
@@ -1266,9 +1259,9 @@ __virGetStorageVol(virConnectPtr conn, c
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name);
- free(ret->pool);
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret->pool);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -1297,9 +1290,9 @@ virReleaseStorageVol(virStorageVolPtr vo
_("vol missing from connection hash table"));
vol->magic = -1;
- free(vol->name);
- free(vol->pool);
- free(vol);
+ VIR_FREE(vol->name);
+ VIR_FREE(vol->pool);
+ VIR_FREE(vol);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2:39 a.m.
On Sun, Apr 27, 2008 at 08:29:33PM +0100, Daniel P. Berrange wrote:
After updating the virBuffer APIs to protect against improper usage I
have
been thinking about how we might provider safer memory allocation APIs
with protection against common usage errors and compile time validation of
checks for failure.
The standard C library malloc/realloc/calloc/free APIs have just about the
worst possible design, positively encouraging serious application coding
errors. There are at least 7 common problems I can think of at the moment,
perhaps more....
1. malloc() - pass incorrect number of bytes
2. malloc() - fail to check the return value
3. malloc() - use uninitialized memory
4. free() - double free by not setting pointer to NULL
5. realloc() - fail to check the return value
6. realloc() - fail to re-assign the pointer with new address
7. realloc() - accidental overwrite of original pointer with NULL on
failure, leaking memory
Many applications / libraries wrap these in their own functions, but typically
fail to address one or more these problems.
eg glib re-definitions try to address point 2 by having g_malloc() call
abort() on failure, but then re-introduce the problem by adding g_try_malloc()
Calling abort() in a library is a major NO-NO and one of the reasons
I avoided glib in most of the code I developped. You just can't exit()/abort()
from a library.
All 7 of the errors listed early could be avoided and/or checked at
compile
time if the APIs used a different calling convention.
1. For any given pointer the compiler knows how many bytes need to be
allocated for a single instance of it. ie sizeof(*(ptr)). Since C
has no data type representing a data type, this cannot be done with
a simple function - it needs a (trivial) macro.
2. The __attribute__((__warn_unused_result__)) annotation causes the
compiler to warn if an application doesn't do something with a return
value. With the standard malloc() API this doesn't help because you
always assign the return value to a pointer. The core problem here
is that the API encodes the error condition as a special case of
the actual data returned. This can be addressed by separating the
two. The return value should simply be bi-state success or fail code
and the return data passed back via an pointer to a pointer parameter.
3. The memory allocated by malloc() is not initialized to zero. For
that a separate calloc() function is provided. This leaves open the
possiblity of an app mistakenly using the wrong variant. Or consider
an app using malloc() and explicitly initializing all struct members.
Some time later a new member is added and now the developer needs to
find all malloc() calls wrt to that struct and explicitly initilize
the new member. It would be safer to always have malloc zero all
memory, even though it has a small performance impact, the net win
in safety is probably worth it.
4. Since free() takes the pointer to be freed directly it cannot reset
the caller's original pointer back to NULL. This can be addressed if
a pointer to the pointer is passed instead. The computational cost
of the often redundant assignment to NULL is negligable given the
safety benefit
5, 6 & 7. As with problem 2, these problems are all caused by the fact
that the error condition is special case of the return data value.
The impact of these is typically far worse because it often results
in a buffer overflow and the complex rules for calling realloc() are
hard to remember.
So the core requirements of a safer API for allocation are:
- Use return values *only* for the success/fail error condition
- Annotate all the return values with __warn_unused_result__
- Pass a pointer to the pointer into all functions
- Define macros around all functions to automatically do the sizeof(*(ptr))
Passing a pointer to a pointer gets a little tedious because of the need
to write '&(foo)' instead of just 'foo', and is actually introduces
a
new problem of the caller accidentally passing merely a pointer, instead
of a pointer to a pointer. This is a minor problem though because it will
be identified on the very first run of the code. In addition, since we're
defining macros around every function we can have the macro also convert
from ptr to &(ptr) for us, avoiding this potential error:
So the primary application usage would be via a set of macros:
VIR_ALLOC(ptr)
VIR_ALLOC_N(ptr, count)
VIR_REALLOC(ptr)
you will need some size here.
VIR_REALLOC_N(ptr, count)
VIR_FREE(ptr)
These call through to the underlying APIs:
int virAlloc(void *, size_t bytes)
int virAllocN(void *, size_t bytes, size_t count)
int virRealloc(void *, size_t bytes)
int virReallocN(void *, size_t bytes, size_t count)
int virFree(void *)
Note that although we're passing a pointer to a pointer into all these, the
first param is still just 'void *' and not 'void **'. This is because
'void *'
is defined to hold any type of pointer, and using 'void **' causes gcc to
complain bitterly about strict aliasing violations. Internally the impls of
these methods will safely cast to 'void **' when deferencing the pointer.
One of the problem this introduce is what uses to be a common mistake
freeing the a non-pointer object which is normally immediately pointed out
by the compiler whicll be lost because your macro will turn this into
a void * to the data, and you will get a runtime problem instead.
All 4 of Alloc/Realloc functions will have __warn_unused_result__
annotation
so the caller is forced to check the return value for failure, validated at
compile time generating a warning (or fatal compile error with -Werror).
So to wire up the macros to the APIs:
#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
#define VIR_REALLOC(ptr) virRealloc(&(ptr), sizeof(*(ptr)))
That i really don't understand. How do you expect to use that realloc
without passing a new size.
#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr),
sizeof(*(ptr)), (count))
That I can understand , but the previous one i can't.
#define VIR_FREE(ptr) virFree(&(ptr))
[...]
Much less ugly:
if (VIR_ALLOC_N(guest->arch.defaultInfo.machines, nmachines) < 0)
return -1;
if (VIR_REALLOC(migrateTrans, caps->host.nmigrateTrans+1) < 0)
return -1;
how does sizeof(*(caps->host.nmigrateTrans+1)) increases the size ?
Doesn't make sense to me, you take a pointer, increment it, so basically just
pointing to the next element in the array, but the size of the pointed object
would be identical and realloc() becomes a noop.
The proposal may help clean a lot of things, but VIR_REALLOC I don't
understand, what did i missed ?
and produced easier to read code too. The cleaner realloc() API is
particularly
appealing.
Well ... i don't understand it !
This does all seem a little bit too good to be true - I'm
wondering why other
apps/libraries don't use this style of allocation functions already ? Perhaps
someone can find nasty flaws in this approach, but hopefuly not...
I would be tempted to hook into the error reporting directly within
memery.c , even if we don't have a good context there, basically if we
have a memory shortage even reporting to stderr is sensible , at least once.
+int virAlloc(void *ptrptr, size_t size)
+{
+ if (size == 0) {
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+
+ *(void **)ptrptr = calloc(1, size);
+ if (*(void **)ptrptr == NULL)
+ return -1;
+ return 0;
+}
[...]
Index: hash.c
===================================================================
RCS file: /data/cvs/libvirt/src/hash.c,v
retrieving revision 1.37
diff -u -p -r1.37 hash.c
--- hash.c 18 Apr 2008 08:33:23 -0000 1.37
+++ hash.c 27 Apr 2008 19:04:28 -0000
The hash example is interesting, but it doesn't use REALLOC ...
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:41 a.m.
On Mon, 2008-04-28 at 03:39 -0400, Daniel Veillard wrote:
> #define VIR_REALLOC(ptr) virRealloc(&(ptr),
sizeof(*(ptr)))
That i really don't understand. How do you expect to use that realloc
without passing a new size.
> #define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)),
(count))
That I can understand , but the previous one i can't.
> #define VIR_FREE(ptr) virFree(&(ptr))
[...]
> Much less ugly:
>
> if (VIR_ALLOC_N(guest->arch.defaultInfo.machines, nmachines) < 0)
> return -1;
>
> if (VIR_REALLOC(migrateTrans, caps->host.nmigrateTrans+1) < 0)
> return -1;
how does sizeof(*(caps->host.nmigrateTrans+1)) increases the size ?
Doesn't make sense to me, you take a pointer, increment it, so basically just
pointing to the next element in the array, but the size of the pointed object
would be identical and realloc() becomes a noop.
The proposal may help clean a lot of things, but VIR_REALLOC I don't
understand, what did i missed ?
Looks to me like VIR_REALLOC() would be a rarely used API and Dan just
had a typo in the above example - it should have used VIR_REALLOC_N()
I'm having difficulty thinking of how VIR_REALLOC() might be interesting
- e.g. the highly contrived example below.
Cheers,
Mark.
struct foo
{
int i;
};
struct bar
{
struct foo foo;
int j;
}
struct foo *get_foo(int i)
{
struct foo *foo;
if (!VIR_ALLOC(foo))
return NULL;
foo->i = i;
return foo;
}
struct bar *get_bar(int i, int j)
{
struct bar *bar;
struct foo *foo;
if (!(foo = get_foo(i)))
return NULL;
bar = (struct bar *) foo;
if (!VIR_REALLOC(bar)) {
VIR_FREE(foo);
return NULL;
}
bar->j = j;
return bar;
}
5 a.m.
On Mon, Apr 28, 2008 at 10:41:46AM +0100, Mark McLoughlin wrote:
On Mon, 2008-04-28 at 03:39 -0400, Daniel Veillard wrote:
> The proposal may help clean a lot of things, but VIR_REALLOC I don't
> understand, what did i missed ?
Looks to me like VIR_REALLOC() would be a rarely used API and Dan just
had a typo in the above example - it should have used VIR_REALLOC_N()
ah, okay, I'm not the only one :-)
I'm having difficulty thinking of how VIR_REALLOC() might be
interesting
- e.g. the highly contrived example below.
[...]
struct bar *get_bar(int i, int j)
{
struct bar *bar;
struct foo *foo;
if (!(foo = get_foo(i)))
return NULL;
bar = (struct bar *) foo;
if (!VIR_REALLOC(bar)) {
Seems to me it's usable only if you cast and grow to a different kind of
structure. i would rather ban code like this than explicitely allow it ;-)
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:05 a.m.
On Mon, Apr 28, 2008 at 03:39:41AM -0400, Daniel Veillard wrote:
On Sun, Apr 27, 2008 at 08:29:33PM +0100, Daniel P. Berrange wrote:
> So the primary application usage would be via a set of macros:
>
> VIR_ALLOC(ptr)
> VIR_ALLOC_N(ptr, count)
> VIR_REALLOC(ptr)
you will need some size here.
Sorry, VIR_REALLOC should not have existed - only REALLOC_N is useful.
> Note that although we're passing a pointer to a pointer into
all these, the
> first param is still just 'void *' and not 'void **'. This is
because 'void *'
> is defined to hold any type of pointer, and using 'void **' causes gcc to
> complain bitterly about strict aliasing violations. Internally the impls of
> these methods will safely cast to 'void **' when deferencing the pointer.
One of the problem this introduce is what uses to be a common mistake
freeing the a non-pointer object which is normally immediately pointed out
by the compiler whicll be lost because your macro will turn this into
a void * to the data, and you will get a runtime problem instead.
That is true, but perhaps it is a worthwhile tradeoff, given that we have
had quite a number of ongoing crash problems in the past with double free()
due to missing NULL assignment. At least if you try to fre a non-pointer
you'll see that error raised the first time the code is run, where as the
double free's have often occurred only in error paths & not seen till post
release.
> All 4 of Alloc/Realloc functions will have
__warn_unused_result__ annotation
> so the caller is forced to check the return value for failure, validated at
> compile time generating a warning (or fatal compile error with -Werror).
>
> So to wire up the macros to the APIs:
>
> #define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
> #define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
> #define VIR_REALLOC(ptr) virRealloc(&(ptr), sizeof(*(ptr)))
That i really don't understand. How do you expect to use that realloc
without passing a new size.
It is a mistake - only REALLOC_N is needed
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
7:31 a.m.
On Mon, Apr 28, 2008 at 01:05:45PM +0100, Daniel P. Berrange wrote:
Sorry, VIR_REALLOC should not have existed - only REALLOC_N is
useful.
> > Note that although we're passing a pointer to a pointer into all these,
the
> > first param is still just 'void *' and not 'void **'. This is
because 'void *'
> > is defined to hold any type of pointer, and using 'void **' causes gcc
to
> > complain bitterly about strict aliasing violations. Internally the impls of
> > these methods will safely cast to 'void **' when deferencing the
pointer.
>
> One of the problem this introduce is what uses to be a common mistake
> freeing the a non-pointer object which is normally immediately pointed out
> by the compiler whicll be lost because your macro will turn this into
> a void * to the data, and you will get a runtime problem instead.
clearly i wrote that before my morning coffee had really precolated up
to the brain, thanks for decrypting :-\
That is true, but perhaps it is a worthwhile tradeoff, given that we
have
had quite a number of ongoing crash problems in the past with double free()
due to missing NULL assignment. At least if you try to fre a non-pointer
you'll see that error raised the first time the code is run, where as the
double free's have often occurred only in error paths & not seen till post
release.
Hum, right, that's the only drawback I could find to the proposal. Overall
it really makes sense. The only thing is that it makes slightly harder for
newbies to contribute to the project because they need to learn a bit, but
libvirt is maturing now and that should not be an issue at all.
Actually we should start compiling more complete code rules for the project
The current HACKING is limited to formatting :-) , we probably need
a Documentation/Coding section in the site 9or that could be done in the Wiki)
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:38 a.m.
On Mon, Apr 28, 2008 at 03:39:41AM -0400, Daniel Veillard wrote:
Calling abort() in a library is a major NO-NO and one of the
reasons
I avoided glib in most of the code I developped. You just can't exit()/abort()
from a library.
That depends ... If you can override the abort() function with an
error handler, then I'd say it is OK.
Remember that only about 1 in 10 memory allocations in a program use
malloc. The other 9 use the stack, and effectively call 'abort()' if
they fail with no opportunity to override.
(For me, of course, all this just points to the desperate need to use
proper programming languages for writing critical software .. In 2008
we shouldn't even be having a discussion about memory management, in
the same way we don't discuss laying out the blocks on the hard disk
any more)
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
1:45 p.m.
On Mon, 2008-04-28 at 13:38 +0100, Richard W.M. Jones wrote:
On Mon, Apr 28, 2008 at 03:39:41AM -0400, Daniel Veillard wrote:
> Calling abort() in a library is a major NO-NO and one of the reasons
> I avoided glib in most of the code I developped. You just can't exit()/abort()
> from a library.
That depends ... If you can override the abort() function with an
error handler, then I'd say it is OK.
I used to not think very highly of calling abort() by default, but
reading Havoc's blog post about that a while ago[1] is making me doubt
conventional wisdom. He cites libxml2 as one example where OOM leads to
crashes - I take that not as an indication that there is something wrong
with libxml2, but with the approach of checking and correctly handling
all allocation failures.
Allocation failure happens very rarely, and testing it properly is near
impossible; allocation failures amount to an additional input stream
that is read deep down in the call hierarchy and can generally not be
checked by the caller like other arguments.
So maybe taking a hint from all the languages that contain 'fat'
runtimes isn't the worst of strategies: die loudly by default, and let
the application specify other handlers. In practice, the usefulness of
those handlers is limited by their inability to unwind the stack and
free dead memory on the way out. Has anybody seen such handlers be
useful short of a full exception implementation ?
David
[1] http://log.ometer.com/2008-02.html#4
2:16 p.m.
On Mon, Apr 28, 2008 at 06:45:54PM +0000, David Lutterkort wrote:
On Mon, 2008-04-28 at 13:38 +0100, Richard W.M. Jones wrote:
> On Mon, Apr 28, 2008 at 03:39:41AM -0400, Daniel Veillard wrote:
> > Calling abort() in a library is a major NO-NO and one of the reasons
> > I avoided glib in most of the code I developped. You just can't
exit()/abort()
> > from a library.
>
> That depends ... If you can override the abort() function with an
> error handler, then I'd say it is OK.
I used to not think very highly of calling abort() by default, but
reading Havoc's blog post about that a while ago[1] is making me doubt
conventional wisdom. He cites libxml2 as one example where OOM leads to
crashes - I take that not as an indication that there is something wrong
with libxml2, but with the approach of checking and correctly handling
all allocation failures.
I agree with Havoc that it is not worth checking for OOM unless you
take the time to prove it is correctly handled. As mentioned earlier
in this thread one of the core problems making it impractical is
the API contract of malloc() which means you need manual code inspection
to verify you checked all mallocs(). The API contract I proposed for
virAlloc at least addresses that 1/2 of the problem by letting the
compiler tell us whether any allocations have missing checks. That
leaves the second part of the problem - the cleanup paths. We need
to have the cleanup paths in the code regardless because arbitrary
syscalls (eg, write(), socket(), etc) we invoke may fail. If we are
making sure those cleanup paths are correct anyway, then handling OOM
in this codepaths is minor incremental code & thus a much more tractable
problem.
I've just hacked up a similar approach to the one DBus uses to fail
the 'nth' malloc and run the 'xmconfigtest' test case for every
'n'
between 1 and 200 and OOM handling worked correctly in every case.
I also ran under valgrind and confirmed there were no leaks reported
during any of the OOM conditions.
It'd be definitely be worthwhile trying to get this somehow hooked up
into all the test cases we have and run it automatically. I'm in 100%
agreement that without good test coverage you can't have a high degree
of confidence in the code reliability.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
4:36 p.m.
On Mon, Apr 28, 2008 at 08:16:02PM +0100, Daniel P. Berrange wrote:
I agree with Havoc that it is not worth checking for OOM unless you
take the time to prove it is correctly handled. As mentioned earlier
in this thread one of the core problems making it impractical is
the API contract of malloc() which means you need manual code inspection
to verify you checked all mallocs().
We could actually verify this automatically with CIL. Needs me to be
free of distractions for a week to code it up mind you ...
The API contract I proposed for
virAlloc at least addresses that 1/2 of the problem by letting the
compiler tell us whether any allocations have missing checks. That
leaves the second part of the problem - the cleanup paths. We need
to have the cleanup paths in the code regardless because arbitrary
syscalls (eg, write(), socket(), etc) we invoke may fail. If we are
making sure those cleanup paths are correct anyway, then handling OOM
in this codepaths is minor incremental code & thus a much more tractable
problem.
And these too ...
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
6:51 p.m.
On Mon, 2008-04-28 at 20:16 +0100, Daniel P. Berrange wrote:
I've just hacked up a similar approach to the one DBus uses to
fail
the 'nth' malloc
Does that fail exactly the nth malloc or the nth malloc and after (or
from nth malloc to (n+k)th malloc) ? The latter two are more realistic
for an OOM scenario, and make sure you don't blow up in some error
handling routine.
and run the 'xmconfigtest' test case for every 'n'
between 1 and 200 and OOM handling worked correctly in every case.
Excellent.
David
6:59 p.m.
On Mon, Apr 28, 2008 at 11:51:19PM +0000, David Lutterkort wrote:
On Mon, 2008-04-28 at 20:16 +0100, Daniel P. Berrange wrote:
> I've just hacked up a similar approach to the one DBus uses to fail
> the 'nth' malloc
Does that fail exactly the nth malloc or the nth malloc and after (or
from nth malloc to (n+k)th malloc) ? The latter two are more realistic
for an OOM scenario, and make sure you don't blow up in some error
handling routine.
My quick hack only did the nth, but if so desired we could easily do
the nth -> *n+k)th malloc at cost of polynomial expansion in number of
possiblities we want to check :-) Seriously though, we could probably
get enough coverage with values of k in (0..5)
Running the test suite native is easily fast enough. Running the checks
under valgrind was seriously slow but if we want to test for leaking
memory in cleanup paths its needed :-)
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2:19 p.m.
On Mon, Apr 28, 2008 at 06:45:54PM +0000, David Lutterkort wrote:
On Mon, 2008-04-28 at 13:38 +0100, Richard W.M. Jones wrote:
> On Mon, Apr 28, 2008 at 03:39:41AM -0400, Daniel Veillard wrote:
> > Calling abort() in a library is a major NO-NO and one of the reasons
> > I avoided glib in most of the code I developped. You just can't
exit()/abort()
> > from a library.
>
> That depends ... If you can override the abort() function with an
> error handler, then I'd say it is OK.
I used to not think very highly of calling abort() by default, but
reading Havoc's blog post about that a while ago[1] is making me doubt
conventional wisdom. He cites libxml2 as one example where OOM leads to
crashes - I take that not as an indication that there is something wrong
with libxml2, but with the approach of checking and correctly handling
all allocation failures.
Allocation failure happens very rarely, and testing it properly is near
impossible; allocation failures amount to an additional input stream
that is read deep down in the call hierarchy and can generally not be
checked by the caller like other arguments.
So maybe taking a hint from all the languages that contain 'fat'
runtimes isn't the worst of strategies: die loudly by default, and let
the application specify other handlers. In practice, the usefulness of
those handlers is limited by their inability to unwind the stack and
free dead memory on the way out. Has anybody seen such handlers be
useful short of a full exception implementation ?
David
[1] http://log.ometer.com/2008-02.html#4
To quote
"dbus-daemon was the motivation for OOM handling, since dbus-daemon
can't crash."
dbus-daemon is a critical piece of any modern Linux OS so this paranoia
is worth it. libvirtd is arguably even more critical, because failure
can take out *every* OS. Which reminds me that as well as beefing up
the OOM handling, we need to make the daemon more failure tolerant by
switching to using UNIX sockets for talking to the monitor, thus allowing
restarts without killing the VMs.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
4:34 p.m.
On Mon, Apr 28, 2008 at 06:45:54PM +0000, David Lutterkort wrote:
So maybe taking a hint from all the languages that contain
'fat'
runtimes isn't the worst of strategies: die loudly by default, and let
the application specify other handlers. In practice, the usefulness of
those handlers is limited by their inability to unwind the stack and
free dead memory on the way out. Has anybody seen such handlers be
useful short of a full exception implementation ?
Since you say, yes!
My old c2lib[1] C library uses longjmp to recover from memory
allocation errors, so of course it doesn't free up as it unwinds the
stack. However, c2lib also mandates using a pool allocator which
means to all intents and purposes the memory and other resources do
get freed up when the current pool is freed. If you scope your pools
sensibly (around transctions, as in that blog entry) then recovering
from memory errors works.
So it is possible, in C. Of course this is highly theoretical because
the only major C program I can think of which uses pools is Apache.
Rich.
[1] http://www.annexia.org/freeware/c2lib/
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
1:39 a.m.
On Mon, Apr 28, 2008 at 06:45:54PM +0000, David Lutterkort wrote:
On Mon, 2008-04-28 at 13:38 +0100, Richard W.M. Jones wrote:
> On Mon, Apr 28, 2008 at 03:39:41AM -0400, Daniel Veillard wrote:
> > Calling abort() in a library is a major NO-NO and one of the reasons
> > I avoided glib in most of the code I developped. You just can't
exit()/abort()
> > from a library.
>
> That depends ... If you can override the abort() function with an
> error handler, then I'd say it is OK.
I used to not think very highly of calling abort() by default, but
reading Havoc's blog post about that a while ago[1] is making me doubt
conventional wisdom. He cites libxml2 as one example where OOM leads to
crashes - I take that not as an indication that there is something wrong
with libxml2, but with the approach of checking and correctly handling
all allocation failures.
It's a matter of testing, Havoc looked at it some years ago, but now
the Huawei people (embedded/telecom) are chasing the behaviour on OOM
systematically. Of course the more dynamic and complex the data structures
are the harder it is, and XML tree processing is on the hard side
unfortunately.
Allocation failure happens very rarely, and testing it properly is
near
impossible; allocation failures amount to an additional input stream
that is read deep down in the call hierarchy and can generally not be
checked by the caller like other arguments.
you can, you do fault injection, testing systematically each allocation
done during a test run, computationally expensive for sure, but doable,
it's being done...
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:38 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
After updating the virBuffer APIs to protect against improper usage I
have
been thinking about how we might provider safer memory allocation APIs
with protection against common usage errors and compile time validation of
checks for failure.
The standard C library malloc/realloc/calloc/free APIs have just about the
worst possible design, positively encouraging serious application coding
errors. There are at least 7 common problems I can think of at the moment,
perhaps more....
1. malloc() - pass incorrect number of bytes
2. malloc() - fail to check the return value
3. malloc() - use uninitialized memory
4. free() - double free by not setting pointer to NULL
5. realloc() - fail to check the return value
6. realloc() - fail to re-assign the pointer with new address
7. realloc() - accidental overwrite of original pointer with NULL on
failure, leaking memory
I like the concept.
...
3. The memory allocated by malloc() is not initialized to zero. For
that a separate calloc() function is provided. This leaves open the
possiblity of an app mistakenly using the wrong variant. Or consider
an app using malloc() and explicitly initializing all struct members.
Some time later a new member is added and now the developer needs to
find all malloc() calls wrt to that struct and explicitly initilize
the new member. It would be safer to always have malloc zero all
memory, even though it has a small performance impact, the net win
in safety is probably worth it.
Always using calloc feels like a cure that's worse than the disease,
because always using calloc would deprive us of the ability to use
tools like valgrind to detect uses of uninitialized heap memory. In your
example, it's true that one must look at every instance and adapt. If you
don't, whenever that code is exercised, there will be a used-uninitialized
error. But if the code uses calloc everywhere, there will be no warning
from valgrind, and merely a use of blindly-initialized-to-zero memory.
That problem may end up being harder to diagnose.
Of course, valgrind can help here only to the extent that we use
it and have sufficient test coverage.
+int virReallocN(void *ptrptr, size_t size, size_t count)
+{
+ void *tmp;
+ if (size == 0 || count == 0) {
+ free(*(void **)ptrptr);
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+ tmp = realloc(*(void**)ptrptr, size * count);
+ if (!tmp)
+ return -1;
+ *(void**)ptrptr = tmp;
+ return 0;
+}
You'll want to guard against integer overflow in the product.
E.g., insert this just before the realloc call:
if (xalloc_oversized (count, size)) {
errno = ENOMEM
return -1;
}
The definition of xalloc_oversized comes from gnulib's xalloc.h:
/* Return 1 if an array of N objects, each of size S, cannot exist due
to size arithmetic overflow. S must be positive and N must be
nonnegative. This is a macro, not an inline function, so that it
works correctly even when SIZE_MAX < N.
By gnulib convention, SIZE_MAX represents overflow in size
calculations, so the conservative dividend to use here is
SIZE_MAX - 1, since SIZE_MAX might represent an overflowed value.
However, malloc (SIZE_MAX) fails on all known hosts where
sizeof (ptrdiff_t) <= sizeof (size_t), so do not bother to test for
exactly-SIZE_MAX allocations on such hosts; this avoids a test and
branch when S is known to be 1. */
# define xalloc_oversized(n, s) \
((size_t) (sizeof (ptrdiff_t) <= sizeof (size_t) ? -1 : -2) / (s) < (n))
11:51 a.m.
On Sun, Apr 27, 2008 at 08:29:33PM +0100, Daniel P. Berrange wrote:
After updating the virBuffer APIs to protect against improper usage I
have
been thinking about how we might provider safer memory allocation APIs
with protection against common usage errors and compile time validation of
checks for failure.
Here is an updated version which removes the bogus VIR_REALLOC function
and illustrates use in capabilities.c which is a more interesting test
case than hash.c
proxy/Makefile.am | 1
src/capabilities.c | 121 +++++++++++++++++++++++------------------------------
src/hash.c | 113 +++++++++++++++++++++++--------------------------
src/internal.h | 7 ++-
src/memory.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++
src/memory.h | 82 +++++++++++++++++++++++++++++++++++
6 files changed, 313 insertions(+), 128 deletions(-)
Dan.
Index: src/capabilities.c
===================================================================
RCS file: /data/cvs/libvirt/src/capabilities.c,v
retrieving revision 1.7
diff -u -p -r1.7 capabilities.c
--- src/capabilities.c 28 Apr 2008 15:14:59 -0000 1.7
+++ src/capabilities.c 28 Apr 2008 16:47:48 -0000
@@ -25,6 +25,7 @@
#include "capabilities.h"
#include "buf.h"
+#include "memory.h"
/**
@@ -42,7 +43,7 @@ virCapabilitiesNew(const char *arch,
{
virCapsPtr caps;
- if ((caps = calloc(1, sizeof(*caps))) == NULL)
+ if (VIR_ALLOC(caps) < 0)
goto no_memory;
if ((caps->host.arch = strdup(arch)) == NULL)
@@ -60,53 +61,53 @@ virCapabilitiesNew(const char *arch,
static void
virCapabilitiesFreeHostNUMACell(virCapsHostNUMACellPtr cell)
{
- free(cell->cpus);
- free(cell);
+ VIR_FREE(cell->cpus);
+ VIR_FREE(cell);
}
static void
virCapabilitiesFreeGuestDomain(virCapsGuestDomainPtr dom)
{
int i;
- free(dom->info.emulator);
- free(dom->info.loader);
+ VIR_FREE(dom->info.emulator);
+ VIR_FREE(dom->info.loader);
for (i = 0 ; i < dom->info.nmachines ; i++)
- free(dom->info.machines[i]);
- free(dom->info.machines);
- free(dom->type);
+ VIR_FREE(dom->info.machines[i]);
+ VIR_FREE(dom->info.machines);
+ VIR_FREE(dom->type);
- free(dom);
+ VIR_FREE(dom);
}
static void
virCapabilitiesFreeGuestFeature(virCapsGuestFeaturePtr feature)
{
- free(feature->name);
- free(feature);
+ VIR_FREE(feature->name);
+ VIR_FREE(feature);
}
static void
virCapabilitiesFreeGuest(virCapsGuestPtr guest)
{
int i;
- free(guest->ostype);
+ VIR_FREE(guest->ostype);
- free(guest->arch.name);
- free(guest->arch.defaultInfo.emulator);
- free(guest->arch.defaultInfo.loader);
+ VIR_FREE(guest->arch.name);
+ VIR_FREE(guest->arch.defaultInfo.emulator);
+ VIR_FREE(guest->arch.defaultInfo.loader);
for (i = 0 ; i < guest->arch.defaultInfo.nmachines ; i++)
- free(guest->arch.defaultInfo.machines[i]);
- free(guest->arch.defaultInfo.machines);
+ VIR_FREE(guest->arch.defaultInfo.machines[i]);
+ VIR_FREE(guest->arch.defaultInfo.machines);
for (i = 0 ; i < guest->arch.ndomains ; i++)
virCapabilitiesFreeGuestDomain(guest->arch.domains[i]);
- free(guest->arch.domains);
+ VIR_FREE(guest->arch.domains);
for (i = 0 ; i < guest->nfeatures ; i++)
virCapabilitiesFreeGuestFeature(guest->features[i]);
- free(guest->features);
+ VIR_FREE(guest->features);
- free(guest);
+ VIR_FREE(guest);
}
@@ -122,21 +123,21 @@ virCapabilitiesFree(virCapsPtr caps) {
for (i = 0 ; i < caps->nguests ; i++)
virCapabilitiesFreeGuest(caps->guests[i]);
- free(caps->guests);
+ VIR_FREE(caps->guests);
for (i = 0 ; i < caps->host.nfeatures ; i++)
- free(caps->host.features[i]);
- free(caps->host.features);
+ VIR_FREE(caps->host.features[i]);
+ VIR_FREE(caps->host.features);
for (i = 0 ; i < caps->host.nnumaCell ; i++)
virCapabilitiesFreeHostNUMACell(caps->host.numaCell[i]);
- free(caps->host.numaCell);
+ VIR_FREE(caps->host.numaCell);
for (i = 0 ; i < caps->host.nmigrateTrans ; i++)
- free(caps->host.migrateTrans[i]);
- free(caps->host.migrateTrans);
+ VIR_FREE(caps->host.migrateTrans[i]);
+ VIR_FREE(caps->host.migrateTrans);
- free(caps->host.arch);
- free(caps);
+ VIR_FREE(caps->host.arch);
+ VIR_FREE(caps);
}
@@ -151,12 +152,9 @@ int
virCapabilitiesAddHostFeature(virCapsPtr caps,
const char *name)
{
- char **features;
-
- if ((features = realloc(caps->host.features,
- sizeof(*features) * (caps->host.nfeatures+1))) == NULL)
+ if (VIR_REALLOC_N(caps->host.features,
+ caps->host.nfeatures + 1) < 0)
return -1;
- caps->host.features = features;
if ((caps->host.features[caps->host.nfeatures] = strdup(name)) == NULL)
return -1;
@@ -177,12 +175,9 @@ int
virCapabilitiesAddHostMigrateTransport(virCapsPtr caps,
const char *name)
{
- char **migrateTrans;
-
- if ((migrateTrans = realloc(caps->host.migrateTrans,
- sizeof(*migrateTrans) * (caps->host.nmigrateTrans+1)))
== NULL)
+ if (VIR_REALLOC_N(caps->host.migrateTrans,
+ caps->host.nmigrateTrans + 1) < 0)
return -1;
- caps->host.migrateTrans = migrateTrans;
if ((caps->host.migrateTrans[caps->host.nmigrateTrans] = strdup(name)) ==
NULL)
return -1;
@@ -208,19 +203,18 @@ virCapabilitiesAddHostNUMACell(virCapsPt
int ncpus,
const int *cpus)
{
- virCapsHostNUMACellPtr cell, *cells;
+ virCapsHostNUMACellPtr cell;
- if ((cells = realloc(caps->host.numaCell,
- sizeof(*cells) * (caps->host.nnumaCell+1))) == NULL)
+ if (VIR_REALLOC_N(caps->host.numaCell,
+ caps->host.nnumaCell + 1) < 0)
return -1;
- caps->host.numaCell = cells;
- if ((cell = calloc(1, sizeof(cell))) == NULL)
+ if (VIR_ALLOC(cell) < 0)
return -1;
caps->host.numaCell[caps->host.nnumaCell] = cell;
- if ((caps->host.numaCell[caps->host.nnumaCell]->cpus =
- malloc(ncpus * sizeof(*cpus))) == NULL)
+ if (VIR_ALLOC_N(caps->host.numaCell[caps->host.nnumaCell]->cpus,
+ ncpus) < 0)
return -1;
memcpy(caps->host.numaCell[caps->host.nnumaCell]->cpus,
cpus,
@@ -259,10 +253,10 @@ virCapabilitiesAddGuest(virCapsPtr caps,
int nmachines,
const char *const *machines)
{
- virCapsGuestPtr guest, *guests;
+ virCapsGuestPtr guest;
int i;
- if ((guest = calloc(1, sizeof(*guest))) == NULL)
+ if (VIR_ALLOC(guest) < 0)
goto no_memory;
if ((guest->ostype = strdup(ostype)) == NULL)
@@ -279,8 +273,8 @@ virCapabilitiesAddGuest(virCapsPtr caps,
(guest->arch.defaultInfo.loader = strdup(loader)) == NULL)
goto no_memory;
if (nmachines) {
- if ((guest->arch.defaultInfo.machines =
- calloc(nmachines, sizeof(*guest->arch.defaultInfo.machines))) == NULL)
+ if (VIR_ALLOC_N(guest->arch.defaultInfo.machines,
+ nmachines) < 0)
goto no_memory;
for (i = 0 ; i < nmachines ; i++) {
if ((guest->arch.defaultInfo.machines[i] = strdup(machines[i])) == NULL)
@@ -289,11 +283,9 @@ virCapabilitiesAddGuest(virCapsPtr caps,
}
}
- if ((guests = realloc(caps->guests,
- sizeof(*guests) *
- (caps->nguests + 1))) == NULL)
+ if (VIR_REALLOC_N(caps->guests,
+ caps->nguests + 1) < 0)
goto no_memory;
- caps->guests = guests;
caps->guests[caps->nguests] = guest;
caps->nguests++;
@@ -325,10 +317,10 @@ virCapabilitiesAddGuestDomain(virCapsGue
int nmachines,
const char *const *machines)
{
- virCapsGuestDomainPtr dom, *doms;
+ virCapsGuestDomainPtr dom;
int i;
- if ((dom = calloc(1, sizeof(*dom))) == NULL)
+ if (VIR_ALLOC(dom) < 0)
goto no_memory;
if ((dom->type = strdup(hvtype)) == NULL)
@@ -341,8 +333,7 @@ virCapabilitiesAddGuestDomain(virCapsGue
(dom->info.loader = strdup(loader)) == NULL)
goto no_memory;
if (nmachines) {
- if ((dom->info.machines =
- calloc(nmachines, sizeof(*dom->info.machines))) == NULL)
+ if (VIR_ALLOC_N(dom->info.machines, nmachines) < 0)
goto no_memory;
for (i = 0 ; i < nmachines ; i++) {
if ((dom->info.machines[i] = strdup(machines[i])) == NULL)
@@ -351,11 +342,9 @@ virCapabilitiesAddGuestDomain(virCapsGue
}
}
- if ((doms = realloc(guest->arch.domains,
- sizeof(*doms) *
- (guest->arch.ndomains + 1))) == NULL)
+ if (VIR_REALLOC_N(guest->arch.domains,
+ guest->arch.ndomains + 1) < 0)
goto no_memory;
- guest->arch.domains = doms;
guest->arch.domains[guest->arch.ndomains] = dom;
guest->arch.ndomains++;
@@ -383,9 +372,9 @@ virCapabilitiesAddGuestFeature(virCapsGu
int defaultOn,
int toggle)
{
- virCapsGuestFeaturePtr feature, *features;
+ virCapsGuestFeaturePtr feature;
- if ((feature = calloc(1, sizeof(*feature))) == NULL)
+ if (VIR_ALLOC(feature) < 0)
goto no_memory;
if ((feature->name = strdup(name)) == NULL)
@@ -393,11 +382,9 @@ virCapabilitiesAddGuestFeature(virCapsGu
feature->defaultOn = defaultOn;
feature->toggle = toggle;
- if ((features = realloc(guest->features,
- sizeof(*features) *
- (guest->nfeatures + 1))) == NULL)
+ if (VIR_REALLOC_N(guest->features,
+ guest->nfeatures + 1) < 0)
goto no_memory;
- guest->features = features;
guest->features[guest->nfeatures] = feature;
guest->nfeatures++;
Index: src/hash.c
===================================================================
RCS file: /data/cvs/libvirt/src/hash.c,v
retrieving revision 1.37
diff -u -p -r1.37 hash.c
--- src/hash.c 18 Apr 2008 08:33:23 -0000 1.37
+++ src/hash.c 28 Apr 2008 16:47:48 -0000
@@ -25,6 +25,7 @@
#include <libxml/threads.h>
#include "internal.h"
#include "hash.h"
+#include "memory.h"
#define MAX_HASH_LEN 8
@@ -85,22 +86,22 @@ virHashComputeKey(virHashTablePtr table,
virHashTablePtr
virHashCreate(int size)
{
- virHashTablePtr table;
+ virHashTablePtr table = NULL;
if (size <= 0)
size = 256;
- table = malloc(sizeof(*table));
- if (table) {
- table->size = size;
- table->nbElems = 0;
- table->table = calloc(1, size * sizeof(*(table->table)));
- if (table->table) {
- return (table);
- }
- free(table);
+ if (VIR_ALLOC(table) < 0)
+ return NULL;
+
+ table->size = size;
+ table->nbElems = 0;
+ if (VIR_ALLOC_N(table->table, size) < 0) {
+ VIR_FREE(table);
+ return NULL;
}
- return (NULL);
+
+ return table;
}
/**
@@ -136,8 +137,7 @@ virHashGrow(virHashTablePtr table, int s
if (oldtable == NULL)
return (-1);
- table->table = calloc(1, size * sizeof(*(table->table)));
- if (table->table == NULL) {
+ if (VIR_ALLOC_N(table->table, size) < 0) {
table->table = oldtable;
return (-1);
}
@@ -170,7 +170,7 @@ virHashGrow(virHashTablePtr table, int s
if (table->table[key].valid == 0) {
memcpy(&(table->table[key]), iter, sizeof(virHashEntry));
table->table[key].next = NULL;
- free(iter);
+ VIR_FREE(iter);
} else {
iter->next = table->table[key].next;
table->table[key].next = iter;
@@ -184,7 +184,7 @@ virHashGrow(virHashTablePtr table, int s
}
}
- free(oldtable);
+ VIR_FREE(oldtable);
#ifdef DEBUG_GROW
xmlGenericError(xmlGenericErrorContext,
@@ -225,19 +225,19 @@ virHashFree(virHashTablePtr table, virHa
next = iter->next;
if ((f != NULL) && (iter->payload != NULL))
f(iter->payload, iter->name);
- free(iter->name);
+ VIR_FREE(iter->name);
iter->payload = NULL;
if (!inside_table)
- free(iter);
+ VIR_FREE(iter);
nbElems--;
inside_table = 0;
iter = next;
}
inside_table = 0;
}
- free(table->table);
+ VIR_FREE(table->table);
}
- free(table);
+ VIR_FREE(table);
}
/**
@@ -281,8 +281,7 @@ virHashAddEntry(virHashTablePtr table, c
if (insert == NULL) {
entry = &(table->table[key]);
} else {
- entry = malloc(sizeof(*entry));
- if (entry == NULL)
+ if (VIR_ALLOC(entry) < 0)
return (-1);
}
@@ -354,8 +353,7 @@ virHashUpdateEntry(virHashTablePtr table
if (insert == NULL) {
entry = &(table->table[key]);
} else {
- entry = malloc(sizeof(*entry));
- if (entry == NULL)
+ if (VIR_ALLOC(entry) < 0)
return (-1);
}
@@ -451,10 +449,10 @@ virHashRemoveEntry(virHashTablePtr table
if ((f != NULL) && (entry->payload != NULL))
f(entry->payload, entry->name);
entry->payload = NULL;
- free(entry->name);
+ VIR_FREE(entry->name);
if (prev) {
prev->next = entry->next;
- free(entry);
+ VIR_FREE(entry);
} else {
if (entry->next == NULL) {
entry->valid = 0;
@@ -462,7 +460,7 @@ virHashRemoveEntry(virHashTablePtr table
entry = entry->next;
memcpy(&(table->table[key]), entry,
sizeof(virHashEntry));
- free(entry);
+ VIR_FREE(entry);
}
}
table->nbElems--;
@@ -535,11 +533,11 @@ int virHashRemoveSet(virHashTablePtr tab
if (iter(entry->payload, entry->name, data)) {
count++;
f(entry->payload, entry->name);
- free(entry->name);
+ VIR_FREE(entry->name);
table->nbElems--;
if (prev) {
prev->next = entry->next;
- free(entry);
+ VIR_FREE(entry);
entry = prev;
} else {
if (entry->next == NULL) {
@@ -549,7 +547,7 @@ int virHashRemoveSet(virHashTablePtr tab
entry = entry->next;
memcpy(&(table->table[i]), entry,
sizeof(virHashEntry));
- free(entry);
+ VIR_FREE(entry);
entry = &(table->table[i]);
continue;
}
@@ -689,8 +687,7 @@ virConnectPtr
virGetConnect(void) {
virConnectPtr ret;
- ret = calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(NULL, VIR_ERR_NO_MEMORY, _("allocating connection"));
goto failed;
}
@@ -729,7 +726,7 @@ failed:
virHashFree(ret->storageVols, (virHashDeallocator)
virStorageVolFreeName);
pthread_mutex_destroy(&ret->lock);
- free(ret);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -759,11 +756,11 @@ virReleaseConnect(virConnectPtr conn) {
if (__lastErr.conn == conn)
__lastErr.conn = NULL;
- free(conn->name);
+ VIR_FREE(conn->name);
pthread_mutex_unlock(&conn->lock);
pthread_mutex_destroy(&conn->lock);
- free(conn);
+ VIR_FREE(conn);
}
/**
@@ -824,8 +821,7 @@ __virGetDomain(virConnectPtr conn, const
ret = (virDomainPtr) virHashLookup(conn->domains, name);
/* TODO check the UUID */
if (ret == NULL) {
- ret = (virDomainPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
goto error;
}
@@ -854,8 +850,8 @@ __virGetDomain(virConnectPtr conn, const
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name );
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -888,8 +884,8 @@ virReleaseDomain(virDomainPtr domain) {
__lastErr.dom = NULL;
domain->magic = -1;
domain->id = -1;
- free(domain->name);
- free(domain);
+ VIR_FREE(domain->name);
+ VIR_FREE(domain);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
@@ -962,8 +958,7 @@ __virGetNetwork(virConnectPtr conn, cons
ret = (virNetworkPtr) virHashLookup(conn->networks, name);
/* TODO check the UUID */
if (ret == NULL) {
- ret = (virNetworkPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
goto error;
}
@@ -991,8 +986,8 @@ __virGetNetwork(virConnectPtr conn, cons
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name );
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -1025,8 +1020,8 @@ virReleaseNetwork(virNetworkPtr network)
__lastErr.net = NULL;
network->magic = -1;
- free(network->name);
- free(network);
+ VIR_FREE(network->name);
+ VIR_FREE(network);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
@@ -1100,8 +1095,7 @@ __virGetStoragePool(virConnectPtr conn,
ret = (virStoragePoolPtr) virHashLookup(conn->storagePools, name);
/* TODO check the UUID */
if (ret == NULL) {
- ret = (virStoragePoolPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating storage
pool"));
goto error;
}
@@ -1129,8 +1123,8 @@ __virGetStoragePool(virConnectPtr conn,
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name);
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -1159,8 +1153,8 @@ virReleaseStoragePool(virStoragePoolPtr
_("pool missing from connection hash table"));
pool->magic = -1;
- free(pool->name);
- free(pool);
+ VIR_FREE(pool->name);
+ VIR_FREE(pool);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
@@ -1232,8 +1226,7 @@ __virGetStorageVol(virConnectPtr conn, c
ret = (virStorageVolPtr) virHashLookup(conn->storageVols, key);
if (ret == NULL) {
- ret = (virStorageVolPtr) calloc(1, sizeof(*ret));
- if (ret == NULL) {
+ if (VIR_ALLOC(ret) < 0) {
virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating storage
vol"));
goto error;
}
@@ -1266,9 +1259,9 @@ __virGetStorageVol(virConnectPtr conn, c
error:
pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- free(ret->name);
- free(ret->pool);
- free(ret);
+ VIR_FREE(ret->name);
+ VIR_FREE(ret->pool);
+ VIR_FREE(ret);
}
return(NULL);
}
@@ -1297,9 +1290,9 @@ virReleaseStorageVol(virStorageVolPtr vo
_("vol missing from connection hash table"));
vol->magic = -1;
- free(vol->name);
- free(vol->pool);
- free(vol);
+ VIR_FREE(vol->name);
+ VIR_FREE(vol->pool);
+ VIR_FREE(vol);
DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
conn->refs--;
Index: src/internal.h
===================================================================
RCS file: /data/cvs/libvirt/src/internal.h,v
retrieving revision 1.71
diff -u -p -r1.71 internal.h
--- src/internal.h 26 Apr 2008 14:22:02 -0000 1.71
+++ src/internal.h 28 Apr 2008 16:47:49 -0000
@@ -78,7 +78,7 @@ extern int debugFlag;
#define VIR_DEBUG(category, fmt,...) \
do { if (debugFlag) fprintf (stderr, "DEBUG: %s: %s (" fmt ")\n",
category, __func__, __VA_ARGS__); } while (0)
#else
-#define VIR_DEBUG(category, fmt,...)
+#define VIR_DEBUG(category, fmt,...) \
do { } while (0)
#endif /* !ENABLE_DEBUG */
@@ -107,9 +107,14 @@ extern int debugFlag;
#define ATTRIBUTE_FORMAT(args...) __attribute__((__format__ (args)))
#endif
+#ifndef ATTRIBUTE_RETURN_CHECK
+#define ATTRIBUTE_RETURN_CHECK __attribute__((__warn_unused_result__))
+#endif
+
#else
#define ATTRIBUTE_UNUSED
#define ATTRIBUTE_FORMAT(...)
+#define ATTRIBUTE_RETURN_CHECK
#endif /* __GNUC__ */
/**
Index: src/memory.c
===================================================================
RCS file: src/memory.c
diff -N src/memory.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ src/memory.c 28 Apr 2008 16:47:49 -0000
@@ -0,0 +1,117 @@
+/*
+ * memory.c: safer memory allocation
+ *
+ * Copyright (C) 2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <stdlib.h>
+
+#include "memory.h"
+
+/**
+ * virAlloc:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ *
+ * Allocate 'size' bytes of memory. Return the address of the
+ * allocated memory in 'ptrptr'. The newly allocated memory is
+ * filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virAlloc(void *ptrptr, size_t size)
+{
+ if (size == 0) {
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+
+ *(void **)ptrptr = calloc(1, size);
+ if (*(void **)ptrptr == NULL)
+ return -1;
+ return 0;
+}
+
+/**
+ * virAllocN:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ * @count: number of elements to allocate
+ *
+ * Allocate an array of memory 'count' elements long,
+ * each with 'size' bytes. Return the address of the
+ * allocated memory in 'ptrptr'. The newly allocated
+ * memory is filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virAllocN(void *ptrptr, size_t size, size_t count)
+{
+ if (size == 0 || count == 0) {
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+
+ *(void**)ptrptr = calloc(count, size);
+ if (*(void**)ptrptr == NULL)
+ return -1;
+ return 0;
+}
+
+/**
+ * virReallocN:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes to allocate
+ * @count: number of elements in array
+ *
+ * Resize the block of memory in 'ptrptr' to be an array of
+ * 'count' elements, each 'size' bytes in length. Update
'ptrptr'
+ * with the address of the newly allocated memory. On failure,
+ * 'ptrptr' is not changed and still points to the original memory
+ * block. The newly allocated memory is filled with zeros.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virReallocN(void *ptrptr, size_t size, size_t count)
+{
+ void *tmp;
+ if (size == 0 || count == 0) {
+ free(*(void **)ptrptr);
+ *(void **)ptrptr = NULL;
+ return 0;
+ }
+ tmp = realloc(*(void**)ptrptr, size * count);
+ if (!tmp)
+ return -1;
+ *(void**)ptrptr = tmp;
+ return 0;
+}
+
+/**
+ * virFree:
+ * @ptrptr: pointer to pointer for address of memory to be freed
+ *
+ * Release the chunk of memory in the pointer pointed to by
+ * the 'ptrptr' variable. After release, 'ptrptr' will be
+ * updated to point to NULL.
+ */
+void virFree(void *ptrptr)
+{
+ free(*(void**)ptrptr);
+ *(void**)ptrptr = NULL;
+}
Index: src/memory.h
===================================================================
RCS file: src/memory.h
diff -N src/memory.h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ src/memory.h 28 Apr 2008 16:47:49 -0000
@@ -0,0 +1,82 @@
+/*
+ * memory.c: safer memory allocation
+ *
+ * Copyright (C) 2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+
+#ifndef __VIR_MEMORY_H_
+#define __VIR_MEMORY_H_
+
+#include "internal.h"
+
+/* Don't call these directly - use the macros below */
+int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK;
+int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
+int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
+void virFree(void *ptrptr);
+
+
+/**
+ * VIR_ALLOC:
+ * @ptr: pointer to hold address of allocated memory
+ *
+ * Allocate sizeof(*ptr) bytes of memory and store
+ * the address of allocated memory in 'ptr'. Fill the
+ * newly allocated memory with zeros.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
+
+/**
+ * VIR_ALLOC_N:
+ * @ptr: pointer to hold address of allocated memory
+ * @count: number of elements to allocate
+ *
+ * Allocate an array of 'count' elements, each sizeof(*ptr)
+ * bytes long and store the address of allocated memory in
+ * 'ptr'. Fill the newly allocated memory with zeros.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
+
+/**
+ * VIR_REALLOC_N:
+ * @ptr: pointer to hold address of allocated memory
+ * @count: number of elements to allocate
+ *
+ * Re-allocate an array of 'count' elements, each sizeof(*ptr)
+ * bytes long and store the address of allocated memory in
+ * 'ptr'. Fill the newly allocated memory with zeros
+ *
+ * Returns -1 on failure, 0 on success
+ */
+#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count))
+
+/**
+ * VIR_FREE:
+ * @ptr: pointer holding address to be freed
+ *
+ * Free the memory stored in 'ptr' and update to point
+ * to NULL.
+ */
+#define VIR_FREE(ptr) virFree(&(ptr));
+
+#endif /* __VIR_MEMORY_H_ */
Index: proxy/Makefile.am
===================================================================
RCS file: /data/cvs/libvirt/proxy/Makefile.am,v
retrieving revision 1.16
diff -u -p -r1.16 Makefile.am
--- proxy/Makefile.am 27 Feb 2008 04:35:08 -0000 1.16
+++ proxy/Makefile.am 28 Apr 2008 16:47:49 -0000
@@ -14,6 +14,7 @@ libvirt_proxy_SOURCES = libvirt_proxy.c
@top_srcdir(a)/src/sexpr.c @top_srcdir(a)/src/xml.c \
@top_srcdir(a)/src/xs_internal.c @top_srcdir(a)/src/buf.c \
@top_srcdir(a)/src/capabilities.c \
+ @top_srcdir(a)/src/memory.c \
@top_srcdir(a)/src/util.c \
@top_srcdir(a)/src/uuid.c
libvirt_proxy_LDFLAGS = $(WARN_CFLAGS)
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:43 p.m.
On Mon, Apr 28, 2008 at 05:51:37PM +0100, Daniel P. Berrange wrote:
On Sun, Apr 27, 2008 at 08:29:33PM +0100, Daniel P. Berrange wrote:
> After updating the virBuffer APIs to protect against improper usage I have
> been thinking about how we might provider safer memory allocation APIs
> with protection against common usage errors and compile time validation of
> checks for failure.
Here is an updated version which removes the bogus VIR_REALLOC function
and illustrates use in capabilities.c which is a more interesting test
case than hash.c
Clearly that makes for cleaner code, and more importantly safer.
We should do this, possibly one module at a time, then we can try to
add new syntax-check rules forbidding malloc/realloc. Also potentially
we could hook up memory checks at runtime with the macro, if we need
to in the future.
+1 for applying this now, then we can fix one module at a time later.
we don't need to grow a gigantic patch.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:49 p.m.
On Mon, Apr 28, 2008 at 01:43:52PM -0400, Daniel Veillard wrote:
On Mon, Apr 28, 2008 at 05:51:37PM +0100, Daniel P. Berrange wrote:
> On Sun, Apr 27, 2008 at 08:29:33PM +0100, Daniel P. Berrange wrote:
> > After updating the virBuffer APIs to protect against improper usage I have
> > been thinking about how we might provider safer memory allocation APIs
> > with protection against common usage errors and compile time validation of
> > checks for failure.
>
> Here is an updated version which removes the bogus VIR_REALLOC function
> and illustrates use in capabilities.c which is a more interesting test
> case than hash.c
Clearly that makes for cleaner code, and more importantly safer.
We should do this, possibly one module at a time, then we can try to
add new syntax-check rules forbidding malloc/realloc. Also potentially
we could hook up memory checks at runtime with the macro, if we need
to in the future.
+1 for applying this now, then we can fix one module at a time later.
we don't need to grow a gigantic patch.
Ok I applied this. I'll update some more of the driver code over the course
of the week.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6051
days inactive
6053
days old
18 comments
6 participants
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
David Lutterkort -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones