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

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

[jpokorny]

Signed-off-by: Jan Pokorn? <jpokorny@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);
 	}
 

[Cluster-devel] [PATCH] rgmanager: reslist: nothing avoids using size_t

[Jan Pokorný]

+ fix comment typo

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 reslist.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/reslist.c b/reslist.c
index e1695fb..f9924cd 100644
--- a/reslist.c
+++ b/reslist.c
@@ -70,7 +70,7 @@ _attr_value(resource_node_t *node, const char *attrname, const char *ptype)
 	resource_t *res;
 	resource_attr_t *ra;
 	char *c, p_type[32];
-	ssize_t len;
+	size_t len;
 	int x;
 
 	if (!node)
@@ -99,11 +99,13 @@ _attr_value(resource_node_t *node, const char *attrname, const char *ptype)
 		c = strchr(ra->ra_value, '%');
 		if (!c) {
 			/* Someone doesn't care or uses older
-			   semantics on inheritance */
+			   semantics of inheritance */
 			return _attr_value(node->rn_parent, ra->ra_value,
 					   NULL);
 		}
 		
+		/* Difference guaranteed to be non-negative
+		   (for x >= 0: &ra->ra_value[x] >= &ra->ra_value[0]) */
 		len = (c - ra->ra_value);
 		memset(p_type, 0, sizeof(p_type));
 		memcpy(p_type, ra->ra_value, len);
-- 
1.7.3.4

[Cluster-devel] [PATCH] rgmanager: reslist: sanitize act_dup

[Jan Pokorný]

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 reslist.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/reslist.c b/reslist.c
index f9924cd..b75f4b0 100644
--- a/reslist.c
+++ b/reslist.c
@@ -626,10 +626,10 @@ print_resources(resource_t **resources)
 }
 
 
-void *
+resource_act_t *
 act_dup(resource_act_t *acts)
 {
-	int x;
+	size_t x;
 	resource_act_t *newacts;
 
 	for (x = 0; acts[x].ra_name; x++);
-- 
1.7.3.4

[Cluster-devel] [PATCH] rgmanager: reslist: another string related cleanup

[Jan Pokorný]

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 reslist.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/reslist.c b/reslist.c
index b75f4b0..43430cd 100644
--- a/reslist.c
+++ b/reslist.c
@@ -107,9 +107,12 @@ _attr_value(resource_node_t *node, const char *attrname, const char *ptype)
 		/* Difference guaranteed to be non-negative
 		   (for x >= 0: &ra->ra_value[x] >= &ra->ra_value[0]) */
 		len = (c - ra->ra_value);
-		memset(p_type, 0, sizeof(p_type));
+		if (len >= sizeof(p_type))
+			len = sizeof(p_type) - 1;
+
 		memcpy(p_type, ra->ra_value, len);
-		
+		p_type[sizeof(p_type)-1] = '\0';
+
 		/* Skip the "%" and recurse */
 		return _attr_value(node->rn_parent, ++c, p_type);
 	}