[Cluster-devel] [PATCH] rgmanager: various cleanups regarding string functions

[jpokorny at redhat.com]

Signed-off-by: Jan Pokorný <jpokorny at redhat.com>
---
 groups.c      |   12 +++++++++---
 main.c        |    2 +-
 restree.c     |    1 +
 rg_state.c    |   12 ++++++++----
 rg_thread.c   |    3 ++-
 slang_event.c |   29 ++++++++++++++++++-----------
 6 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/groups.c b/groups.c
index c4929e4..cdfc997 100644
--- a/groups.c
+++ b/groups.c
@@ -1129,7 +1129,8 @@ _group_property(const char *groupname, const char *property,
 	for (; res->r_attrs[x].ra_name; x++) {
 		if (strcasecmp(res->r_attrs[x].ra_name, property))
 			continue;
-		strncpy(ret, res->r_attrs[x].ra_value, len);
+		strncpy(ret, res->r_attrs[x].ra_value, len-1);
+		ret[len-1] = '\0';
 		return 0;
 	}
 
@@ -1826,14 +1827,19 @@ get_recovery_policy(const char *rg_name, char *buf, size_t buflen)
 	resource_t *res;
 	const char *val;
 
+	assert(buflen >= 1);  /* and expect partial result if doesn't fit */
+
 	pthread_rwlock_rdlock(&resource_lock);
 
-	strncpy(buf, "restart", buflen);
+	strncpy(buf, "restart", buflen-1);
+	buf[buflen-1] = '\0';
+
 	res = find_root_by_ref(&_resources, rg_name);
 	if (res) {
 		val = res_attr_value(res, "recovery");
 		if (val) {
-			strncpy(buf, val, buflen);
+			strncpy(buf, val, buflen-1);
+			/* Already terminated */
 		}
 	}
 
diff --git a/main.c b/main.c
index 931d95e..c823357 100644
--- a/main.c
+++ b/main.c
@@ -50,7 +50,7 @@ segfault(int __attribute__ ((unused)) sig)
 	char ow[64];
 	int err; // dumb error checking... will be replaced by logsys
 
-	snprintf(ow, sizeof(ow)-1, "PID %d Thread %d: SIGSEGV\n", getpid(),
+	snprintf(ow, sizeof(ow), "PID %d Thread %d: SIGSEGV\n", getpid(),
 		 gettid());
 	err = write(2, ow, strlen(ow));
 	while(1)
diff --git a/restree.c b/restree.c
index 8280c56..c1fb069 100644
--- a/restree.c
+++ b/restree.c
@@ -105,6 +105,7 @@ add_ocf_stuff(resource_t *res, char **env, int depth, int refcnt, int timeout)
 		strncpy(ver, OCF_API_VERSION, sizeof(ver)-1);
 	else 
 		strncpy(ver, res->r_rule->rr_version, sizeof(ver)-1);
+	ver[sizeof(ver)-1] = '\0';
 
 	minor = strchr(ver, '.');
 	if (minor) {
diff --git a/rg_state.c b/rg_state.c
index 3db6bd9..6d41fa5 100644
--- a/rg_state.c
+++ b/rg_state.c
@@ -207,8 +207,10 @@ set_rg_state(const char *name, rg_state_t *svcblk)
 	int ret, tries = 0;
 #endif
 
-	if (name)
-		strncpy(svcblk->rs_name, name, sizeof(svcblk->rs_name));
+	if (name) {
+		strncpy(svcblk->rs_name, name, sizeof(svcblk->rs_name)-1);
+		svcblk->rs_name[sizeof(svcblk->rs_name)-1] = '\0';
+	}
 
 	snprintf(res, sizeof(res), "rg=\"%s\"", name);
 
@@ -270,7 +272,8 @@ get_rg_state(const char *name, rg_state_t *svcblk)
 		return -1;
 	}
 
-	strncpy(svcblk->rs_name, name, sizeof(svcblk->rs_name));
+	strncpy(svcblk->rs_name, name, sizeof(svcblk->rs_name)-1);
+	svcblk->rs_name[sizeof(svcblk->rs_name)-1];
 
 	snprintf(res, sizeof(res),"rg=\"%s\"", svcblk->rs_name);
 
@@ -367,7 +370,8 @@ get_rg_state_local(const char *name, rg_state_t *svcblk)
 		errno = EINVAL;
 		return -1;
 	}
-	strncpy(svcblk->rs_name, name, sizeof(svcblk->rs_name));
+	strncpy(svcblk->rs_name, name, sizeof(svcblk->rs_name)-1);
+	svcblk->rs_name[sizeof(svcblk->rs_name)-1] = '\0';
 
 	snprintf(res, sizeof(res),"rg=\"%s\"", svcblk->rs_name);
 
diff --git a/rg_thread.c b/rg_thread.c
index 75af6f1..a7bf3f9 100644
--- a/rg_thread.c
+++ b/rg_thread.c
@@ -179,7 +179,8 @@ resgroup_thread_main(void *arg)
 
 	rg_inc_threads();
 
-	strncpy(myname, arg, 256);
+	strncpy(myname, arg, sizeof(myname)-1);
+	myname[sizeof(myname)-1] = '\0';
 	dbg_printf("Thread %s (tid %d) starting\n",myname,gettid());
 
 	pthread_mutex_init(&my_queue_mutex, NULL);
diff --git a/slang_event.c b/slang_event.c
index 0b9f0d0..ccdda53 100644
--- a/slang_event.c
+++ b/slang_event.c
@@ -850,24 +850,24 @@ array_to_string(char *buf, int buflen, int *array, int arraylen)
 
 	memset(intbuf, 0, sizeof(intbuf));
 	memset(buf, 0, buflen);
-	len = snprintf(buf, buflen - 1, "[ ");
-	if (len == buflen)
+	len = snprintf(buf, buflen, "[ ");
+	if (len >= buflen)
 		return -1;
 
 	remain -= len;
 	for (x = 0; x < arraylen; x++) {
-		len = snprintf(intbuf, sizeof(intbuf) - 1, "%d ", array[x]);
+		len = snprintf(intbuf, sizeof(intbuf), "%d ", array[x]);
 		remain -= len;
-		if (remain > 0) {
+		if (remain >= 0) {
 			strncat(buf, intbuf, len);
 		} else {
 			return -1;
 		}
 	}
 
-	len = snprintf(intbuf, sizeof(intbuf) - 1 ,  "]");
+	len = snprintf(intbuf, sizeof(intbuf) , "]");
 	remain -= len;
-	if (remain > 0) {
+	if (remain >= 0) {
 		strncat(buf, intbuf, len);
 	} else {
 		return -1;
@@ -938,28 +938,35 @@ sl_logt_print(int level)
 		case SLANG_INT_TYPE:
 			if (SLang_pop_integer(&s_intval) < 0)
 				return;
-			len=snprintf(tmp, sizeof(tmp) - 1, "%d", s_intval);
+			len=snprintf(tmp, sizeof(tmp), "%d", s_intval);
+			if (len > sizeof(tmp))
+				len = sizeof(tmp);
 			break;
 		case SLANG_STRING_TYPE:
 			need_free = 0;
 			if (SLpop_string(&s_strval) < 0)
 				return;
-			len=snprintf(tmp, sizeof(tmp) - 1, "%s", s_strval);
+			len=snprintf(tmp, sizeof(tmp), "%s", s_strval);
 			SLfree(s_strval);
+			if (len > sizeof(tmp))
+				len = sizeof(tmp);
 			break;
 		default:
 			need_free = 0;
-			len=snprintf(tmp, sizeof(tmp) - 1,
+			len=snprintf(tmp, sizeof(tmp),
 				     "{UnknownType %d}", t);
+			if (len > sizeof(tmp))
+				len = sizeof(tmp);
 			break;
 		}
 
 		--nargs;
 
 		if (len > remain)
-			return;
-		remain -= len;
+			/* Partial log is better than none */
+			break;
 
+		remain -= len;
 		memcpy(&logbuf[remain], tmp, len);
 	}