Re: [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()

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))

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;

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...

+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