patch-0012-xenstore-handle-memory-allocation-failures-in-xensto.patch

From 8020a9588c32ecdd472669c8ea17be8931777084 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Mon, 5 Dec 2016 08:48:53 +0100
Subject: [PATCH 12/25] xenstore: handle memory allocation failures in
 xenstored

Check for failures when allocating new memory in xenstored.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c        | 212 +++++++++++++++++++------
 tools/xenstore/xenstored_domain.c      |  10 ++
 tools/xenstore/xenstored_transaction.c |  26 +++
 tools/xenstore/xenstored_watch.c       |  12 ++
 4 files changed, 212 insertions(+), 48 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 537967127034..79aab5f718ed 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -149,8 +149,10 @@ void trace(const char *fmt, ...)
 	va_start(arglist, fmt);
 	str = talloc_vasprintf(NULL, fmt, arglist);
 	va_end(arglist);
-	dummy = write(tracefd, str, strlen(str));
-	talloc_free(str);
+	if (str) {
+		dummy = write(tracefd, str, strlen(str));
+		talloc_free(str);
+	}
 }
 
 static void trace_io(const struct connection *conn,
@@ -397,7 +399,16 @@ static struct node *read_node(struct connection *conn, const void *ctx,
 	}
 
 	node = talloc(ctx, struct node);
+	if (!node) {
+		errno = ENOMEM;
+		return NULL;
+	}
 	node->name = talloc_strdup(node, name);
+	if (!node->name) {
+		talloc_free(node);
+		errno = ENOMEM;
+		return NULL;
+	}
 	node->parent = NULL;
 	node->tdb = tdb_context(conn);
 	talloc_steal(node, data.dptr);
@@ -495,35 +506,46 @@ static enum xs_perm_type perm_for_conn(struct connection *conn,
  */
 static char *get_parent(const void *ctx, const char *node)
 {
+	char *parent;
 	char *slash = strrchr(node + 1, '/');
-	if (!slash)
-		return talloc_strdup(ctx, "/");
-	return talloc_asprintf(ctx, "%.*s", (int)(slash - node), node);
+
+	parent = slash ? talloc_asprintf(ctx, "%.*s", (int)(slash - node), node)
+		       : talloc_strdup(ctx, "/");
+	if (!parent)
+		errno = ENOMEM;
+
+	return parent;
 }
 
 /*
  * 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)
+static int ask_parents(struct connection *conn, const void *ctx,
+		       const char *name, enum xs_perm_type *perm)
 {
 	struct node *node;
 
 	do {
 		name = get_parent(ctx, name);
+		if (!name)
+			return errno;
 		node = read_node(conn, ctx, name);
 		if (node)
 			break;
+		if (errno == ENOMEM)
+			return errno;
 	} while (!streq(name, "/"));
 
 	/* No permission at root?  We're in trouble. */
 	if (!node) {
 		corrupt(conn, "No permissions file at root");
-		return XS_PERM_NONE;
+		*perm = XS_PERM_NONE;
+		return 0;
 	}
 
-	return perm_for_conn(conn, node->perms, node->num_perms);
+	*perm = perm_for_conn(conn, node->perms, node->num_perms);
+	return 0;
 }
 
 /*
@@ -537,11 +559,15 @@ static int errno_from_parents(struct connection *conn, const void *ctx,
 			      const char *node, int errnum,
 			      enum xs_perm_type perm)
 {
+	enum xs_perm_type parent_perm = XS_PERM_NONE;
+
 	/* We always tell them about memory failures. */
 	if (errnum == ENOMEM)
 		return errnum;
 
-	if (ask_parents(conn, ctx, node) & perm)
+	if (ask_parents(conn, ctx, node, &parent_perm))
+		return errno;
+	if (parent_perm & perm)
 		return errnum;
 	return EACCES;
 }
@@ -571,7 +597,7 @@ struct node *get_node(struct connection *conn,
 		}
 	}
 	/* Clean up errno if they weren't supposed to know. */
-	if (!node) 
+	if (!node && errno != ENOMEM)
 		errno = errno_from_parents(conn, ctx, name, errno, perm);
 	return node;
 }
@@ -661,11 +687,29 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 	} else {
 		/* Message is a child of the connection for auto-cleanup. */
 		bdata = new_buffer(conn);
+
+		/*
+		 * Allocation failure here is unfortunate: we have no way to
+		 * tell anybody about it.
+		 */
+		if (!bdata)
+			return;
 	}
 	if (len <= DEFAULT_BUFFER_SIZE)
 		bdata->buffer = bdata->default_buffer;
 	else
 		bdata->buffer = talloc_array(bdata, char, len);
+	if (!bdata->buffer) {
+		if (type == XS_WATCH_EVENT) {
+			/* Same as above: no way to tell someone. */
+			talloc_free(bdata);
+			return;
+		}
+		/* re-establish request buffer for sending ENOMEM. */
+		conn->in = bdata;
+		send_error(conn, ENOMEM);
+		return;
+	}
 
 	/* Update relevant header fields and fill in the message body. */
 	bdata->hdr.msg.type = type;
@@ -674,6 +718,8 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 
 	/* Queue for later transmission. */
 	list_add_tail(&bdata->list, &conn->out_list);
+
+	return;
 }
 
 /* Some routines (write, mkdir, etc) just need a non-error return */
@@ -735,6 +781,8 @@ static char *perms_to_strings(const void *ctx,
 
 		strings = talloc_realloc(ctx, strings, char,
 					 *len + strlen(buffer) + 1);
+		if (!strings)
+			return NULL;
 		strcpy(strings + *len, buffer);
 		*len += strlen(buffer) + 1;
 	}
@@ -881,6 +929,9 @@ static struct node *construct_node(struct connection *conn, const void *ctx,
 	struct node *parent, *node;
 	char *children, *parentname = get_parent(ctx, name);
 
+	if (!parentname)
+		return NULL;
+
 	/* If parent doesn't exist, create it. */
 	parent = read_node(conn, parentname, parentname);
 	if (!parent)
@@ -1030,6 +1081,7 @@ static void delete_node(struct connection *conn, struct node *node,
 			bool changed)
 {
 	unsigned int i;
+	char *name;
 
 	/* Delete self, then delete children.  If we crash, then the worst
 	   that can happen is the children will continue to take up space, but
@@ -1040,17 +1092,18 @@ 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, node,
-				  talloc_asprintf(node, "%s/%s", node->name,
-						  node->children + i));
+		name = talloc_asprintf(node, "%s/%s", node->name,
+				       node->children + i);
+		child = name ? read_node(conn, node, name) : NULL;
 		if (child) {
 			delete_node(conn, child, false);
 		}
 		else {
-			trace("delete_node: No child '%s/%s' found!\n",
+			trace("delete_node: Error deleting child '%s/%s'!\n",
 			      node->name, node->children + i);
 			/* Skip it, we've already deleted the parent. */
 		}
+		talloc_free(name);
 	}
 }
 
@@ -1093,9 +1146,15 @@ static int _rm(struct connection *conn, const void *ctx, struct node *node,
 	/* 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, ctx, get_parent(ctx, name));
+	struct node *parent;
+	char *parentname = get_parent(ctx, name);
+
+	if (!parentname)
+		return errno;
+
+	parent = read_node(conn, ctx, parentname);
 	if (!parent)
-		return EINVAL;
+		return (errno == ENOMEM) ? ENOMEM : EINVAL;
 
 	if (!delete_child(conn, parent, basename(name)))
 		return EINVAL;
@@ -1121,19 +1180,24 @@ static int do_rm(struct connection *conn, struct buffered_data *in)
 	struct node *node;
 	int ret;
 	char *name;
+	char *parentname;
 
 	node = get_node_canonicalized(conn, in, onearg(in), &name,
 				      XS_PERM_WRITE);
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
-			node = read_node(conn, in, get_parent(in, name));
+			parentname = get_parent(in, name);
+			if (!parentname)
+				return errno;
+			node = read_node(conn, in, parentname);
 			if (node) {
 				send_ack(conn, XS_RM);
 				return 0;
 			}
 			/* Restore errno, just in case. */
-			errno = ENOENT;
+			if (errno != ENOMEM)
+				errno = ENOENT;
 		}
 		return errno;
 	}
@@ -1194,6 +1258,8 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in)
 	num--;
 
 	perms = talloc_array(node, struct xs_permissions, num);
+	if (!perms)
+		return ENOMEM;
 	if (!xs_strings_to_perms(perms, num, permstr))
 		return errno;
 
@@ -1328,7 +1394,7 @@ static void handle_input(struct connection *conn)
 
 	if (!conn->in) {
 		conn->in = new_buffer(conn);
-		/* In case of no memory just try it next time again. */
+		/* In case of no memory just try it again next time. */
 		if (!conn->in)
 			return;
 	}
@@ -1336,26 +1402,29 @@ static void handle_input(struct connection *conn)
 
 	/* Not finished header yet? */
 	if (in->inhdr) {
-		bytes = conn->read(conn, in->hdr.raw + in->used,
-				   sizeof(in->hdr) - in->used);
-		if (bytes < 0)
-			goto bad_client;
-		in->used += bytes;
-		if (in->used != sizeof(in->hdr))
-			return;
-
-		if (in->hdr.msg.len > XENSTORE_PAYLOAD_MAX) {
-			syslog(LOG_ERR, "Client tried to feed us %i",
-			       in->hdr.msg.len);
-			goto bad_client;
+		if (in->used != sizeof(in->hdr)) {
+			bytes = conn->read(conn, in->hdr.raw + in->used,
+					   sizeof(in->hdr) - in->used);
+			if (bytes < 0)
+				goto bad_client;
+			in->used += bytes;
+			if (in->used != sizeof(in->hdr))
+				return;
+
+			if (in->hdr.msg.len > XENSTORE_PAYLOAD_MAX) {
+				syslog(LOG_ERR, "Client tried to feed us %i",
+				       in->hdr.msg.len);
+				goto bad_client;
+			}
 		}
 
 		if (in->hdr.msg.len <= DEFAULT_BUFFER_SIZE)
 			in->buffer = in->default_buffer;
 		else
 			in->buffer = talloc_array(in, char, in->hdr.msg.len);
+		/* In case of no memory just try it again next time. */
 		if (!in->buffer)
-			goto bad_client;
+			return;
 		in->used = 0;
 		in->inhdr = false;
 	}
@@ -1478,6 +1547,9 @@ static void manual_node(const char *name, const char *child)
 	struct xs_permissions perms = { .id = 0, .perms = XS_PERM_NONE };
 
 	node = talloc_zero(NULL, struct node);
+	if (!node)
+		barf_perror("Could not allocate initial node %s", name);
+
 	node->name = name;
 	node->perms = &perms;
 	node->num_perms = 1;
@@ -1515,6 +1587,8 @@ static void setup_structure(void)
 {
 	char *tdbname;
 	tdbname = talloc_strdup(talloc_autofree_context(), xs_daemon_tdb());
+	if (!tdbname)
+		barf_perror("Could not create tdbname");
 
 	if (!(tdb_flags & TDB_INTERNAL))
 		tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
@@ -1593,11 +1667,14 @@ static char *child_name(const char *s1, const char *s2)
 }
 
 
-static void remember_string(struct hashtable *hash, const char *str)
+static int remember_string(struct hashtable *hash, const char *str)
 {
 	char *k = malloc(strlen(str) + 1);
+
+	if (!k)
+		return 0;
 	strcpy(k, str);
-	hashtable_insert(hash, k, (void *)1);
+	return hashtable_insert(hash, k, (void *)1);
 }
 
 
@@ -1614,9 +1691,10 @@ static void remember_string(struct hashtable *hash, const char *str)
  * As we go, we record each node in the given reachable hashtable.  These
  * entries will be used later in clean_store.
  */
-static void check_store_(const char *name, struct hashtable *reachable)
+static int check_store_(const char *name, struct hashtable *reachable)
 {
 	struct node *node = read_node(NULL, name, name);
+	int ret = 0;
 
 	if (node) {
 		size_t i = 0;
@@ -1624,14 +1702,24 @@ static void check_store_(const char *name, struct hashtable *reachable)
 		struct hashtable * children =
 			create_hashtable(16, hash_from_key_fn, keys_equal_fn);
 
-		remember_string(reachable, name);
+		if (!remember_string(reachable, name)) {
+			hashtable_destroy(children, 0);
+			log("check_store: ENOMEM");
+			return ENOMEM;
+		}
 
-		while (i < node->childlen) {
+		while (i < node->childlen && !ret) {
+			struct node *childnode;
 			size_t childlen = strlen(node->children + i);
 			char * childname = child_name(node->name,
 						      node->children + i);
-			struct node *childnode = read_node(NULL, childname,
-							   childname);
+
+			if (!childname) {
+				log("check_store: ENOMEM");
+				ret = ENOMEM;
+				break;
+			}
+			childnode = read_node(NULL, childname, childname);
 			
 			if (childnode) {
 				if (hashtable_search(children, childname)) {
@@ -1645,11 +1733,18 @@ static void check_store_(const char *name, struct hashtable *reachable)
 					}
 				}
 				else {
-					remember_string(children, childname);
-					check_store_(childname, reachable);
+					if (!remember_string(children,
+							     childname)) {
+						log("check_store: ENOMEM");
+						talloc_free(childnode);
+						talloc_free(childname);
+						ret = ENOMEM;
+						break;
+					}
+					ret = check_store_(childname,
+							   reachable);
 				}
-			}
-			else {
+			} else if (errno != ENOMEM) {
 				log("check_store: No child '%s' found!\n",
 				    childname);
 
@@ -1657,6 +1752,9 @@ static void check_store_(const char *name, struct hashtable *reachable)
 					remove_child_entry(NULL, node, i);
 					i -= childlen + 1;
 				}
+			} else {
+				log("check_store: ENOMEM");
+				ret = ENOMEM;
 			}
 
 			talloc_free(childnode);
@@ -1667,14 +1765,18 @@ static void check_store_(const char *name, struct hashtable *reachable)
 		hashtable_destroy(children, 0 /* Don't free values (they are
 						 all (void *)1) */);
 		talloc_free(node);
-	}
-	else {
+	} else if (errno != ENOMEM) {
 		/* Impossible, because no database should ever be without the
 		   root, and otherwise, we've just checked in our caller
 		   (which made a recursive call to get here). */
 		   
 		log("check_store: No child '%s' found: impossible!", name);
+	} else {
+		log("check_store: ENOMEM");
+		ret = ENOMEM;
 	}
+
+	return ret;
 }
 
 
@@ -1687,6 +1789,11 @@ static int clean_store_(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA val,
 	struct hashtable *reachable = private;
 	char * name = talloc_strndup(NULL, key.dptr, key.dsize);
 
+	if (!name) {
+		log("clean_store: ENOMEM");
+		return 1;
+	}
+
 	if (!hashtable_search(reachable, name)) {
 		log("clean_store: '%s' is orphaned!", name);
 		if (recovery) {
@@ -1716,9 +1823,14 @@ static void check_store(void)
 	struct hashtable * reachable =
 		create_hashtable(16, hash_from_key_fn, keys_equal_fn);
  
+	if (!reachable) {
+		log("check_store: ENOMEM");
+		return;
+	}
+
 	log("Checking store ...");
-	check_store_(root, reachable);
-	clean_store(reachable);
+	if (!check_store_(root, reachable))
+		clean_store(reachable);
 	log("Checking store complete.");
 
 	hashtable_destroy(reachable, 0 /* Don't free values (they are all
@@ -1772,10 +1884,14 @@ static void init_sockets(int **psock, int **pro_sock)
 
 	/* Create sockets for them to listen to. */
 	*psock = sock = talloc(talloc_autofree_context(), int);
+	if (!sock)
+		barf_perror("No memory when creating sockets");
 	*sock = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (*sock < 0)
 		barf_perror("Could not create socket");
 	*pro_sock = ro_sock = talloc(talloc_autofree_context(), int);
+	if (!ro_sock)
+		barf_perror("No memory when creating sockets");
 	*ro_sock = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (*ro_sock < 0)
 		barf_perror("Could not create socket");
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 1e88f1202d06..6ff8b39e2d25 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -296,10 +296,15 @@ static struct domain *new_domain(void *context, unsigned int domid,
 	int rc;
 
 	domain = talloc(context, struct domain);
+	if (!domain)
+		return NULL;
+
 	domain->port = 0;
 	domain->shutdown = 0;
 	domain->domid = domid;
 	domain->path = talloc_domain_path(domain, domid);
+	if (!domain->path)
+		return NULL;
 
 	wrl_domain_new(domain);
 
@@ -313,6 +318,9 @@ static struct domain *new_domain(void *context, unsigned int domid,
 	domain->port = rc;
 
 	domain->conn = new_connection(writechn, readchn);
+	if (!domain->conn)
+		return NULL;
+
 	domain->conn->domain = domain;
 	domain->conn->id = domid;
 
@@ -517,6 +525,8 @@ int do_get_domain_path(struct connection *conn, struct buffered_data *in)
 		return EINVAL;
 
 	path = talloc_domain_path(conn, atoi(domid_str));
+	if (!path)
+		return errno;
 
 	send_reply(conn, XS_GET_DOMAIN_PATH, path, strlen(path) + 1);
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 2bd34c02a771..a01f8cfe3486 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -119,7 +119,18 @@ void add_change_node(struct connection *conn, struct node *node, bool recurse)
 	}
 
 	i = talloc(trans, struct changed_node);
+	if (!i) {
+		/* All we can do is let the transaction fail. */
+		generation++;
+		return;
+	}
 	i->node = talloc_strdup(i, node->name);
+	if (!i->node) {
+		/* All we can do is let the transaction fail. */
+		generation++;
+		talloc_free(i);
+		return;
+	}
 	i->recurse = recurse;
 	list_add_tail(&i->list, &trans->changes);
 }
@@ -164,11 +175,16 @@ int do_transaction_start(struct connection *conn, struct buffered_data *in)
 
 	/* Attach transaction to input for autofree until it's complete */
 	trans = talloc_zero(in, struct transaction);
+	if (!trans)
+		return ENOMEM;
+
 	INIT_LIST_HEAD(&trans->changes);
 	INIT_LIST_HEAD(&trans->changed_domains);
 	trans->generation = generation;
 	trans->tdb_name = talloc_asprintf(trans, "%s.%p",
 					  xs_daemon_tdb(), trans);
+	if (!trans->tdb_name)
+		return ENOMEM;
 	trans->tdb = tdb_copy(tdb_context(conn), trans->tdb_name);
 	if (!trans->tdb)
 		return errno;
@@ -251,6 +267,11 @@ void transaction_entry_inc(struct transaction *trans, unsigned int domid)
 		}
 
 	d = talloc(trans, struct changed_domain);
+	if (!d) {
+		/* Let the transaction fail. */
+		generation++;
+		return;
+	}
 	d->domid = domid;
 	d->nbentry = 1;
 	list_add_tail(&d->list, &trans->changed_domains);
@@ -267,6 +288,11 @@ void transaction_entry_dec(struct transaction *trans, unsigned int domid)
 		}
 
 	d = talloc(trans, struct changed_domain);
+	if (!d) {
+		/* Let the transaction fail. */
+		generation++;
+		return;
+	}
 	d->domid = domid;
 	d->nbentry = -1;
 	list_add_tail(&d->list, &trans->changed_domains);
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 94251dba91bd..0dc5a40b99a9 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -111,6 +111,8 @@ static void add_event(struct connection *conn,
 
 	len = strlen(name) + 1 + strlen(watch->token) + 1;
 	data = talloc_array(ctx, char, len);
+	if (!data)
+		return;
 	strcpy(data, name);
 	strcpy(data + strlen(name) + 1, watch->token);
 	send_reply(conn, XS_WATCH_EVENT, data, len);
@@ -165,6 +167,8 @@ int do_watch(struct connection *conn, struct buffered_data *in)
 	} else {
 		relative = !strstarts(vec[0], "/");
 		vec[0] = canonicalize(conn, in, vec[0]);
+		if (!vec[0])
+			return ENOMEM;
 		if (!is_valid_nodename(vec[0]))
 			return EINVAL;
 	}
@@ -180,8 +184,14 @@ int do_watch(struct connection *conn, struct buffered_data *in)
 		return E2BIG;
 
 	watch = talloc(conn, struct watch);
+	if (!watch)
+		return ENOMEM;
 	watch->node = talloc_strdup(watch, vec[0]);
 	watch->token = talloc_strdup(watch, vec[1]);
+	if (!watch->node || !watch->token) {
+		talloc_free(watch);
+		return ENOMEM;
+	}
 	if (relative)
 		watch->relative_path = get_implicit_path(conn);
 	else
@@ -210,6 +220,8 @@ int do_unwatch(struct connection *conn, struct buffered_data *in)
 		return EINVAL;
 
 	node = canonicalize(conn, in, vec[0]);
+	if (!node)
+		return ENOMEM;
 	list_for_each_entry(watch, &conn->watches, list) {
 		if (streq(watch->node, node) && streq(watch->token, vec[1])) {
 			list_del(&watch->list);
-- 
2.25.4