|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] [v3] libxl: Add API to retrieve domain console tty [and 2 more messages]
Bamvor Jian Zhang writes ("[PATCH] [v3] libxl: Add API to retrieve
domain consol
> BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0,
> tty) will lead to null pointer access in CTX maco.
So once again, well done for spotting this.
During some private discussion with Ian Campbell I came up with the
a proposal to fix it, which I have now implemented in the form of the
patch below.
Please comment.
NB this patch cannot be applied to xen-unstable.hg staging tip. It
needs to go in the middle of my save/restore series. I think this is
a sufficiently off-the-mainline bug (it will only show up with
allocation errors) that it's better to avoid introducing any more
merge conflicts than absolutely necessary. And doing it this way
avoids my series ending up with a tip containing callers which wrongly
pass 0 for gc_opt.
Thanks,
Ian.
From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Subject: [PATCH] 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.
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>
Cc: 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 | 36 +++++++++++++++++++++++-------------
tools/libxl/libxl_utils.c | 6 ++----
tools/libxl/libxl_xshelp.c | 7 ++-----
10 files changed, 47 insertions(+), 30 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 53626ae..6a0fa85 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 eaf378b..431791a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -107,7 +107,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 13d2fc1..875aaea 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()) */
@@ -2111,6 +2120,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 (&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 2382c9d..3a5f292 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)
--
tg: (e2dea8e..) t/xen/xl.nogc (depends on: t/xen/xl.savefile.async)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |