All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ian Jackson <ian.jackson@eu.citrix.com>
To: xen-devel@lists.xensource.com
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH 08/15] libxl: Crash (more sensibly) on malloc failure
Date: Fri, 24 Feb 2012 18:54:56 +0000	[thread overview]
Message-ID: <1330109703-6536-9-git-send-email-ian.jackson@eu.citrix.com> (raw)
In-Reply-To: <1330109703-6536-1-git-send-email-ian.jackson@eu.citrix.com>

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.
 - 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@eu.citrix.com>
---
 tools/libxl/libxl.h          |    4 ++
 tools/libxl/libxl_internal.c |   79 ++++++++++++++++++------------------------
 tools/libxl/libxl_internal.h |    5 ++-
 3 files changed, 42 insertions(+), 46 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 19ef1de..65961d2 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -123,6 +123,10 @@
  * No temporary objects allocated from the pool may be explicitly freed.
  * Therefore public functions which initialize a libxl__gc MUST call
  * libxl__free_all() before returning.
+ *
+ * Memory allocation failures are not handled gracefully.  If malloc
+ * (or realloc) fails, libxl will cause the entire process to print
+ * a message to stderr and exit with status 255.
  */
 #ifndef LIBXL_H
 #define LIBXL_H
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 12c32dc..dfa2153 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -17,40 +17,40 @@
 
 #include "libxl_internal.h"
 
-int libxl__error_set(libxl__gc *gc, int code)
-{
-    return 0;
+void libxl__alloc_failed(const char *func, size_t nmemb, size_t size) {
+    fprintf(stderr,
+            "libxl: FATAL ERROR: memory allocation failure (%s, %lu x %lu)\n",
+            func, (unsigned long)nmemb, (unsigned long)size);
+    fflush(stderr);
+    _exit(-1);
 }
 
-int libxl__ptr_add(libxl__gc *gc, void *ptr)
+void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
     int i;
-    void **re;
 
     if (!ptr)
-        return 0;
+        return;
 
     /* fast case: we have space in the array for storing the pointer */
     for (i = 0; i < gc->alloc_maxsize; i++) {
         if (!gc->alloc_ptrs[i]) {
             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;
+    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(__func__, sizeof(void*), new_maxsize);
 
-    /* replace the old alloc_ptr */
-    free(gc->alloc_ptrs);
-    gc->alloc_ptrs = re;
-    gc->alloc_maxsize += 25;
-    return 0;
+    gc->alloc_ptrs[gc->alloc_maxsize++] = ptr;
+
+    while (gc->alloc_maxsize < new_maxsize)
+        gc->alloc_ptrs[gc->alloc_maxsize++] = 0;
+
+    return;
 }
 
 void libxl__free_all(libxl__gc *gc)
@@ -71,10 +71,7 @@ void libxl__free_all(libxl__gc *gc)
 void *libxl__zalloc(libxl__gc *gc, int bytes)
 {
     void *ptr = calloc(bytes, 1);
-    if (!ptr) {
-        libxl__error_set(gc, ENOMEM);
-        return NULL;
-    }
+    if (!ptr) libxl__alloc_failed(__func__, bytes, 1);
 
     libxl__ptr_add(gc, ptr);
     return ptr;
@@ -83,10 +80,7 @@ void *libxl__zalloc(libxl__gc *gc, int bytes)
 void *libxl__calloc(libxl__gc *gc, size_t nmemb, size_t size)
 {
     void *ptr = calloc(nmemb, size);
-    if (!ptr) {
-        libxl__error_set(gc, ENOMEM);
-        return NULL;
-    }
+    if (!ptr) libxl__alloc_failed(__func__, nmemb, size);
 
     libxl__ptr_add(gc, ptr);
     return ptr;
@@ -97,9 +91,8 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
     void *new_ptr = realloc(ptr, new_size);
     int i = 0;
 
-    if (new_ptr == NULL && new_size != 0) {
-        return NULL;
-    }
+    if (new_ptr == NULL && new_size != 0)
+        libxl__alloc_failed(__func__, new_size, 1);
 
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
@@ -112,7 +105,6 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
         }
     }
 
-
     return new_ptr;
 }
 
@@ -126,16 +118,13 @@ char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
     ret = vsnprintf(NULL, 0, fmt, ap);
     va_end(ap);
 
-    if (ret < 0) {
-        return NULL;
-    }
+    assert(ret >= 0);
 
     s = libxl__zalloc(gc, ret + 1);
-    if (s) {
-        va_start(ap, fmt);
-        ret = vsnprintf(s, ret + 1, fmt, ap);
-        va_end(ap);
-    }
+    va_start(ap, fmt);
+    ret = vsnprintf(s, ret + 1, fmt, ap);
+    va_end(ap);
+
     return s;
 }
 
@@ -143,8 +132,9 @@ char *libxl__strdup(libxl__gc *gc, const char *c)
 {
     char *s = strdup(c);
 
-    if (s)
-        libxl__ptr_add(gc, s);
+    if (!s) libxl__alloc_failed(__func__, strlen(c), 1);
+
+    libxl__ptr_add(gc, s);
 
     return s;
 }
@@ -153,8 +143,7 @@ char *libxl__strndup(libxl__gc *gc, const char *c, size_t n)
 {
     char *s = strndup(c, n);
 
-    if (s)
-        libxl__ptr_add(gc, s);
+    if (!s) libxl__alloc_failed(__func__, n, 1);
 
     return s;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ac892ae..ab2898e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -116,6 +116,9 @@ typedef struct libxl__gc libxl__gc;
 typedef struct libxl__egc libxl__egc;
 typedef struct libxl__ao libxl__ao;
 
+void libxl__alloc_failed(const char *func, size_t nmemb, size_t size)
+    __attribute__((noreturn));
+
 typedef struct libxl__ev_fd libxl__ev_fd;
 typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
                                    int fd, short events, short revents);
@@ -370,7 +373,7 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
  * collection on exit from the outermost libxl callframe.
  */
 /* register @ptr in @gc for free on exit from outermost libxl callframe. */
-_hidden int libxl__ptr_add(libxl__gc *gc, void *ptr);
+_hidden void libxl__ptr_add(libxl__gc *gc, void *ptr);
 /* if this is the outermost libxl callframe then free all pointers in @gc */
 _hidden void libxl__free_all(libxl__gc *gc);
 /* allocate and zero @bytes. (similar to a gc'd malloc(3)+memzero()) */
-- 
1.7.2.5

  parent reply	other threads:[~2012-02-24 18:54 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-24 18:54 (no subject) Ian Jackson
2012-02-24 18:54 ` [PATCH 01/15] libxl: ao: allow immediate completion Ian Jackson
2012-02-24 18:54 ` [PATCH 02/15] libxl: fix hang due to libxl__initiate_device_remove Ian Jackson
2012-02-24 18:54 ` [PATCH 03/15] libxl: Fix eventloop_iteration over-locking Ian Jackson
2012-02-24 18:54 ` [PATCH 04/15] libxl: Fix leak of ctx->lock Ian Jackson
2012-02-24 18:54 ` [PATCH 05/15] libxl: abolish libxl_ctx_postfork Ian Jackson
2012-02-24 18:54 ` [PATCH 06/15] tools: Correct PTHREAD options in config/StdGNU.mk Ian Jackson
2012-02-24 18:54 ` [PATCH 07/15] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS Ian Jackson
2012-02-24 18:54 ` Ian Jackson [this message]
2012-02-24 18:54 ` [PATCH 09/15] libxl: Make libxl__zalloc et al tolerate a NULL gc Ian Jackson
2012-02-24 18:54 ` [PATCH 10/15] libxl: Introduce some convenience macros Ian Jackson
2012-02-24 18:54 ` [PATCH 11/15] libxl: Protect fds with CLOEXEC even with forking threads Ian Jackson
2012-02-24 18:55 ` [PATCH 12/15] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS Ian Jackson
2012-02-24 18:55 ` [PATCH 13/15] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
2012-02-24 18:55 ` [PATCH 14/15] libxl: Provide libxl_string_list_length Ian Jackson
2012-02-24 18:55 ` [PATCH 15/15] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
2012-02-24 18:57 ` [PATCH v2 00/15] libxl: child process handling Ian Jackson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1330109703-6536-9-git-send-email-ian.jackson@eu.citrix.com \
    --to=ian.jackson@eu.citrix.com \
    --cc=xen-devel@lists.xensource.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.