[Libvir] RFC: safer memory allocation APIs with compile time checking

[Jim Meyering]

"Daniel P. Berrange" <berrange at 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))