[PATCH v3 0/5] xenstore: fix memory leak of xenstored

[PATCH v3 0/5] xenstore: fix memory leak of xenstored

[Juergen Gross]

xenstored has a memory leak when setting watches: a no longer active
watch which fired in the past will still use some memory. This is
critical for long running connections to xenstored like the qemu
process serving as a qdisk backend for dom0. It will use some few
kB in xenstored for each domain create/destroy pair.

Fix this leak by using a temporary memory context for all allocations
in xenstored when firing a watch event.

Changes in V3:
- renamed temporary context parameter name and added comments in 
  patches 2-5 as requested by Wei Liu

Changes in V2:
- modified patch description as requested by Ian Jackson
- split up patch 2 as requested by Ian Jackson

Juergen Gross (5):
  xenstore: call each xenstored command function with temporary context
  xenstore: add explicit memory context parameter to get_parent()
  xenstore: add explicit memory context parameter to read_node()
  xenstore: add explicit memory context parameter to get_node()
  xenstore: use temporary memory context for firing watches

 tools/xenstore/xenstored_core.c        | 134 ++++++++++++++++++++-------------
 tools/xenstore/xenstored_core.h        |   4 +
 tools/xenstore/xenstored_domain.c      |  20 +++--
 tools/xenstore/xenstored_domain.h      |  10 +--
 tools/xenstore/xenstored_transaction.c |   5 +-
 tools/xenstore/xenstored_transaction.h |   2 +-
 tools/xenstore/xenstored_watch.c       |  22 ++++--
 tools/xenstore/xenstored_watch.h       |   3 +-
 8 files changed, 123 insertions(+), 77 deletions(-)

-- 
2.6.6


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

[PATCH v3 1/5] xenstore: call each xenstored command function with temporary context

[Juergen Gross]

In order to be able to avoid leaving temporary memory allocated after
processing of a command in xenstored call all command functions with
the temporary "in" context. Each function can then make use of that
temporary context for allocating temporary memory instead of either
leaving that memory allocated until the connection is dropped (or
even until end of xenstored) or freeing the memory itself.

This requires to modify the interfaces of the functions taking only
one argument from the connection by moving the call of onearg() into
the single functions. Other than that no functional change.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c        | 39 +++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h        |  3 +++
 tools/xenstore/xenstored_domain.c      | 14 +++++++-----
 tools/xenstore/xenstored_domain.h      | 10 ++++-----
 tools/xenstore/xenstored_transaction.c |  3 ++-
 tools/xenstore/xenstored_transaction.h |  2 +-
 6 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 098865f..94c809c 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -713,7 +713,7 @@ bool is_valid_nodename(const char *node)
 /* We expect one arg in the input: return NULL otherwise.
  * The payload must contain exactly one nul, at the end.
  */
-static const char *onearg(struct buffered_data *in)
+const char *onearg(struct buffered_data *in)
 {
 	if (!in->used || get_string(in, 0) != in->used)
 		return NULL;
@@ -761,9 +761,10 @@ bool check_event_node(const char *node)
 	return true;
 }
 
-static void send_directory(struct connection *conn, const char *name)
+static void send_directory(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_READ);
@@ -775,9 +776,10 @@ static void send_directory(struct connection *conn, const char *name)
 	send_reply(conn, XS_DIRECTORY, node->children, node->childlen);
 }
 
-static void do_read(struct connection *conn, const char *name)
+static void do_read(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_READ);
@@ -943,9 +945,10 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	send_ack(conn, XS_WRITE);
 }
 
-static void do_mkdir(struct connection *conn, const char *name)
+static void do_mkdir(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_WRITE);
@@ -1060,9 +1063,10 @@ static void internal_rm(const char *name)
 }
 
 
-static void do_rm(struct connection *conn, const char *name)
+static void do_rm(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_WRITE);
@@ -1094,9 +1098,10 @@ static void do_rm(struct connection *conn, const char *name)
 }
 
 
-static void do_get_perms(struct connection *conn, const char *name)
+static void do_get_perms(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 	char *strings;
 	unsigned int len;
 
@@ -1210,11 +1215,11 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 
 	switch (in->hdr.msg.type) {
 	case XS_DIRECTORY:
-		send_directory(conn, onearg(in));
+		send_directory(conn, in);
 		break;
 
 	case XS_READ:
-		do_read(conn, onearg(in));
+		do_read(conn, in);
 		break;
 
 	case XS_WRITE:
@@ -1222,15 +1227,15 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_MKDIR:
-		do_mkdir(conn, onearg(in));
+		do_mkdir(conn, in);
 		break;
 
 	case XS_RM:
-		do_rm(conn, onearg(in));
+		do_rm(conn, in);
 		break;
 
 	case XS_GET_PERMS:
-		do_get_perms(conn, onearg(in));
+		do_get_perms(conn, in);
 		break;
 
 	case XS_SET_PERMS:
@@ -1254,7 +1259,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_TRANSACTION_END:
-		do_transaction_end(conn, onearg(in));
+		do_transaction_end(conn, in);
 		break;
 
 	case XS_INTRODUCE:
@@ -1262,19 +1267,19 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_IS_DOMAIN_INTRODUCED:
-		do_is_domain_introduced(conn, onearg(in));
+		do_is_domain_introduced(conn, in);
 		break;
 
 	case XS_RELEASE:
-		do_release(conn, onearg(in));
+		do_release(conn, in);
 		break;
 
 	case XS_GET_DOMAIN_PATH:
-		do_get_domain_path(conn, onearg(in));
+		do_get_domain_path(conn, in);
 		break;
 
 	case XS_RESUME:
-		do_resume(conn, onearg(in));
+		do_resume(conn, in);
 		break;
 
 	case XS_SET_TARGET:
@@ -1282,7 +1287,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_RESET_WATCHES:
-		do_reset_watches(conn);
+		do_reset_watches(conn, in);
 		break;
 
 	default:
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3a497f7..5dbf9c8 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -122,6 +122,9 @@ struct node {
 	char *children;
 };
 
+/* Return the only argument in the input. */
+const char *onearg(struct buffered_data *in);
+
 /* Break input into vectors, return the number, fill in up to num of them. */
 unsigned int get_strings(struct buffered_data *data,
 			 char *vec[], unsigned int num);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 47b4f03..c66539a 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -453,8 +453,9 @@ void do_set_target(struct connection *conn, struct buffered_data *in)
 }
 
 /* domid */
-void do_release(struct connection *conn, const char *domid_str)
+void do_release(struct connection *conn, struct buffered_data *in)
 {
+	const char *domid_str = onearg(in);
 	struct domain *domain;
 	unsigned int domid;
 
@@ -490,10 +491,11 @@ void do_release(struct connection *conn, const char *domid_str)
 	send_ack(conn, XS_RELEASE);
 }
 
-void do_resume(struct connection *conn, const char *domid_str)
+void do_resume(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
 	unsigned int domid;
+	const char *domid_str = onearg(in);
 
 	if (!domid_str) {
 		send_error(conn, EINVAL);
@@ -527,9 +529,10 @@ void do_resume(struct connection *conn, const char *domid_str)
 	send_ack(conn, XS_RESUME);
 }
 
-void do_get_domain_path(struct connection *conn, const char *domid_str)
+void do_get_domain_path(struct connection *conn, struct buffered_data *in)
 {
 	char *path;
+	const char *domid_str = onearg(in);
 
 	if (!domid_str) {
 		send_error(conn, EINVAL);
@@ -543,10 +546,11 @@ void do_get_domain_path(struct connection *conn, const char *domid_str)
 	talloc_free(path);
 }
 
-void do_is_domain_introduced(struct connection *conn, const char *domid_str)
+void do_is_domain_introduced(struct connection *conn, struct buffered_data *in)
 {
 	int result;
 	unsigned int domid;
+	const char *domid_str = onearg(in);
 
 	if (!domid_str) {
 		send_error(conn, EINVAL);
@@ -563,7 +567,7 @@ void do_is_domain_introduced(struct connection *conn, const char *domid_str)
 }
 
 /* Allow guest to reset all watches */
-void do_reset_watches(struct connection *conn)
+void do_reset_watches(struct connection *conn, struct buffered_data *in)
 {
 	conn_delete_all_watches(conn);
 	conn_delete_all_transactions(conn);
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 83488ed..2554423 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -25,22 +25,22 @@ void handle_event(void);
 void do_introduce(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_is_domain_introduced(struct connection *conn, const char *domid_str);
+void do_is_domain_introduced(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_release(struct connection *conn, const char *domid_str);
+void do_release(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_resume(struct connection *conn, const char *domid_str);
+void do_resume(struct connection *conn, struct buffered_data *in);
 
 /* domid, target */
 void do_set_target(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_get_domain_path(struct connection *conn, const char *domid_str);
+void do_get_domain_path(struct connection *conn, struct buffered_data *in);
 
 /* Allow guest to reset all watches */
-void do_reset_watches(struct connection *conn);
+void do_reset_watches(struct connection *conn, struct buffered_data *in);
 
 void domain_init(void);
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index d0e4739..3cde26e 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -184,8 +184,9 @@ void do_transaction_start(struct connection *conn, struct buffered_data *in)
 	send_reply(conn, XS_TRANSACTION_START, id_str, strlen(id_str)+1);
 }
 
-void do_transaction_end(struct connection *conn, const char *arg)
+void do_transaction_end(struct connection *conn, struct buffered_data *in)
 {
+	const char *arg = onearg(in);
 	struct changed_node *i;
 	struct changed_domain *d;
 	struct transaction *trans;
diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
index cfeeae1..0c868ee 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -22,7 +22,7 @@
 struct transaction;
 
 void do_transaction_start(struct connection *conn, struct buffered_data *node);
-void do_transaction_end(struct connection *conn, const char *arg);
+void do_transaction_end(struct connection *conn, struct buffered_data *in);
 
 struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
 
-- 
2.6.6


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

[PATCH v3 2/5] xenstore: add explicit memory context parameter to get_parent()

[Juergen Gross]

Add a parameter to xenstored get_parent() function to explicitly
specify the memory context to be used for allocations. This will make
it easier to avoid memory leaks by using a context which is freed
soon.

When available use a temporary context when calling get_parent(),
otherwise mimic the old behavior by calling get_parent() with the same
argument for both parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 94c809c..f2c12ab 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -507,12 +507,16 @@ static enum xs_perm_type perm_for_conn(struct connection *conn,
 	return perms[0].perms & mask;
 }
 
-static char *get_parent(const char *node)
+/*
+ * Get name of node parent.
+ * Temporary memory allocations are done with ctx.
+ */
+static char *get_parent(const void *ctx, const char *node)
 {
 	char *slash = strrchr(node + 1, '/');
 	if (!slash)
-		return talloc_strdup(node, "/");
-	return talloc_asprintf(node, "%.*s", (int)(slash - node), node);
+		return talloc_strdup(ctx, "/");
+	return talloc_asprintf(ctx, "%.*s", (int)(slash - node), node);
 }
 
 /* What do parents say? */
@@ -521,7 +525,7 @@ static enum xs_perm_type ask_parents(struct connection *conn, const char *name)
 	struct node *node;
 
 	do {
-		name = get_parent(name);
+		name = get_parent(name, name);
 		node = read_node(conn, name);
 		if (node)
 			break;
@@ -816,7 +820,7 @@ static struct node *construct_node(struct connection *conn, const char *name)
 	const char *base;
 	unsigned int baselen;
 	struct node *parent, *node;
-	char *children, *parentname = get_parent(name);
+	char *children, *parentname = get_parent(name, name);
 
 	/* If parent doesn't exist, create it. */
 	parent = read_node(conn, parentname);
@@ -1036,7 +1040,7 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 	/* Delete from parent first, then if we crash, the worst that can
 	   happen is the child will continue to take up space, but will
 	   otherwise be unreachable. */
-	struct node *parent = read_node(conn, get_parent(name));
+	struct node *parent = read_node(conn, get_parent(name, name));
 	if (!parent) {
 		send_error(conn, EINVAL);
 		return 0;
@@ -1073,7 +1077,7 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
-			node = read_node(conn, get_parent(name));
+			node = read_node(conn, get_parent(in, name));
 			if (node) {
 				send_ack(conn, XS_RM);
 				return;
-- 
2.6.6


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

[PATCH v3 3/5] xenstore: add explicit memory context parameter to read_node()

[Juergen Gross]

Add a parameter to xenstored read_node() function to explicitly
specify the memory context to be used for allocations. This will make
it easier to avoid memory leaks by using a context which is freed
soon.

When calling read_node() select a sensible memory context for the new
parameter by preferring a temporary one.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f2c12ab..c462115 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -397,8 +397,12 @@ bool is_child(const char *child, const char *parent)
 	return child[len] == '/' || child[len] == '\0';
 }
 
-/* If it fails, returns NULL and sets errno. */
-static struct node *read_node(struct connection *conn, const char *name)
+/*
+ * If it fails, returns NULL and sets errno.
+ * Temporary memory allocations will be done with ctx.
+ */
+static struct node *read_node(struct connection *conn, const void *ctx,
+			      const char *name)
 {
 	TDB_DATA key, data;
 	uint32_t *p;
@@ -419,7 +423,7 @@ static struct node *read_node(struct connection *conn, const char *name)
 		return NULL;
 	}
 
-	node = talloc(name, struct node);
+	node = talloc(ctx, struct node);
 	node->name = talloc_strdup(node, name);
 	node->parent = NULL;
 	node->tdb = tdb_context(conn);
@@ -526,7 +530,7 @@ static enum xs_perm_type ask_parents(struct connection *conn, const char *name)
 
 	do {
 		name = get_parent(name, name);
-		node = read_node(conn, name);
+		node = read_node(conn, name, name);
 		if (node)
 			break;
 	} while (!streq(name, "/"));
@@ -567,7 +571,7 @@ struct node *get_node(struct connection *conn,
 		errno = EINVAL;
 		return NULL;
 	}
-	node = read_node(conn, name);
+	node = read_node(conn, name, name);
 	/* If we don't have permission, we don't have node. */
 	if (node) {
 		if ((perm_for_conn(conn, node->perms, node->num_perms) & perm)
@@ -823,7 +827,7 @@ static struct node *construct_node(struct connection *conn, const char *name)
 	char *children, *parentname = get_parent(name, name);
 
 	/* If parent doesn't exist, create it. */
-	parent = read_node(conn, parentname);
+	parent = read_node(conn, parentname, parentname);
 	if (!parent)
 		parent = construct_node(conn, parentname);
 	if (!parent)
@@ -988,7 +992,7 @@ static void delete_node(struct connection *conn, struct node *node)
 	for (i = 0; i < node->childlen; i += strlen(node->children+i) + 1) {
 		struct node *child;
 
-		child = read_node(conn, 
+		child = read_node(conn, node,
 				  talloc_asprintf(node, "%s/%s", node->name,
 						  node->children + i));
 		if (child) {
@@ -1040,7 +1044,7 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 	/* Delete from parent first, then if we crash, the worst that can
 	   happen is the child will continue to take up space, but will
 	   otherwise be unreachable. */
-	struct node *parent = read_node(conn, get_parent(name, name));
+	struct node *parent = read_node(conn, name, get_parent(name, name));
 	if (!parent) {
 		send_error(conn, EINVAL);
 		return 0;
@@ -1059,7 +1063,7 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 static void internal_rm(const char *name)
 {
 	char *tname = talloc_strdup(NULL, name);
-	struct node *node = read_node(NULL, tname);
+	struct node *node = read_node(NULL, tname, tname);
 	if (node)
 		_rm(NULL, node, tname);
 	talloc_free(node);
@@ -1077,7 +1081,7 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
-			node = read_node(conn, get_parent(in, name));
+			node = read_node(conn, in, get_parent(in, name));
 			if (node) {
 				send_ack(conn, XS_RM);
 				return;
@@ -1608,7 +1612,7 @@ static void remember_string(struct hashtable *hash, const char *str)
  */
 static void check_store_(const char *name, struct hashtable *reachable)
 {
-	struct node *node = read_node(NULL, name);
+	struct node *node = read_node(NULL, name, name);
 
 	if (node) {
 		size_t i = 0;
@@ -1622,7 +1626,8 @@ static void check_store_(const char *name, struct hashtable *reachable)
 			size_t childlen = strlen(node->children + i);
 			char * childname = child_name(node->name,
 						      node->children + i);
-			struct node *childnode = read_node(NULL, childname);
+			struct node *childnode = read_node(NULL, childname,
+							   childname);
 			
 			if (childnode) {
 				if (hashtable_search(children, childname)) {
-- 
2.6.6


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

[PATCH v3 4/5] xenstore: add explicit memory context parameter to get_node()

[Juergen Gross]

Add a parameter to xenstored get_node() function to explicitly
specify the memory context to be used for allocations. This will make
it easier to avoid memory leaks by using a context which is freed
soon.

This requires adding the temporary context to errno_from_parents() and
ask_parents(), too.

When calling get_node() select a sensible memory context for the new
parameter by preferring a temporary one.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c  | 50 +++++++++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h  |  1 +
 tools/xenstore/xenstored_watch.c |  2 +-
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c462115..4239410 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -523,14 +523,18 @@ static char *get_parent(const void *ctx, const char *node)
 	return talloc_asprintf(ctx, "%.*s", (int)(slash - node), node);
 }
 
-/* What do parents say? */
-static enum xs_perm_type ask_parents(struct connection *conn, const char *name)
+/*
+ * What do parents say?
+ * Temporary memory allocations are done with ctx.
+ */
+static enum xs_perm_type ask_parents(struct connection *conn, const void *ctx,
+				     const char *name)
 {
 	struct node *node;
 
 	do {
-		name = get_parent(name, name);
-		node = read_node(conn, name, name);
+		name = get_parent(ctx, name);
+		node = read_node(conn, ctx, name);
 		if (node)
 			break;
 	} while (!streq(name, "/"));
@@ -544,24 +548,32 @@ static enum xs_perm_type ask_parents(struct connection *conn, const char *name)
 	return perm_for_conn(conn, node->perms, node->num_perms);
 }
 
-/* We have a weird permissions system.  You can allow someone into a
+/*
+ * We have a weird permissions system.  You can allow someone into a
  * specific node without allowing it in the parents.  If it's going to
  * fail, however, we don't want the errno to indicate any information
- * about the node. */
-static int errno_from_parents(struct connection *conn, const char *node,
-			      int errnum, enum xs_perm_type perm)
+ * about the node.
+ * Temporary memory allocations are done with ctx.
+ */
+static int errno_from_parents(struct connection *conn, const void *ctx,
+			      const char *node, int errnum,
+			      enum xs_perm_type perm)
 {
 	/* We always tell them about memory failures. */
 	if (errnum == ENOMEM)
 		return errnum;
 
-	if (ask_parents(conn, node) & perm)
+	if (ask_parents(conn, ctx, node) & perm)
 		return errnum;
 	return EACCES;
 }
 
-/* If it fails, returns NULL and sets errno. */
+/*
+ * If it fails, returns NULL and sets errno.
+ * Temporary memory allocations are done with ctx.
+ */
 struct node *get_node(struct connection *conn,
+		      const void *ctx,
 		      const char *name,
 		      enum xs_perm_type perm)
 {
@@ -571,7 +583,7 @@ struct node *get_node(struct connection *conn,
 		errno = EINVAL;
 		return NULL;
 	}
-	node = read_node(conn, name, name);
+	node = read_node(conn, ctx, name);
 	/* If we don't have permission, we don't have node. */
 	if (node) {
 		if ((perm_for_conn(conn, node->perms, node->num_perms) & perm)
@@ -582,7 +594,7 @@ struct node *get_node(struct connection *conn,
 	}
 	/* Clean up errno if they weren't supposed to know. */
 	if (!node) 
-		errno = errno_from_parents(conn, name, errno, perm);
+		errno = errno_from_parents(conn, ctx, name, errno, perm);
 	return node;
 }
 
@@ -775,7 +787,7 @@ static void send_directory(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_READ);
+	node = get_node(conn, in, name, XS_PERM_READ);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -790,7 +802,7 @@ static void do_read(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_READ);
+	node = get_node(conn, in, name, XS_PERM_READ);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -927,7 +939,7 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	datalen = in->used - offset;
 
 	name = canonicalize(conn, vec[0]);
-	node = get_node(conn, name, XS_PERM_WRITE);
+	node = get_node(conn, in, name, XS_PERM_WRITE);
 	if (!node) {
 		/* No permissions, invalid input? */
 		if (errno != ENOENT) {
@@ -959,7 +971,7 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_WRITE);
+	node = get_node(conn, in, name, XS_PERM_WRITE);
 
 	/* If it already exists, fine. */
 	if (!node) {
@@ -1077,7 +1089,7 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_WRITE);
+	node = get_node(conn, in, name, XS_PERM_WRITE);
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
@@ -1114,7 +1126,7 @@ static void do_get_perms(struct connection *conn, struct buffered_data *in)
 	unsigned int len;
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_READ);
+	node = get_node(conn, in, name, XS_PERM_READ);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -1146,7 +1158,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 	num--;
 
 	/* We must own node to do this (tools can do this too). */
-	node = get_node(conn, name, XS_PERM_WRITE|XS_PERM_OWNER);
+	node = get_node(conn, in, name, XS_PERM_WRITE|XS_PERM_OWNER);
 	if (!node) {
 		send_error(conn, errno);
 		return;
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 5dbf9c8..ecc614f 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -149,6 +149,7 @@ bool check_event_node(const char *node);
 
 /* Get this node, checking we have permissions. */
 struct node *get_node(struct connection *conn,
+		      const void *ctx,
 		      const char *name,
 		      enum xs_perm_type perm);
 
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 8543999..beefd6c 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -57,7 +57,7 @@ static void add_event(struct connection *conn,
 
 	if (!check_event_node(name)) {
 		/* Can this conn load node, or see that it doesn't exist? */
-		struct node *node = get_node(conn, name, XS_PERM_READ);
+		struct node *node = get_node(conn, name, name, XS_PERM_READ);
 		/*
 		 * XXX We allow EACCES here because otherwise a non-dom0
 		 * backend driver cannot watch for disappearance of a frontend
-- 
2.6.6


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

[PATCH v3 5/5] xenstore: use temporary memory context for firing watches

[Juergen Gross]

Use a temporary memory context for memory allocations when firing
watches. This will avoid leaking memory in case of long living
connections and/or xenstore entries.

This requires adding a new parameter to fire_watches() and add_event()
to specify the memory context to use for allocations.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c        |  8 ++++----
 tools/xenstore/xenstored_domain.c      |  6 +++---
 tools/xenstore/xenstored_transaction.c |  2 +-
 tools/xenstore/xenstored_watch.c       | 22 ++++++++++++++++------
 tools/xenstore/xenstored_watch.h       |  3 ++-
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 4239410..1232169 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -961,7 +961,7 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	}
 
 	add_change_node(conn->transaction, name, false);
-	fire_watches(conn, name, false);
+	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_WRITE);
 }
 
@@ -986,7 +986,7 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 			return;
 		}
 		add_change_node(conn->transaction, name, false);
-		fire_watches(conn, name, false);
+		fire_watches(conn, in, name, false);
 	}
 	send_ack(conn, XS_MKDIR);
 }
@@ -1112,7 +1112,7 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 
 	if (_rm(conn, node, name)) {
 		add_change_node(conn->transaction, name, true);
-		fire_watches(conn, name, true);
+		fire_watches(conn, in, name, true);
 		send_ack(conn, XS_RM);
 	}
 }
@@ -1188,7 +1188,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 	}
 
 	add_change_node(conn->transaction, name, false);
-	fire_watches(conn, name, false);
+	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_SET_PERMS);
 }
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index c66539a..5de93d4 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -204,7 +204,7 @@ static int destroy_domain(void *_domain)
 			unmap_interface(domain->interface);
 	}
 
-	fire_watches(NULL, "@releaseDomain", false);
+	fire_watches(NULL, domain, "@releaseDomain", false);
 
 	return 0;
 }
@@ -232,7 +232,7 @@ static void domain_cleanup(void)
 	}
 
 	if (notify)
-		fire_watches(NULL, "@releaseDomain", false);
+		fire_watches(NULL, NULL, "@releaseDomain", false);
 }
 
 /* We scan all domains rather than use the information given here. */
@@ -389,7 +389,7 @@ void do_introduce(struct connection *conn, struct buffered_data *in)
 		/* Now domain belongs to its connection. */
 		talloc_steal(domain->conn, domain);
 
-		fire_watches(NULL, "@introduceDomain", false);
+		fire_watches(NULL, in, "@introduceDomain", false);
 	} else if ((domain->mfn == mfn) && (domain->conn != conn)) {
 		/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
 		if (domain->port)
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 3cde26e..34720fa 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -227,7 +227,7 @@ void do_transaction_end(struct connection *conn, struct buffered_data *in)
 
 		/* Fire off the watches for everything that changed. */
 		list_for_each_entry(i, &trans->changes, list)
-			fire_watches(conn, i->node, i->recurse);
+			fire_watches(conn, in, i->node, i->recurse);
 		generation++;
 	}
 	send_ack(conn, XS_TRANSACTION_END);
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index beefd6c..856750e 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -47,7 +47,12 @@ struct watch
 	char *node;
 };
 
+/*
+ * Send a watch event.
+ * Temporary memory allocations are done with ctx.
+ */
 static void add_event(struct connection *conn,
+		      void *ctx,
 		      struct watch *watch,
 		      const char *name)
 {
@@ -57,7 +62,7 @@ static void add_event(struct connection *conn,
 
 	if (!check_event_node(name)) {
 		/* Can this conn load node, or see that it doesn't exist? */
-		struct node *node = get_node(conn, name, name, XS_PERM_READ);
+		struct node *node = get_node(conn, ctx, name, XS_PERM_READ);
 		/*
 		 * XXX We allow EACCES here because otherwise a non-dom0
 		 * backend driver cannot watch for disappearance of a frontend
@@ -78,14 +83,19 @@ static void add_event(struct connection *conn,
 	}
 
 	len = strlen(name) + 1 + strlen(watch->token) + 1;
-	data = talloc_array(watch, char, len);
+	data = talloc_array(ctx, char, len);
 	strcpy(data, name);
 	strcpy(data + strlen(name) + 1, watch->token);
 	send_reply(conn, XS_WATCH_EVENT, data, len);
 	talloc_free(data);
 }
 
-void fire_watches(struct connection *conn, const char *name, bool recurse)
+/*
+ * Check whether any watch events are to be sent.
+ * Temporary memory allocations are done with ctx.
+ */
+void fire_watches(struct connection *conn, void *ctx, const char *name,
+		  bool recurse)
 {
 	struct connection *i;
 	struct watch *watch;
@@ -98,9 +108,9 @@ void fire_watches(struct connection *conn, const char *name, bool recurse)
 	list_for_each_entry(i, &connections, list) {
 		list_for_each_entry(watch, &i->watches, list) {
 			if (is_child(name, watch->node))
-				add_event(i, watch, name);
+				add_event(i, ctx, watch, name);
 			else if (recurse && is_child(watch->node, name))
-				add_event(i, watch, watch->node);
+				add_event(i, ctx, watch, watch->node);
 		}
 	}
 }
@@ -169,7 +179,7 @@ void do_watch(struct connection *conn, struct buffered_data *in)
 	send_ack(conn, XS_WATCH);
 
 	/* We fire once up front: simplifies clients and restart. */
-	add_event(conn, watch, watch->node);
+	add_event(conn, in, watch, watch->node);
 }
 
 void do_unwatch(struct connection *conn, struct buffered_data *in)
diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h
index 5bc4f88..8ed1dde 100644
--- a/tools/xenstore/xenstored_watch.h
+++ b/tools/xenstore/xenstored_watch.h
@@ -25,7 +25,8 @@ void do_watch(struct connection *conn, struct buffered_data *in);
 void do_unwatch(struct connection *conn, struct buffered_data *in);
 
 /* Fire all watches: recurse means all the children are affected (ie. rm). */
-void fire_watches(struct connection *conn, const char *name, bool recurse);
+void fire_watches(struct connection *conn, void *tmp, const char *name,
+		  bool recurse);
 
 void dump_watches(struct connection *conn);
 
-- 
2.6.6


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

Re: [PATCH v3 0/5] xenstore: fix memory leak of xenstored

[Wei Liu]

Queued. Thanks.

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