[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 13/19] libxl: Do not pass NULL as gc_opt; introduce NOGC



On Fri, 2012-06-08 at 18:34 +0100, Ian Jackson wrote:
> 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@xxxxxxxx>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Bamvor Jian Zhang <bjzhang@xxxxxxxx>

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

> ---
>  tools/libxl/libxl.c          |    3 +++
>  tools/libxl/libxl_aoutils.c  |    3 ++-
>  tools/libxl/libxl_create.c   |    2 +-
>  tools/libxl/libxl_event.c    |    5 +++--
>  tools/libxl/libxl_exec.c     |    2 +-
>  tools/libxl/libxl_fork.c     |    2 +-
>  tools/libxl/libxl_internal.c |   11 +++++++++--
>  tools/libxl/libxl_internal.h |   37 +++++++++++++++++++++++--------------
>  tools/libxl/libxl_utils.c    |    6 ++----
>  tools/libxl/libxl_xshelp.c   |    7 ++-----
>  10 files changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2a31528..a257902 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 7f8d6d3..99972a2 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 00705d8..ab000bc 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -108,7 +108,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 565d2c2..eb23a93 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 082bf44..cfa379c 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 9ff99e0..044ddad 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 8139520..fbff7d0 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 f90814a..e69482c 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 67ef82c..08c7dac 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 993f527..7fdf164 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)



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.