[Cluster-devel] cluster/cman/daemon cmanccs.c

[rmccabe@sourceware.org <rmccabe@sourceware.org>]

CVSROOT:	/cvs/cluster
Module name:	cluster
Changes by:	rmccabe at sourceware.org	2007-10-24 05:55:07

Modified files:
	cman/daemon    : cmanccs.c 

Log message:
	- Fix unsafe string handling:
	- replace memset(s,c,n);sprintf(s,...); with snprintf with proper error checking
	- don't overflow the stack if the cluster name specified in the env var is too long
	- don't overflow the stack if the local nodename from uname(2) is too long
	- don't overflow the stack if the local nodename specified in the env var is too long
	
	- Don't leak the ccs descriptor in get_ccs_join_info() on errors

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/cluster/cman/daemon/cmanccs.c.diff?cvsroot=cluster&r1=1.34&r2=1.35

--- cluster/cman/daemon/cmanccs.c	2007/10/24 03:21:45	1.34
+++ cluster/cman/daemon/cmanccs.c	2007/10/24 05:55:07	1.35
@@ -1,7 +1,7 @@
 /******************************************************************************
 *******************************************************************************
 **
-**  Copyright (C) 2005-2006 Red Hat, Inc.  All rights reserved.
+**  Copyright (C) 2005-2007 Red Hat, Inc.  All rights reserved.
 **
 **  This copyrighted material is made available to anyone wishing to use,
 **  modify, copy, or redistribute it subject to the terms and conditions
@@ -125,49 +125,60 @@
     if (!ccs_get(ctree, TWO_NODE_PATH, &str)) {
 	    two_node = atoi(str);
 	    free(str);
-    }
-    else
-	two_node = 0;
+    } else
+		two_node = 0;
 
     for (i=1;;i++) {
-	char path[MAX_PATH_LEN];
-	int  votes=0, nodeid=0;
+		char path[MAX_PATH_LEN];
+		int votes=0, nodeid=0;
+		int ret;
+
+		ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNUM, i);
+		if (ret < 0 || (size_t) ret >= sizeof(path))
+			return -E2BIG;
 
-	memset(path, 0, MAX_PATH_LEN);
-	sprintf(path, NODE_NAME_PATH_BYNUM, i);
-	error = ccs_get(ctree, path, &nodename);
-	if (error)
-	    break;
+		error = ccs_get(ctree, path, &nodename);
+		if (error)
+			break;
 
-	memset(path, 0, MAX_PATH_LEN);
-	sprintf(path, NODE_VOTES_PATH, nodename);
-	if (!ccs_get(ctree, path, &str)) {
-	    votes = atoi(str);
-	    free(str);
-	} else
-	    votes = 1;
+		ret = snprintf(path, sizeof(path), NODE_VOTES_PATH, nodename);
+		if (ret < 0 || ret >= sizeof(path)) {
+			error = -E2BIG;
+			goto out_err;
+		}
 
-	memset(path, 0, MAX_PATH_LEN);
-	sprintf(path, NODE_NODEID_PATH, nodename);
-	if (!ccs_get(ctree, path, &str)) {
-	    nodeid = atoi(str);
-	    free(str);
+		if (!ccs_get(ctree, path, &str)) {
+			votes = atoi(str);
+			free(str);
+		} else
+			votes = 1;
 
-	}
+		ret = snprintf(path, sizeof(path), NODE_NODEID_PATH, nodename);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			goto out_err;
+		}
 
-	if (check_nodeids && nodeid == 0) {
-		char message[132];
+		if (!ccs_get(ctree, path, &str)) {
+			nodeid = atoi(str);
+			free(str);
+		}
 
-		sprintf(message, "No node ID for %s, run 'ccs_tool addnodeids' to fix", nodename);
-		log_printf(LOG_ERR, message);
-		write_cman_pipe(message);
-		return -1;
-	}
+		if (check_nodeids && nodeid == 0) {
+			char message[132];
 
-	P_MEMB("Got node %s from ccs (id=%d, votes=%d)\n", nodename, nodeid, votes);
-	add_ccs_node(nodename, nodeid, votes, expected);
+			snprintf(message, sizeof(message),
+				"No node ID for %s, run 'ccs_tool addnodeids' to fix",
+				nodename);
+			log_printf(LOG_ERR, message);
+			write_cman_pipe(message);
+			error = -EINVAL;
+			goto out_err;
+		}
 
-	free(nodename);
+		P_MEMB("Got node %s from ccs (id=%d, votes=%d)\n", nodename, nodeid, votes);
+		add_ccs_node(nodename, nodeid, votes, expected);
+		free(nodename);
     }
 
     if (expected)
@@ -177,6 +188,11 @@
     ccs_disconnect(ctree);
 
     return 0;
+
+out_err:
+	free(nodename);
+	ccs_disconnect(ctree);
+	return error;
 }
 
 static char *default_mcast(uint16_t cluster_id)
@@ -260,11 +276,14 @@
 	struct ifaddrs *ifa, *ifa_list;
 	struct sockaddr *sa;
 	int error, i;
+	int ret;
 
 	/* nodename is either from commandline or from uname */
 	str = NULL;
-	memset(path, 0, MAX_PATH_LEN);
-	sprintf(path, NODE_NAME_PATH_BYNAME, nodename);
+
+	ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNAME, nodename);
+	if (ret < 0 || (size_t) ret >= sizeof(path))
+		return -E2BIG;
 
 	error = ccs_get(cd, path, &str);
 	if (!error) {
@@ -274,13 +293,14 @@
 
 	/* If nodename was from uname, try a domain-less version of it */
 	strcpy(nodename2, nodename);
-	dot = strstr(nodename2, ".");
+	dot = strchr(nodename2, '.');
 	if (dot) {
 		*dot = '\0';
 
 		str = NULL;
-		memset(path, 0, MAX_PATH_LEN);
-		sprintf(path, NODE_NAME_PATH_BYNAME, nodename2);
+		ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNAME, nodename2);
+		if (ret < 0 || (size_t) ret >= sizeof(path))
+			return -E2BIG;
 
 		error = ccs_get(cd, path, &str);
 		if (!error) {
@@ -290,21 +310,25 @@
 		}
 	}
 
-
 	/* If nodename (from uname) is domain-less, try to match against
 	   cluster.conf names which may have domainname specified */
 	for (i = 1; ; i++) {
 		int len;
+
 		str = NULL;
-		memset(path, 0, 256);
-		sprintf(path, "/cluster/clusternodes/clusternode[%d]/@name", i);
+		ret = snprintf(path, sizeof(path),
+				"/cluster/clusternodes/clusternode[%d]/@name", i);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			break;
+		}
 
 		error = ccs_get(cd, path, &str);
 		if (error || !str)
 			break;
 
 		strcpy(nodename3, str);
-		dot = strstr(nodename3, ".");
+		dot = strchr(nodename3, '.');
 		if (dot)
 			len = dot-nodename3;
 		else
@@ -328,7 +352,6 @@
 		return -1;
 
 	for (ifa = ifa_list; ifa; ifa = ifa->ifa_next) {
-
 		/* Restore this */
 		strcpy(nodename2, nodename);
 		sa = ifa->ifa_addr;
@@ -341,8 +364,11 @@
 			goto out;
 
 		str = NULL;
-		memset(path, 0, 256);
-		sprintf(path, NODE_NAME_PATH_BYNAME, nodename2);
+		ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNAME, nodename2);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			goto out;
+		}
 
 		error = ccs_get(cd, path, &str);
 		if (!error) {
@@ -353,14 +379,17 @@
 
 		/* truncate this name and try again */
 
-		dot = strstr(nodename2, ".");
+		dot = strchr(nodename2, '.');
 		if (!dot)
 			continue;
 		*dot = '\0';
 
 		str = NULL;
-		memset(path, 0, 256);
-		sprintf(path, NODE_NAME_PATH_BYNAME, nodename2);
+		ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNAME, nodename2);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			goto out;
+		}
 
 		error = ccs_get(cd, path, &str);
 		if (!error) {
@@ -376,8 +405,11 @@
 			goto out;
 
 		str = NULL;
-		memset(path, 0, 256);
-		sprintf(path, NODE_NAME_PATH_BYNAME, nodename2);
+		ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNAME, nodename2);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			goto out;
+		}
 
 		error = ccs_get(cd, path, &str);
 		if (!error) {
@@ -398,7 +430,7 @@
 {
 	char path[MAX_PATH_LEN];
 	char nodename[MAX_CLUSTER_MEMBER_NAME_LEN+1];
-	char *str, *name, *cname = NULL;
+	char *str, *name, *cname = NULL, *nodename_env;
 	int cd, error, i, vote_sum = 0, node_count = 0;
 	unsigned short port = 0;
 
@@ -420,17 +452,26 @@
 	if (error) {
 		log_printf(LOG_ERR, "cannot find cluster name in config file");
 		write_cman_pipe("Can't find cluster name in CCS");
-		return -ENOENT;
+		error = -ENOENT;
+		goto out;
 	}
 
 	if (cname) {
 		if (strcmp(cname, str)) {
 			log_printf(LOG_ERR, "cluster names not equal %s %s", cname, str);
 			write_cman_pipe("Cluster name in CCS does not match that passed to cman_tool");
-			return -ENOENT;
+			error = -ENOENT;
+			goto out;
 		}
 	}
 
+	if (strlen(str) >= sizeof(cluster_name)) {
+		free(str);
+		write_cman_pipe("Cluster name in CCS is too long");
+		error = -E2BIG;
+		goto out;
+	}
+
 	strcpy(cluster_name, str);
 	free(str);
 
@@ -444,32 +485,55 @@
 	}
 
 	/* our nodename */
-	memset(nodename, 0, sizeof(nodename));
+	nodename_env = getenv("CMAN_NODENAME");
+	if (nodename_env != NULL) {
+		int ret;
+
+		if (strlen(nodename_env) >= sizeof(nodename)) {
+			log_printf(LOG_ERR, "Overridden node name %s is too long", nodename);
+			write_cman_pipe("Overridden node name is too long");
+			error = -E2BIG;
+			goto out;
+		}
 
-	if (getenv("CMAN_NODENAME")) {
-		strcpy(nodename, getenv("CMAN_NODENAME"));
+		strcpy(nodename, nodename_env);
 		log_printf(LOG_INFO, "Using override node name %s\n", nodename);
 
-		sprintf(path, NODE_NAME_PATH_BYNAME, nodename);
+		ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNAME, nodename);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			log_printf(LOG_ERR, "Overridden node name %s is too long", nodename);
+			write_cman_pipe("Overridden node name is too long");
+			error = -E2BIG;
+			goto out;
+		}
 
 		error = ccs_get(cd, path, &str);
 		if (!error) {
 			free(str);
-		}
-		else {
+		} else {
 			log_printf(LOG_ERR, "Overridden node name %s is not in CCS", nodename);
 			write_cman_pipe("Overridden node name is not in CCS");
-			return -ENOENT;
+			error = -ENOENT;
+			goto out;
 		}
-	}
-	else {
+	} else {
 		struct utsname utsname;
+
 		error = uname(&utsname);
 		if (error) {
 			log_printf(LOG_ERR, "cannot get node name, uname failed");
 			write_cman_pipe("Can't determine local node name");
-			return -ENOENT;
+			error = -ENOENT;
+			goto out;
+		}
+
+		if (strlen(utsname.nodename) >= sizeof(nodename)) {
+			log_printf(LOG_ERR, "node name from uname is too long");
+			write_cman_pipe("Can't determine local node name");
+			error = -E2BIG;
+			goto out;
 		}
+
 		strcpy(nodename, utsname.nodename);
 	}
 
@@ -480,9 +544,15 @@
 		log_printf(LOG_ERR, "local node name \"%s\" not found in cluster.conf",
 			nodename);
 		write_cman_pipe("Can't find local node name in cluster.conf");
-		return -ENOENT;
+		error = -ENOENT;
+		goto out;
 	}
+
 	nodenames[0] = strdup(nodename);
+	if (nodenames[0] == NULL) {
+		error = -ENOMEM;
+		goto out;
+	}
 
 	expected_votes = 0;
 	if (getenv("CMAN_EXPECTEDVOTES")) {
@@ -499,9 +569,14 @@
 	/* Sum node votes for expected */
 	if (expected_votes == 0) {
 		for (i = 1; ; i++) {
+			int ret;
+
 			name = NULL;
-			memset(path, 0, MAX_PATH_LEN);
-			sprintf(path, NODE_NAME_PATH_BYNUM, i);
+			ret = snprintf(path, sizeof(path), NODE_NAME_PATH_BYNUM, i);
+			if (ret < 0 || (size_t) ret >= sizeof(path)) {
+				error = -E2BIG;
+				break;
+			}
 
 			error = ccs_get(cd, path, &name);
 			if (error || !name)
@@ -509,10 +584,14 @@
 
 			node_count++;
 
-			memset(path, 0, MAX_PATH_LEN);
-			sprintf(path, NODE_VOTES_PATH, name);
+			ret = snprintf(path, sizeof(path), NODE_VOTES_PATH, name);
 			free(name);
 
+			if (ret < 0 || (size_t) ret >= sizeof(path)) {
+				error = -E2BIG;
+				break;
+			}
+
 			error = ccs_get(cd, path, &str);
 			if (error)
 				vote_sum++;
@@ -520,7 +599,8 @@
 				if (atoi(str) < 0) {
 					log_printf(LOG_ERR, "negative votes not allowed");
 					write_cman_pipe("Found negative votes for this node in CCS");
-					return -EINVAL;
+					error = -EINVAL;
+					goto out;
 				}
 				vote_sum += atoi(str);
 				free(str);
@@ -572,8 +652,12 @@
 	}
 
 	if (!votes) {
-		memset(path, 0, MAX_PATH_LEN);
-		sprintf(path, NODE_VOTES_PATH, nodename);
+		int ret = snprintf(path, sizeof(path), NODE_VOTES_PATH, nodename);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			log_printf(LOG_ERR, "unable to find votes for %s", nodename);
+			write_cman_pipe("Unable to find votes for node in CCS");
+			return -E2BIG;
+		}
 
 		error = ccs_get(cd, path, &str);
 		if (!error) {
@@ -599,13 +683,14 @@
 	}
 
 	if (!nodeid) {
-		memset(path, 0, MAX_PATH_LEN);
-		sprintf(path, NODE_NODEID_PATH, nodename);
+		int ret = snprintf(path, sizeof(path), NODE_NODEID_PATH, nodename);
 
-		error = ccs_get(cd, path, &str);
-		if (!error) {
-			nodeid = atoi(str);
-			free(str);
+		if (ret >= 0 && (size_t) ret < sizeof(path)) {
+			error = ccs_get(cd, path, &str);
+			if (!error) {
+				nodeid = atoi(str);
+				free(str);
+			}
 		}
 	}
 
@@ -638,35 +723,44 @@
 	/* Get all alternative node names */
 	num_nodenames = 1;
 
-	memset(path, 0, MAX_PATH_LEN);
-
-
 	for (i = 1; ; i++) {
-		str = NULL;
-		sprintf(path, NODE_ALTNAMES_PATH, nodename, i);
+		int ret = snprintf(path, sizeof(path), NODE_ALTNAMES_PATH, nodename, i);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			break;
+		}
 
+		str = NULL;
 		error = ccs_get(cd, path, &str);
 		if (error || !str)
 			break;
 
 		nodenames[i] = str;
 
-		sprintf(path, NODE_ALTNAMES_PORT, nodename, i);
+		ret = snprintf(path, sizeof(path), NODE_ALTNAMES_PORT, nodename, i);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			break;
+		}
+
 		error = ccs_get(cd, path, &str);
 		if (error || !str) {
 			portnums[i] = portnums[0];
-		}
-		else {
+		} else {
 			portnums[i] = atoi(str);
 			free(str);
 		}
 
-		sprintf(path, NODE_ALTNAMES_MCAST, nodename, i);
+		ret = snprintf(path, sizeof(path), NODE_ALTNAMES_MCAST, nodename, i);
+		if (ret < 0 || (size_t) ret >= sizeof(path)) {
+			error = -E2BIG;
+			break;
+		}
+
 		error = ccs_get(cd, path, &str);
 		if (error || !str) {
 			mcast[i] = mcast_name;
-		}
-		else {
+		} else {
 			mcast[i] = str;
 		}
 
@@ -686,7 +780,8 @@
 					"votes of 1 (node_count=%d vote_sum=%d)",
 					node_count, vote_sum);
 				write_cman_pipe("two_node set but there are more than 2 nodes");
-				return -EINVAL;
+				error = -EINVAL;
+				goto out;
 			}
 
 			if (votes != 1) {
@@ -694,13 +789,17 @@
 					"nodes with one vote each and expected "
 					"votes of 1 (votes=%d)", votes);
 				write_cman_pipe("two_node set but votes not set to 1");
-				return -EINVAL;
+				error = -EINVAL;
+				goto out;
 			}
 		}
 	}
 
+	error = 0;
+
+out:
 	ccs_disconnect(cd);
-	return 0;
+	return error;
 }