[Libvir] RFC: safer memory allocation APIs with compile time checking
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 :|
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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; }
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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 :|
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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
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
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 :|
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
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
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 :|
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
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 :|
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
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
"Daniel P. Berrange" <berrange@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))
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@/src/sexpr.c @top_srcdir@/src/xml.c \ @top_srcdir@/src/xs_internal.c @top_srcdir@/src/buf.c \ @top_srcdir@/src/capabilities.c \ + @top_srcdir@/src/memory.c \ @top_srcdir@/src/util.c \ @top_srcdir@/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 :|
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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 :|
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
David Lutterkort -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones