Re: [Libvir] RFC: safer memory allocation APIs with compile time checking
"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))