libxl: Do not pass NULL as gc_opt; introduce NOGC

In 25182:6c3345d7e9d9 the practice of passing NULL to gc-using memory
allocation functions was introduced.  However, the arrangements there
were not correct as committed, because the error handling and logging
depends on getting a ctx from the gc - so an allocation error would in
fact result in libxl dereferencing NULL.

Instead, provide a special dummy gc in the ctx, called `nogc_gc'.  It
is marked out specially by having alloc_maxsize==-1, which is
otherwise invalid.

Functions which need to actually look into the gc use the new test
function gc_is_real (whose purpose is mainly clarity of the code) to
check whether the gc is the dummy one, and do nothing if it is.  And
we provide a helper macro NOGC which uses the in-scope real gc to find
the ctx and hence the dummy gc (and which replaces the previous
#define NOGC NULL).

Change all callers which pass 0 or NULL to an allocation function to
use NOGC or &ctx->nogc_gc, as applicable in the context.

We add a comment near the definition of LIBXL_INIT_GC pointing out
that it isn't any more the only place a libxl__gc struct is
initialised, for the benefit of anyone changing the contents of gc's
in the future.

Also, actually document that libxl__ptr_add is legal with ptr==NULL,
and change a couple of calls not to check for NULL argument.

Reported-by: Bamvor Jian Zhang <bjzhang@suse.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

10 files changed, 47 insertions, 31 deletions

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4e831427e8..3d8441e67e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -41,6 +41,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 
     /* First initialise pointers etc. (cannot fail) */
 
+    ctx->nogc_gc.alloc_maxsize = -1;
+    ctx->nogc_gc.owner = ctx;
+
     LIBXL_TAILQ_INIT(&ctx->occurred);
 
     ctx->osevent_hooks = 0;
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 7f8d6d3bf6..99972a23b0 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -77,6 +77,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
 void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
                                   const void *data, size_t len)
 {
+    EGC_GC;
     libxl__datacopier_buf *buf;
     /*
      * It is safe for this to be called immediately after _start, as
@@ -88,7 +89,7 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
 
     assert(len < dc->maxsz - dc->used);
 
-    buf = libxl__zalloc(0, sizeof(*buf) - sizeof(buf->buf) + len);
+    buf = libxl__zalloc(NOGC, sizeof(*buf) - sizeof(buf->buf) + len);
     buf->used = len;
     memcpy(buf->buf, data, len);
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 93f0b12461..f46a2b3388 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -165,7 +165,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     }
 
     if (b_info->blkdev_start == NULL)
-        b_info->blkdev_start = libxl__strdup(0, "xvda");
+        b_info->blkdev_start = libxl__strdup(NOGC, "xvda");
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!b_info->u.hvm.bios)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 565d2c2dfc..eb23a93887 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -772,7 +772,7 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
         if (poller->fd_rindices_allocd < maxfd) {
             assert(ARRAY_SIZE_OK(poller->fd_rindices, maxfd));
             poller->fd_rindices =
-                libxl__realloc(0, poller->fd_rindices,
+                libxl__realloc(NOGC, poller->fd_rindices,
                                maxfd * sizeof(*poller->fd_rindices));
             memset(poller->fd_rindices + poller->fd_rindices_allocd,
                    0,
@@ -1099,9 +1099,10 @@ void libxl_event_free(libxl_ctx *ctx, libxl_event *event)
 libxl_event *libxl__event_new(libxl__egc *egc,
                               libxl_event_type type, uint32_t domid)
 {
+    EGC_GC;
     libxl_event *ev;
 
-    ev = libxl__zalloc(0,sizeof(*ev));
+    ev = libxl__zalloc(NOGC,sizeof(*ev));
     ev->type = type;
     ev->domid = domid;
 
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 082bf44ced..cfa379c7e6 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -280,7 +280,7 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     int status, rc;
 
     libxl__spawn_init(ss);
-    ss->ssd = libxl__zalloc(0, sizeof(*ss->ssd));
+    ss->ssd = libxl__zalloc(NOGC, sizeof(*ss->ssd));
     libxl__ev_child_init(&ss->ssd->mid);
 
     rc = libxl__ev_time_register_rel(gc, &ss->timeout,
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 9ff99e0bfb..044ddad139 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -92,7 +92,7 @@ libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd)
     libxl__carefd *cf = 0;
 
     libxl_fd_set_cloexec(ctx, fd, 1);
-    cf = libxl__zalloc(NULL, sizeof(*cf));
+    cf = libxl__zalloc(&ctx->nogc_gc, sizeof(*cf));
     cf->fd = fd;
     LIBXL_LIST_INSERT_HEAD(&carefds, cf, entry);
     return cf;
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 8139520b78..fbff7d06d9 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -30,11 +30,16 @@ void libxl__alloc_failed(libxl_ctx *ctx, const char *func,
 #undef L
 }
 
+static int gc_is_real(const libxl__gc *gc)
+{
+    return gc->alloc_maxsize >= 0;
+}
+
 void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
     int i;
 
-    if (!gc)
+    if (!gc_is_real(gc))
         return;
 
     if (!ptr)
@@ -66,6 +71,8 @@ void libxl__free_all(libxl__gc *gc)
     void *ptr;
     int i;
 
+    assert(gc_is_real(gc));
+
     for (i = 0; i < gc->alloc_maxsize; i++) {
         ptr = gc->alloc_ptrs[i];
         gc->alloc_ptrs[i] = NULL;
@@ -104,7 +111,7 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
 
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
-    } else if (new_ptr != ptr && gc != NULL) {
+    } else if (new_ptr != ptr && gc_is_real(gc)) {
         for (i = 0; i < gc->alloc_maxsize; i++) {
             if (gc->alloc_ptrs[i] == ptr) {
                 gc->alloc_ptrs[i] = new_ptr;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9ca5c2cb44..14d46d1d83 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -277,10 +277,18 @@ struct libxl__poller {
     int wakeup_pipe[2]; /* 0 means no fd allocated */
 };
 
+struct libxl__gc {
+    /* mini-GC */
+    int alloc_maxsize; /* -1 means this is the dummy non-gc gc */
+    void **alloc_ptrs;
+    libxl_ctx *owner;
+};
+
 struct libxl__ctx {
     xentoollog_logger *lg;
     xc_interface *xch;
     struct xs_handle *xsh;
+    libxl__gc nogc_gc;
 
     const libxl_event_hooks *event_hooks;
     void *event_hooks_user;
@@ -356,13 +364,6 @@ typedef struct {
 
 #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
 
-struct libxl__gc {
-    /* mini-GC */
-    int alloc_maxsize;
-    void **alloc_ptrs;
-    libxl_ctx *owner;
-};
-
 struct libxl__egc {
     /* For event-generating functions only.
      * The egc and its gc may be accessed only on the creating thread. */
@@ -420,6 +421,7 @@ struct libxl__ao {
         (gc).alloc_ptrs = 0;                    \
         (gc).owner = (ctx);                     \
     } while(0)
+    /* NB, also, a gc struct ctx->nogc_gc is initialised in libxl_ctx_alloc */
 
 static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
 {
@@ -438,13 +440,20 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
  * All pointers returned by these functions are registered for garbage
  * collection on exit from the outermost libxl callframe.
  *
- * However, where the argument is stated to be "gc_opt", NULL may be
- * passed instead, in which case no garbage collection will occur; the
- * pointer must later be freed with free().  This is for memory
- * allocations of types (b) and (c).
+ * However, where the argument is stated to be "gc_opt", &ctx->nogc_gc
+ * may be passed instead, in which case no garbage collection will
+ * occur; the pointer must later be freed with free().  (Passing NULL
+ * for gc_opt is not permitted.)  This is for memory allocations of
+ * types (b) and (c).  The convenience macro NOGC should be used where
+ * possible.
+ *
+ * NOGC (and ctx->nogc_gc) may ONLY be used with functions which
+ * explicitly declare that it's OK.  Use with nonconsenting functions
+ * may result in leaks of those functions' internal allocations on the
+ * psuedo-gc.
  */
-/* register @ptr in @gc for free on exit from outermost libxl callframe. */
-_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr);
+/* register ptr in gc for free on exit from outermost libxl callframe. */
+_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr /* may be NULL */);
 /* 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()) */
@@ -2110,7 +2119,7 @@ _hidden const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid);
 #define GC_INIT(ctx)  libxl__gc gc[1]; LIBXL_INIT_GC(gc[0],ctx)
 #define GC_FREE       libxl__free_all(gc)
 #define CTX           libxl__gc_owner(gc)
-#define NOGC          NULL
+#define NOGC          (&CTX->nogc_gc) /* pass only to consenting functions */
 
 /* Allocation macros all of which use the gc. */
 
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 707306213c..f69a24c267 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -58,8 +58,7 @@ char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
 char *libxl__domid_to_name(libxl__gc *gc, uint32_t domid)
 {
     char *s = libxl_domid_to_name(libxl__gc_owner(gc), domid);
-    if ( s )
-        libxl__ptr_add(gc, s);
+    libxl__ptr_add(gc, s);
     return s;
 }
 
@@ -107,8 +106,7 @@ char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid)
 char *libxl__cpupoolid_to_name(libxl__gc *gc, uint32_t poolid)
 {
     char *s = libxl_cpupoolid_to_name(libxl__gc_owner(gc), poolid);
-    if ( s )
-        libxl__ptr_add(gc, s);
+    libxl__ptr_add(gc, s);
     return s;
 }
 
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 993f527c58..7fdf164c42 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -86,11 +86,8 @@ char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
     char *ptr;
 
     ptr = xs_read(ctx->xsh, t, path, NULL);
-    if (ptr != NULL) {
-        libxl__ptr_add(gc, ptr);
-        return ptr;
-    }
-    return 0;
+    libxl__ptr_add(gc, ptr);
+    return ptr;
 }
 
 char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid)