Re: [Xen-devel] [PATCH 10/31] libxl: Crash (more sensibly) on malloc failure

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-04-10 at 20:07 +0100, Ian Jackson wrote:
> Formally change the libxl memory allocation failure policy to "crash".
> 
> Previously we had a very uneven approach; much code assumed that
> libxl__sprintf (for example) would never return NULL, but some code
> was written more carefully.
> 
> We think it is unlikely that we will be able to make the library
> actually robust against allocation failure (since that would be an
> awful lot of never-tested error paths) and few calling environments
> will be able to cope anyway.  So, instead, adopt the alternative
> approach: provide allocation functions which never return null, but
> will crash the whole process instead.
> 
> Consequently,
>  - New noreturn function libxl__alloc_failed which may be used for
>    printing a vaguely-useful error message, rather than simply
>    dereferencing a null pointer.

We got that in the next patch?

>  - libxl__ptr_add now returns void as it crashes on failure.
>  - libxl__zalloc, _calloc, _strdup, _strndup, crash on failure using
>    libxl__alloc_failed.  So all the code that uses these can no longer
>    dereference null on malloc failure.
> 
> While we're at it, make libxl__ptr_add use realloc rather than
> emulating it with calloc and free, and make it grow the array
> exponentially rather than linearly.
> 
> Things left to do:
>  - Provide versions of malloc, realloc and free which do the
>    checking but which do not enroll the pointer in the gc.
>  - Remove a lot of now-spurious error handling.
>  - Remove the ERROR_NOMEM error code.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

Couple of unimportant nits below.

>              gc->alloc_ptrs[i] = ptr;
> -            return 0;
> +            return;
>          }
>      }
> -    /* realloc alloc_ptrs manually with calloc/free/replace */
> -    re = calloc(gc->alloc_maxsize + 25, sizeof(void *));
> -    if (!re)
> -        return -1;
> -    for (i = 0; i < gc->alloc_maxsize; i++)
> -        re[i] = gc->alloc_ptrs[i];
> -    /* assign the next pointer */
> -    re[i] = ptr;
> +    int new_maxsize = gc->alloc_maxsize * 2 + 25;

+ 25? (I don't mind, seems even more arbitrary now that we have the *2
though).

> +    assert(new_maxsize < INT_MAX / sizeof(void*) / 2);
> +    gc->alloc_ptrs = realloc(gc->alloc_ptrs, new_maxsize * sizeof(void *));
> +    if (!gc->alloc_ptrs)
> +        libxl__alloc_failed(CTX, __func__, sizeof(void*), new_maxsize);

Strictly this should be "..., new_maxsize, sizeof(void*)" since the
arguments are nmemb and size?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel