[Cluster-devel] [coverity] cman

[Cluster-devel] [coverity] cman

[Fabio M. Di Nitto]

Before:

Analysis summary report:
------------------------
Files analyzed                 : 27
Total LoC input to cov-analyze : 32029
Functions analyzed             : 380
Paths analyzed                 : 52124
New defects found              : 315 Total
                                   3 BAD_SIZEOF
                                  16 CHECKED_RETURN
                                   4 CONSTANT_EXPRESSION_RESULT
                                   1 DEADCODE
                                   5 FORWARD_NULL
                                   2 INTEGER_OVERFLOW
                                   1 MISSING_BREAK
                                   1 NEGATIVE_RETURNS
                                   5 NO_EFFECT
                                 139 NULL_RETURNS
                                   2 PW.DECLARED_BUT_NOT_REFERENCED
                                   1 PW.MAIN_WRONG_NUM_PARAMS
                                  13 PW.MIXED_ENUM_TYPE
                                   1 PW.NOT_COMPATIBLE_WITH_PREVIOUS_DECL
                                  12 PW.SET_BUT_NOT_USED
                                   7 RESOURCE_LEAK
                                   3 REVERSE_INULL
                                  51 SECURE_CODING
                                   3 SIZEOF_MISMATCH
                                   1 STRING_NULL
                                  12 STRING_OVERFLOW
                                   6 TAINTED_SCALAR
                                   1 TAINTED_STRING
                                   4 TOCTOU
                                  20 UNINIT
                                   1 UNUSED_VALUE

After:

Analysis summary report:
------------------------
Files analyzed                 : 27
Total LoC input to cov-analyze : 32078
Functions analyzed             : 379
Paths analyzed                 : 52565
New defects found              : 205 Total
                                   8 CHECKED_RETURN
                                   2 INTEGER_OVERFLOW
                                   2 NO_EFFECT
                                 134 NULL_RETURNS
                                   2 PW.DECLARED_BUT_NOT_REFERENCED
                                  12 PW.MIXED_ENUM_TYPE
                                   1 PW.NOT_COMPATIBLE_WITH_PREVIOUS_DECL
                                   6 PW.SET_BUT_NOT_USED
                                   4 RESOURCE_LEAK
                                  10 SECURE_CODING
                                   6 TAINTED_SCALAR
                                   1 TAINTED_STRING
                                   4 TOCTOU
                                  13 UNINIT

Of the remaining warnings,
12 PW.MIXED_ENUM_TYPE
 6 TAINTED_SCALAR
 1 TAINTED_STRING
need further investigation as the warning is non obvious at all.

All the others can be ignored.

[Cluster-devel] [PATCH 01/41] libcman: fix bad flags check

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 a89c731... 9ea82c1... M	cman/lib/libcman.c
 cman/lib/libcman.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index a89c731..9ea82c1 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -467,7 +467,7 @@ int cman_dispatch(cman_handle_t handle, int flags)
 			len = smsg->msg->length;
 			free(smsg->msg);
 			free(smsg);
-			if (res || (flags & CMAN_DISPATCH_ONE))
+			if (res || (flags & CMAN_DISPATCH_TYPE_MASK) == CMAN_DISPATCH_ONE)
 				break;
 			else
 				continue;
@@ -481,7 +481,7 @@ int cman_dispatch(cman_handle_t handle, int flags)
 			len = smsg->msg->length;
 			free(smsg->msg);
 			free(smsg);
-			if (res || (flags & CMAN_DISPATCH_ONE))
+			if (res || (flags & CMAN_DISPATCH_TYPE_MASK) == CMAN_DISPATCH_ONE)
 				break;
 			else
 				continue;
@@ -495,7 +495,7 @@ int cman_dispatch(cman_handle_t handle, int flags)
 			len = smsg->msg->length;
 			free(smsg->msg);
 			free(smsg);
-			if (res || (flags & CMAN_DISPATCH_ONE))
+			if (res || (flags & CMAN_DISPATCH_TYPE_MASK) == CMAN_DISPATCH_ONE)
 				break;
 			else
 				continue;
-- 
1.7.4.4

[Cluster-devel] [PATCH 02/41] libcman: fix fd usage

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 9ea82c1... 46f5c89... M	cman/lib/libcman.c
 cman/lib/libcman.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 9ea82c1..46f5c89 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -318,8 +318,8 @@ static cman_handle_t open_socket(const char *name, int namelen, void *privdata)
 		free(h);
 		h = NULL;
 		errno = saved_errno;
-	}
-	fcntl(h->zero_fd, F_SETFD, 1); /* Set close-on-exec */
+	} else
+		fcntl(h->zero_fd, F_SETFD, 1); /* Set close-on-exec */
 
 	return (cman_handle_t)h;
 }
-- 
1.7.4.4

[Cluster-devel] [PATCH 02/41] libcman: fix fd usage

[Steven Whitehouse]

Hi,

On Wed, 2011-11-23 at 11:15 +0100, Fabio M. Di Nitto wrote:
> Spotted by Coverity Scan
> 
> Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> ---
> :100644 100644 9ea82c1... 46f5c89... M	cman/lib/libcman.c
>  cman/lib/libcman.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
> index 9ea82c1..46f5c89 100644
> --- a/cman/lib/libcman.c
> +++ b/cman/lib/libcman.c
> @@ -318,8 +318,8 @@ static cman_handle_t open_socket(const char *name, int namelen, void *privdata)
>  		free(h);
>  		h = NULL;
>  		errno = saved_errno;
> -	}
> -	fcntl(h->zero_fd, F_SETFD, 1); /* Set close-on-exec */
> +	} else
> +		fcntl(h->zero_fd, F_SETFD, 1); /* Set close-on-exec */
FD_CLOEXEC would be a bit clearer than 1, even with the comment. Also
its normal to do a F_GETFD first and OR the new flag so that you don't
accidentally reset any other flags that are set on the fd beforehand.
I'm not sure whether that makes sense in this particular case though,

Steve.

>  
>  	return (cman_handle_t)h;
>  }

[Cluster-devel] [PATCH 02/41] libcman: fix fd usage

[Fabio M. Di Nitto]

On 11/23/2011 11:32 AM, Steven Whitehouse wrote:
> Hi,
> 
> On Wed, 2011-11-23 at 11:15 +0100, Fabio M. Di Nitto wrote:
>> Spotted by Coverity Scan
>>
>> Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
>> ---
>> :100644 100644 9ea82c1... 46f5c89... M	cman/lib/libcman.c
>>  cman/lib/libcman.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
>> index 9ea82c1..46f5c89 100644
>> --- a/cman/lib/libcman.c
>> +++ b/cman/lib/libcman.c
>> @@ -318,8 +318,8 @@ static cman_handle_t open_socket(const char *name, int namelen, void *privdata)
>>  		free(h);
>>  		h = NULL;
>>  		errno = saved_errno;
>> -	}
>> -	fcntl(h->zero_fd, F_SETFD, 1); /* Set close-on-exec */
>> +	} else
>> +		fcntl(h->zero_fd, F_SETFD, 1); /* Set close-on-exec */
> FD_CLOEXEC would be a bit clearer than 1, even with the comment. Also
> its normal to do a F_GETFD first and OR the new flag so that you don't
> accidentally reset any other flags that are set on the fd beforehand.
> I'm not sure whether that makes sense in this particular case though,
> 
> Steve.


Good catch... will change that for final submission

Fabio

[Cluster-devel] [PATCH 03/41] libcman: fix variable type

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 46f5c89... fbf5bf8... M	cman/lib/libcman.c
 cman/lib/libcman.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 46f5c89..fbf5bf8 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -232,7 +232,7 @@ static int loopy_writev(int fd, struct iovec *iovptr, size_t iovlen)
 static int send_message(struct cman_handle *h, int msgtype, const void *inbuf, int inlen)
 {
 	struct sock_header header;
-	size_t len;
+	int len;
 	struct iovec iov[2];
 	size_t iovlen = 1;
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 04/41] libcman: fix possible memory leak

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 fbf5bf8... 5b015f0... M	cman/lib/libcman.c
 cman/lib/libcman.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index fbf5bf8..5b015f0 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -533,16 +533,24 @@ int cman_dispatch(cman_handle_t handle, int flags)
 		{
 			len = read(h->fd, bufptr+offset, header->length-offset);
 			if (len == 0) {
+				if (bufptr != buf)
+					free(bufptr);
 				errno = EHOSTDOWN;
 				return -1;
 			}
 
 			if (len < 0 &&
-			    (errno == EINTR || errno == EAGAIN))
+			    (errno == EINTR || errno == EAGAIN)) {
+				if (bufptr != buf)
+					free(bufptr);
 				return 0;
+			}
 
-			if (len < 0)
+			if (len < 0) {
+				if (bufptr != buf)
+					free(bufptr);
 				return -1;
+			}
 			offset += len;
 		}
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 05/41] libcman: correctly check for vars before using them

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 5b015f0... ae6c709... M	cman/lib/libcman.c
 cman/lib/libcman.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 5b015f0..ae6c709 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -1018,12 +1018,6 @@ static int cman_set_quorum_device(cman_handle_t handle,
 	char buf[strlen(name)+1 + sizeof(int)];
 	VALIDATE_HANDLE(h);
 
-	if ((!name) || (strlen(name) > MAX_CLUSTER_MEMBER_NAME_LEN) || (votes < 0))
-	{
-		errno = EINVAL;
-		return -1;
-	}
-
 	memcpy(buf, &votes, sizeof(int));
 	strcpy(buf+sizeof(int), name);
 	return info_call(h, ops, buf, strlen(name)+1+sizeof(int), NULL, 0);
@@ -1031,6 +1025,11 @@ static int cman_set_quorum_device(cman_handle_t handle,
 
 int cman_register_quorum_device(cman_handle_t handle, char *name, int votes)
 {
+	if ((!name) || (strlen(name) > MAX_CLUSTER_MEMBER_NAME_LEN) || (votes < 0))
+	{
+		errno = EINVAL;
+		return -1;
+	}
 	return cman_set_quorum_device(handle, CMAN_CMD_REG_QUORUMDEV, name, votes);
 }
 
@@ -1069,6 +1068,11 @@ int cman_get_quorum_device(cman_handle_t handle, struct cman_qdev_info *info)
 
 int cman_update_quorum_device(cman_handle_t handle, char *name, int votes)
 {
+	if ((!name) || (strlen(name) > MAX_CLUSTER_MEMBER_NAME_LEN) || (votes < 0))
+	{
+		errno = EINVAL;
+		return -1;
+	}
 	return cman_set_quorum_device(handle, CMAN_CMD_UPDATE_QUORUMDEV, name, votes);
 }
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 06/41] libcman: switch to strncpy to avoid possible buffer overflows

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 ae6c709... b49cf6f... M	cman/lib/libcman.c
 cman/lib/libcman.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index ae6c709..b49cf6f 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -82,7 +82,7 @@ static void copy_node(cman_node_t *unode, struct cl_cluster_node *knode)
 {
 	unode->cn_nodeid = knode->node_id;
 	unode->cn_member = knode->state == NODESTATE_MEMBER?1:0;
-	strcpy(unode->cn_name, knode->name);
+	strncpy(unode->cn_name, knode->name, sizeof(unode->cn_name) - 1);
 	unode->cn_incarnation = knode->incarnation;
 	unode->cn_jointime = knode->jointime;
 
@@ -678,14 +678,14 @@ int cman_get_node(cman_handle_t handle, int nodeid, cman_node_t *node)
 	int status;
 	VALIDATE_HANDLE(h);
 
-	if (!node || strlen(node->cn_name) > sizeof(cman_node.name))
+	if (!node || strlen(node->cn_name) >= sizeof(cman_node.name))
 	{
 		errno = EINVAL;
 		return -1;
 	}
 
 	cman_node.node_id = nodeid;
-	strcpy(cman_node.name, node->cn_name);
+	strncpy(cman_node.name, node->cn_name, sizeof(cman_node.name) - 1);
 	status = info_call(h, CMAN_CMD_GETNODE, &cman_node, sizeof(struct cl_cluster_node),
 			   &cman_node, sizeof(struct cl_cluster_node));
 	if (status < 0)
@@ -905,14 +905,14 @@ int cman_barrier_register(cman_handle_t handle, const char *name, int flags, int
 	struct cl_barrier_info binfo;
 	VALIDATE_HANDLE(h);
 
-	if (strlen(name) > MAX_BARRIER_NAME_LEN)
+	if (strlen(name) >= MAX_BARRIER_NAME_LEN)
 	{
 		errno = EINVAL;
 		return -1;
 	}
 
 	binfo.cmd = BARRIER_CMD_REGISTER;
-	strcpy(binfo.name, name);
+	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
 	binfo.arg = nodes;
 	binfo.flags = flags;
 
@@ -926,14 +926,14 @@ int cman_barrier_change(cman_handle_t handle, const char *name, int flags, int a
 	struct cl_barrier_info binfo;
 	VALIDATE_HANDLE(h);
 
-	if (strlen(name) > MAX_BARRIER_NAME_LEN)
+	if (strlen(name) >= MAX_BARRIER_NAME_LEN)
 	{
 		errno = EINVAL;
 		return -1;
 	}
 
 	binfo.cmd = BARRIER_CMD_CHANGE;
-	strcpy(binfo.name, name);
+	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
 	binfo.arg = arg;
 	binfo.flags = flags;
 
@@ -947,14 +947,14 @@ int cman_barrier_wait(cman_handle_t handle, const char *name)
 	struct cl_barrier_info binfo;
 	VALIDATE_HANDLE(h);
 
-	if (strlen(name) > MAX_BARRIER_NAME_LEN)
+	if (strlen(name) >= MAX_BARRIER_NAME_LEN)
 	{
 		errno = EINVAL;
 		return -1;
 	}
 
 	binfo.cmd = BARRIER_CMD_WAIT;
-	strcpy(binfo.name, name);
+	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
 
 	return info_call(h, CMAN_CMD_BARRIER, &binfo, sizeof(binfo), NULL, 0);
 }
@@ -965,14 +965,14 @@ int cman_barrier_delete(cman_handle_t handle, const char *name)
 	struct cl_barrier_info binfo;
 	VALIDATE_HANDLE(h);
 
-	if (strlen(name) > MAX_BARRIER_NAME_LEN)
+	if (strlen(name) >= MAX_BARRIER_NAME_LEN)
 	{
 		errno = EINVAL;
 		return -1;
 	}
 
 	binfo.cmd = BARRIER_CMD_DELETE;
-	strcpy(binfo.name, name);
+	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
 
 	return info_call(h, CMAN_CMD_BARRIER, &binfo, sizeof(binfo), NULL, 0);
 }
@@ -1019,7 +1019,7 @@ static int cman_set_quorum_device(cman_handle_t handle,
 	VALIDATE_HANDLE(h);
 
 	memcpy(buf, &votes, sizeof(int));
-	strcpy(buf+sizeof(int), name);
+	strncpy(buf+sizeof(int), name, strlen(name)+1 + sizeof(int) - 1);
 	return info_call(h, ops, buf, strlen(name)+1+sizeof(int), NULL, 0);
 }
 
@@ -1059,7 +1059,7 @@ int cman_get_quorum_device(cman_handle_t handle, struct cman_qdev_info *info)
 	cman_node.node_id = CLUSTER_GETNODE_QUORUMDEV;
 	ret = info_call(h, CMAN_CMD_GETNODE, &cman_node, sizeof(cman_node), &cman_node, sizeof(cman_node));
 	if (!ret) {
-		strcpy(info->qi_name, cman_node.name);
+		strncpy(info->qi_name, cman_node.name, sizeof(info->qi_name) - 1);
 		info->qi_state = cman_node.state;
 		info->qi_votes = cman_node.votes;
 	}
@@ -1087,7 +1087,7 @@ int cman_get_fenceinfo(cman_handle_t handle, int nodeid, uint64_t *time, int *fe
 	if (!ret) {
 		*time = f.fence_time;
 		if (agent)
-			strcpy(agent, f.fence_agent);
+			strncpy(agent, f.fence_agent, sizeof(f.fence_agent) - 1);
 		*fenced = ((f.flags & FENCE_FLAGS_FENCED) != 0);
 	}
 	return ret;
@@ -1131,6 +1131,6 @@ int cman_node_fenced(cman_handle_t handle, int nodeid, uint64_t time, char *agen
 
 	f.nodeid = nodeid;
 	f.fence_time = time;
-	strcpy(f.fence_agent, agent);
+	strncpy(f.fence_agent, agent, sizeof(f.fence_agent) - 1);
 	return info_call(h, CMAN_CMD_UPDATE_FENCE_INFO, &f, sizeof(f), NULL, 0);
 }
-- 
1.7.4.4

[Cluster-devel] [PATCH 07/41] cman_tool: make cman_error static and make it use it's arg instead of global errno

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 e1250ee... 60091c9... M	cman/cman_tool/cman_tool.h
:100644 100644 a33c8b4... 872528b... M	cman/cman_tool/join.c
:100644 100644 88612c4... a336c42... M	cman/cman_tool/main.c
 cman/cman_tool/cman_tool.h |    2 --
 cman/cman_tool/join.c      |    1 +
 cman/cman_tool/main.c      |   37 +++++++++++++++++++------------------
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/cman/cman_tool/cman_tool.h b/cman/cman_tool/cman_tool.h
index e1250ee..60091c9 100644
--- a/cman/cman_tool/cman_tool.h
+++ b/cman/cman_tool/cman_tool.h
@@ -12,7 +12,6 @@
 #include <arpa/inet.h>
 #include <net/if.h>
 #include <stdio.h>
-#include <errno.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stddef.h>
@@ -107,6 +106,5 @@ struct commandline
 typedef struct commandline commandline_t;
 
 int join(commandline_t *comline, char *envp[]);
-const char *cman_error(int err);
 
 #endif  /*  __CMAN_TOOL_DOT_H__  */
diff --git a/cman/cman_tool/join.c b/cman/cman_tool/join.c
index a33c8b4..872528b 100644
--- a/cman/cman_tool/join.c
+++ b/cman/cman_tool/join.c
@@ -1,3 +1,4 @@
+#include <errno.h>
 #include <sys/wait.h>
 #include <stdint.h>
 #include <signal.h>
diff --git a/cman/cman_tool/main.c b/cman/cman_tool/main.c
index 88612c4..a336c42 100644
--- a/cman/cman_tool/main.c
+++ b/cman/cman_tool/main.c
@@ -1,3 +1,4 @@
+#include <errno.h>
 #include <inttypes.h>
 #include <unistd.h>
 #include <signal.h>
@@ -194,6 +195,24 @@ static char *membership_state(char *buf, int buflen, int node_state)
 	return buf;
 }
 
+static const char *cman_error(int err)
+{
+	const char *die_error;
+
+	switch (err) {
+	case ENOTCONN:
+		die_error = "Cluster software not started";
+		break;
+	case ENOENT:
+		die_error = "Node is not yet a cluster member";
+		break;
+	default:
+		die_error = strerror(err);
+		break;
+	}
+	return die_error;
+}
+
 static void show_status(void)
 {
 	cman_cluster_t info;
@@ -557,24 +576,6 @@ static int show_services(void)
 }
 
 
-const char *cman_error(int err)
-{
-	const char *die_error;
-
-	switch (errno) {
-	case ENOTCONN:
-		die_error = "Cluster software not started";
-		break;
-	case ENOENT:
-		die_error = "Node is not yet a cluster member";
-		break;
-	default:
-		die_error = strerror(errno);
-		break;
-	}
-	return die_error;
-}
-
 static void leave(commandline_t *comline)
 {
 	cman_handle_t h;
-- 
1.7.4.4

[Cluster-devel] [PATCH 08/41] cman_tool: don't use envp from main

[Fabio M. Di Nitto]

according to wikipedia it is a microsoft extensions. Use __environ
directly from unistd.h

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 60091c9... 6c5744e... M	cman/cman_tool/cman_tool.h
:100644 100644 872528b... b92090c... M	cman/cman_tool/join.c
:100644 100644 a336c42... 6d8a1eb... M	cman/cman_tool/main.c
 cman/cman_tool/cman_tool.h |    3 +--
 cman/cman_tool/join.c      |   13 +++++++------
 cman/cman_tool/main.c      |   10 +++++-----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/cman/cman_tool/cman_tool.h b/cman/cman_tool/cman_tool.h
index 60091c9..6c5744e 100644
--- a/cman/cman_tool/cman_tool.h
+++ b/cman/cman_tool/cman_tool.h
@@ -19,7 +19,6 @@
 #include <fcntl.h>
 #include <netdb.h>
 #include <limits.h>
-#include <unistd.h>
 
 extern char *prog_name;
 
@@ -105,6 +104,6 @@ struct commandline
 };
 typedef struct commandline commandline_t;
 
-int join(commandline_t *comline, char *envp[]);
+int join(commandline_t *comline);
 
 #endif  /*  __CMAN_TOOL_DOT_H__  */
diff --git a/cman/cman_tool/join.c b/cman/cman_tool/join.c
index 872528b..b92090c 100644
--- a/cman/cman_tool/join.c
+++ b/cman/cman_tool/join.c
@@ -1,4 +1,5 @@
 #include <errno.h>
+#include <unistd.h>
 #include <sys/wait.h>
 #include <stdint.h>
 #include <signal.h>
@@ -119,7 +120,7 @@ static int check_corosync_status(pid_t pid)
 	return status;
 }
 
-int join(commandline_t *comline, char *main_envp[])
+int join(commandline_t *comline)
 {
 	int i, err;
 	int envptr = 0;
@@ -205,9 +206,9 @@ int join(commandline_t *comline, char *main_envp[])
 
 	/* Copy any COROSYNC_* env variables to the new daemon */
 	i=0;
-	while (i < MAX_ARGS && main_envp[i]) {
-		if (strncmp(main_envp[i], "COROSYNC_", 9) == 0)
-			envp[envptr++] = main_envp[i];
+	while (i < MAX_ARGS && __environ[i]) {
+		if (strncmp(__environ[i], "COROSYNC_", 9) == 0)
+			envp[envptr++] = __environ[i];
 		i++;
 	}
 
@@ -363,7 +364,7 @@ int join(commandline_t *comline, char *main_envp[])
 	res = confdb_object_create(confdb_handle, OBJECT_PARENT_HANDLE, "cman_private", strlen("cman_private"), &object_handle);
 	if (res == CS_OK) {
 		int envnum = 0;
-		const char *envvar = main_envp[envnum];
+		const char *envvar = __environ[envnum];
 		const char *equal;
 		char envname[PATH_MAX];
 
@@ -381,7 +382,7 @@ int join(commandline_t *comline, char *main_envp[])
 					}
 				}
 			}
-			envvar = main_envp[++envnum];
+			envvar = __environ[++envnum];
 		}
 	}
 	res = confdb_key_create_typed(confdb_handle, object_handle,
diff --git a/cman/cman_tool/main.c b/cman/cman_tool/main.c
index a336c42..6d8a1eb 100644
--- a/cman/cman_tool/main.c
+++ b/cman/cman_tool/main.c
@@ -1190,7 +1190,7 @@ static void check_arguments(commandline_t *comline)
 }
 
 
-static void do_join(commandline_t *comline, char *envp[])
+static void do_join(commandline_t *comline)
 {
 	int ret;
 
@@ -1212,18 +1212,18 @@ static void do_join(commandline_t *comline, char *envp[])
 			die("Not joining, configuration is not valid\n");
 	}
 
-	join(comline, envp);
+	join(comline);
 	if (comline->wait_opt || comline->wait_quorate_opt) {
 		do {
 			ret = cluster_wait(comline);
 			if (ret == ENOTCONN)
-				join(comline, envp);
+				join(comline);
 
 		} while (ret == ENOTCONN);
 	}
 }
 
-int main(int argc, char *argv[], char *envp[])
+int main(int argc, char *argv[])
 {
 	commandline_t comline;
 
@@ -1235,7 +1235,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	switch (comline.operation) {
 	case OP_JOIN:
-		do_join(&comline, envp);
+		do_join(&comline);
 		break;
 
 	case OP_LEAVE:
-- 
1.7.4.4

[Cluster-devel] [PATCH 08/41] cman_tool: don't use envp from main

[Steven Whitehouse]

Hi,

On Wed, 2011-11-23 at 11:15 +0100, Fabio M. Di Nitto wrote:
> according to wikipedia it is a microsoft extensions. Use __environ
> directly from unistd.h
> 
> Spotted by Coverity Scan
> 
> Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> ---
> :100644 100644 60091c9... 6c5744e... M	cman/cman_tool/cman_tool.h
> :100644 100644 872528b... b92090c... M	cman/cman_tool/join.c
> :100644 100644 a336c42... 6d8a1eb... M	cman/cman_tool/main.c
>  cman/cman_tool/cman_tool.h |    3 +--
>  cman/cman_tool/join.c      |   13 +++++++------
>  cman/cman_tool/main.c      |   10 +++++-----
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/cman/cman_tool/cman_tool.h b/cman/cman_tool/cman_tool.h
> index 60091c9..6c5744e 100644
> --- a/cman/cman_tool/cman_tool.h
> +++ b/cman/cman_tool/cman_tool.h
> @@ -19,7 +19,6 @@
>  #include <fcntl.h>
>  #include <netdb.h>
>  #include <limits.h>
> -#include <unistd.h>
>  
>  extern char *prog_name;
>  
> @@ -105,6 +104,6 @@ struct commandline
>  };
>  typedef struct commandline commandline_t;
>  
> -int join(commandline_t *comline, char *envp[]);
> +int join(commandline_t *comline);
>  
>  #endif  /*  __CMAN_TOOL_DOT_H__  */
> diff --git a/cman/cman_tool/join.c b/cman/cman_tool/join.c
> index 872528b..b92090c 100644
> --- a/cman/cman_tool/join.c
> +++ b/cman/cman_tool/join.c
> @@ -1,4 +1,5 @@
>  #include <errno.h>
> +#include <unistd.h>
>  #include <sys/wait.h>
>  #include <stdint.h>
>  #include <signal.h>
> @@ -119,7 +120,7 @@ static int check_corosync_status(pid_t pid)
>  	return status;
>  }
>  
> -int join(commandline_t *comline, char *main_envp[])
> +int join(commandline_t *comline)
>  {
>  	int i, err;
>  	int envptr = 0;
> @@ -205,9 +206,9 @@ int join(commandline_t *comline, char *main_envp[])
>  
>  	/* Copy any COROSYNC_* env variables to the new daemon */
>  	i=0;
> -	while (i < MAX_ARGS && main_envp[i]) {
> -		if (strncmp(main_envp[i], "COROSYNC_", 9) == 0)
> -			envp[envptr++] = main_envp[i];
> +	while (i < MAX_ARGS && __environ[i]) {
> +		if (strncmp(__environ[i], "COROSYNC_", 9) == 0)
> +			envp[envptr++] = __environ[i];
>  		i++;
>  	}
Why not just use getenv() ?

Steve.

[Cluster-devel] [PATCH 08/41] cman_tool: don't use envp from main

[Fabio M. Di Nitto]

On 11/23/2011 11:28 AM, Steven Whitehouse wrote:
> Hi,
> 
> On Wed, 2011-11-23 at 11:15 +0100, Fabio M. Di Nitto wrote:
>> according to wikipedia it is a microsoft extensions. Use __environ
>> directly from unistd.h
>>
>> Spotted by Coverity Scan
>>
>> Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
>> ---
>> :100644 100644 60091c9... 6c5744e... M	cman/cman_tool/cman_tool.h
>> :100644 100644 872528b... b92090c... M	cman/cman_tool/join.c
>> :100644 100644 a336c42... 6d8a1eb... M	cman/cman_tool/main.c
>>  cman/cman_tool/cman_tool.h |    3 +--
>>  cman/cman_tool/join.c      |   13 +++++++------
>>  cman/cman_tool/main.c      |   10 +++++-----
>>  3 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/cman/cman_tool/cman_tool.h b/cman/cman_tool/cman_tool.h
>> index 60091c9..6c5744e 100644
>> --- a/cman/cman_tool/cman_tool.h
>> +++ b/cman/cman_tool/cman_tool.h
>> @@ -19,7 +19,6 @@
>>  #include <fcntl.h>
>>  #include <netdb.h>
>>  #include <limits.h>
>> -#include <unistd.h>
>>  
>>  extern char *prog_name;
>>  
>> @@ -105,6 +104,6 @@ struct commandline
>>  };
>>  typedef struct commandline commandline_t;
>>  
>> -int join(commandline_t *comline, char *envp[]);
>> +int join(commandline_t *comline);
>>  
>>  #endif  /*  __CMAN_TOOL_DOT_H__  */
>> diff --git a/cman/cman_tool/join.c b/cman/cman_tool/join.c
>> index 872528b..b92090c 100644
>> --- a/cman/cman_tool/join.c
>> +++ b/cman/cman_tool/join.c
>> @@ -1,4 +1,5 @@
>>  #include <errno.h>
>> +#include <unistd.h>
>>  #include <sys/wait.h>
>>  #include <stdint.h>
>>  #include <signal.h>
>> @@ -119,7 +120,7 @@ static int check_corosync_status(pid_t pid)
>>  	return status;
>>  }
>>  
>> -int join(commandline_t *comline, char *main_envp[])
>> +int join(commandline_t *comline)
>>  {
>>  	int i, err;
>>  	int envptr = 0;
>> @@ -205,9 +206,9 @@ int join(commandline_t *comline, char *main_envp[])
>>  
>>  	/* Copy any COROSYNC_* env variables to the new daemon */
>>  	i=0;
>> -	while (i < MAX_ARGS && main_envp[i]) {
>> -		if (strncmp(main_envp[i], "COROSYNC_", 9) == 0)
>> -			envp[envptr++] = main_envp[i];
>> +	while (i < MAX_ARGS && __environ[i]) {
>> +		if (strncmp(__environ[i], "COROSYNC_", 9) == 0)
>> +			envp[envptr++] = __environ[i];
>>  		i++;
>>  	}
> Why not just use getenv() ?

getenv implies that you know the values you are looking for.

This function is generic and look for everything that starts with
COROSYNC_ in the environment.

Fabio

[Cluster-devel] [PATCH 09/41] cman_tool: prevent buffer overrun

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 6d8a1eb... 5a5aa7b... M	cman/cman_tool/main.c
 cman/cman_tool/main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cman/cman_tool/main.c b/cman/cman_tool/main.c
index 6d8a1eb..5a5aa7b 100644
--- a/cman/cman_tool/main.c
+++ b/cman/cman_tool/main.c
@@ -641,7 +641,7 @@ static void set_votes(commandline_t *comline)
 	else {
 		/* Resolve node name into a number */
 		node.cn_nodeid = 0;
-		strcpy(node.cn_name, comline->nodenames[0]);
+		strncpy(node.cn_name, comline->nodenames[0], CMAN_MAX_NODENAME_LEN);
 		if (cman_get_node(h, node.cn_nodeid, &node))
 			die("Can't set votes for node %s : %s\n", node.cn_name, strerror(errno));
 		nodeid = node.cn_nodeid;
@@ -864,7 +864,7 @@ static void kill_node(commandline_t *comline)
 
 	    /* Resolve node name into a number */
 	    node.cn_nodeid = 0;
-	    strcpy(node.cn_name, comline->nodenames[i]);
+	    strncpy(node.cn_name, comline->nodenames[i], CMAN_MAX_NODENAME_LEN);
 	    if (cman_get_node(h, node.cn_nodeid, &node)) {
 		fprintf(stderr, "Can't kill node %s : %s\n", node.cn_name, strerror(errno));
 		continue;
-- 
1.7.4.4

[Cluster-devel] [PATCH 10/41] cman_tool: check that memory is allocated

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 5a5aa7b... 46c315d... M	cman/cman_tool/main.c
 cman/cman_tool/main.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/cman/cman_tool/main.c b/cman/cman_tool/main.c
index 5a5aa7b..46c315d 100644
--- a/cman/cman_tool/main.c
+++ b/cman/cman_tool/main.c
@@ -539,6 +539,9 @@ static void show_nodes(commandline_t *comline)
 	/* Get Disallowed nodes, so we can show them as such */
 	dis_nodes = malloc(sizeof(cman_node_t) * count);
 
+	if (!dis_nodes)
+		die("cannot allocate memory for disallowed node\n");
+
 	if (cman_get_disallowed_nodes(h, count, &dis_count, dis_nodes) == 0) {
 		for (i = 0; i < numnodes; i++) {
 			for (j = 0; j < dis_count; j++) {
-- 
1.7.4.4

[Cluster-devel] [PATCH 11/41] cman_tool: drop unused vars and fix value check

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 b92090c... a1c1c14... M	cman/cman_tool/join.c
:100644 100644 46c315d... 0e8876a... M	cman/cman_tool/main.c
 cman/cman_tool/join.c |   10 +++++-----
 cman/cman_tool/main.c |    6 ++----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/cman/cman_tool/join.c b/cman/cman_tool/join.c
index b92090c..a1c1c14 100644
--- a/cman/cman_tool/join.c
+++ b/cman/cman_tool/join.c
@@ -122,7 +122,7 @@ static int check_corosync_status(pid_t pid)
 
 int join(commandline_t *comline)
 {
-	int i, err;
+	int i;
 	int envptr = 0;
 	int argvptr = 0;
 	char scratch[1024];
@@ -252,13 +252,13 @@ int join(commandline_t *comline)
 		be_daemon();
 
 		sprintf(scratch, "FORKED: %d\n", getpid());
-		err = write(p[1], scratch, strlen(scratch));
+		write(p[1], scratch, strlen(scratch));
 
 		execve(COROSYNCBIN, argv, envp);
 
 		/* exec failed - tell the parent process */
 		sprintf(scratch, "execve of " COROSYNCBIN " failed: %s", strerror(errno));
-		err = write(p[1], scratch, strlen(scratch));
+		write(p[1], scratch, strlen(scratch));
 		exit(1);
 		break;
 
@@ -290,7 +290,7 @@ int join(commandline_t *comline)
 			if ((len = read(p[0], message, sizeof(message)) > 0)) {
 
 				/* Forked OK - get the real corosync pid */
-				if (sscanf(messageptr, "FORKED: %d", &corosync_pid) == 1) {
+				if ((messageptr) && (sscanf(messageptr, "FORKED: %d", &corosync_pid) == 1)) {
 					if (comline->verbose & DEBUG_STARTUP_ONLY)
 						fprintf(stderr, "forked process ID is %d\n", corosync_pid);
 					status = 0;
@@ -303,7 +303,7 @@ int join(commandline_t *comline)
 						continue;
 				}
 				/* Success! get the new PID of double-forked corosync */
-				if (sscanf(messageptr, "SUCCESS: %d", &corosync_pid) == 1) {
+				if ((messageptr) && (sscanf(messageptr, "SUCCESS: %d", &corosync_pid) == 1)) {
 					if (comline->verbose & DEBUG_STARTUP_ONLY)
 						fprintf(stderr, "corosync running, process ID is %d\n", corosync_pid);
 					status = 0;
diff --git a/cman/cman_tool/main.c b/cman/cman_tool/main.c
index 46c315d..0e8876a 100644
--- a/cman/cman_tool/main.c
+++ b/cman/cman_tool/main.c
@@ -618,11 +618,10 @@ static void leave(commandline_t *comline)
 static void set_expected(commandline_t *comline)
 {
 	cman_handle_t h;
-	int result;
 
 	h = open_cman_handle(1);
 
-	if ((result = cman_set_expected_votes(h, comline->expected_votes)))
+	if (cman_set_expected_votes(h, comline->expected_votes))
 		die("can't set expected votes: %s", cman_error(errno));
 
 	cman_finish(h);
@@ -631,7 +630,6 @@ static void set_expected(commandline_t *comline)
 static void set_votes(commandline_t *comline)
 {
 	cman_handle_t h;
-	int result;
 	int nodeid;
 	struct cman_node node;
 
@@ -650,7 +648,7 @@ static void set_votes(commandline_t *comline)
 		nodeid = node.cn_nodeid;
 	}
 
-	if ((result = cman_set_votes(h, comline->votes, nodeid)))
+	if (cman_set_votes(h, comline->votes, nodeid))
 		die("can't set votes: %s", cman_error(errno));
 
 	cman_finish(h);
-- 
1.7.4.4

[Cluster-devel] [PATCH 12/41] cman: add check to guarantee we found our own node name

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 16ef65f... f23f386... M	cman/daemon/cmanconfig.c
 cman/daemon/cmanconfig.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/cman/daemon/cmanconfig.c b/cman/daemon/cmanconfig.c
index 16ef65f..f23f386 100644
--- a/cman/daemon/cmanconfig.c
+++ b/cman/daemon/cmanconfig.c
@@ -71,7 +71,15 @@ int read_cman_nodes(struct corosync_api_v1 *corosync, unsigned int *config_versi
 	    objdb_get_int(corosync, object_handle, "expected_votes", &expected, 0);
 	    objdb_get_int(corosync, object_handle, "two_node", (unsigned int *)&two_node, 0);
 	    objdb_get_int(corosync, object_handle, "cluster_id", &cluster_id, 0);
-	    objdb_get_string(corosync, object_handle, "nodename", &our_nodename);
+	    if (objdb_get_string(corosync, object_handle, "nodename", &our_nodename) != 0) {
+		char message[132];
+		snprintf(message, sizeof(message),
+			 "cman was unable to determine our node name!\n");
+		log_printf(LOG_ERR, "%s", message);
+		write_cman_pipe(message);
+		error = -EINVAL;
+		goto out_err;
+	    }
 	    objdb_get_int(corosync, object_handle, "max_queued", &max_outstanding_messages, DEFAULT_MAX_QUEUED);
     }
     corosync->object_find_destroy(find_handle);
-- 
1.7.4.4

[Cluster-devel] [PATCH 13/41] cman: drop dead code and fix code logic

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 f4362cf... 7fb3005... M	cman/daemon/barrier.c
 cman/daemon/barrier.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/cman/daemon/barrier.c b/cman/daemon/barrier.c
index f4362cf..7fb3005 100644
--- a/cman/daemon/barrier.c
+++ b/cman/daemon/barrier.c
@@ -177,11 +177,11 @@ void process_barrier_msg(struct cl_barriermsg *msg,
 {
 	struct cl_barrier *barrier;
 
-	barrier = find_barrier(msg->name);
-
 	/* Ignore other peoples' messages */
 	if (!we_are_a_cluster_member)
 		return;
+
+	barrier = find_barrier(msg->name);
 	if (!barrier)
 		return;
 
@@ -200,8 +200,6 @@ void process_barrier_msg(struct cl_barriermsg *msg,
 		break;
 
 	case BARRIER_COMPLETE:
-		if (!barrier)
-			return;
 		/* Once we receive COMPLETE, we know that everyone has completed.
 		   I love VS */
 		barrier_complete_phase2(barrier, 0);
-- 
1.7.4.4

[Cluster-devel] [PATCH 14/41] cman_tool: fix a few possible buffer overflow

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 a1c1c14... 44c8d69... M	cman/cman_tool/join.c
:100644 100644 0e8876a... b0a2116... M	cman/cman_tool/main.c
 cman/cman_tool/join.c |    5 +++--
 cman/cman_tool/main.c |    6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/cman/cman_tool/join.c b/cman/cman_tool/join.c
index a1c1c14..44c8d69 100644
--- a/cman/cman_tool/join.c
+++ b/cman/cman_tool/join.c
@@ -94,7 +94,7 @@ static const char *corosync_exit_reason(signed char status)
 		return "Another Corosync instance is already running";
 		break;
 	default:
-		sprintf(reason, "Error, reason code is %d", status);
+		snprintf(reason, sizeof(reason) - 1, "Error, reason code is %d", status);
 		return reason;
 		break;
 	}
@@ -287,7 +287,8 @@ int join(commandline_t *comline)
 		/* Did we get a cman-reported error? */
 		if (status == 1) {
 			int len;
-			if ((len = read(p[0], message, sizeof(message)) > 0)) {
+			if ((len = read(p[0], message, sizeof(message) - 1) > 0)) {
+				message[sizeof(message) - 1] = '\0';
 
 				/* Forked OK - get the real corosync pid */
 				if ((messageptr) && (sscanf(messageptr, "FORKED: %d", &corosync_pid) == 1)) {
diff --git a/cman/cman_tool/main.c b/cman/cman_tool/main.c
index 0e8876a..b0a2116 100644
--- a/cman/cman_tool/main.c
+++ b/cman/cman_tool/main.c
@@ -188,7 +188,7 @@ static char *membership_state(char *buf, int buflen, int node_state)
 		strncpy(buf, "Leaving", buflen);
 		break;
 	default:
-		sprintf(buf, "Unknown: code=%d", node_state);
+		snprintf(buf, buflen - 1, "Unknown: code=%d", node_state);
 		break;
 	}
 
@@ -414,7 +414,7 @@ static void print_node(commandline_t *comline, cman_handle_t h, int *format, str
 	if (node->cn_jointime.tv_sec && node->cn_member)
 		strftime(jstring, sizeof(jstring), "%F %H:%M:%S", jtime);
 	else
-		strcpy(jstring, "                   ");
+		strncpy(jstring, "                   ", sizeof(jstring));
 
 	if (!comline->format_opts) {
 		printf("%4u   %c  %5d   %s  %s\n",
@@ -1018,7 +1018,7 @@ static void decode_arguments(int argc, char *argv[], commandline_t *comline)
 			if (strlen(optarg) > MAX_NODE_NAME_LEN-1)
 				die("maximum cluster name length is %d",
 				    MAX_CLUSTER_NAME_LEN-1);
-			strcpy(comline->clustername, optarg);
+			strncpy(comline->clustername, optarg, sizeof(comline->clustername) - 1);
 			comline->clustername_opt = TRUE;
 			break;
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 15/41] cman: fix a few var checks and types

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 fb534a9... 9d52fde... M	cman/daemon/cman-preconfig.c
:100644 100644 f23f386... d3009d0... M	cman/daemon/cmanconfig.c
:100644 100644 02fe88d... 1489f7c... M	cman/daemon/commands.c
:100644 100644 92cfd47... b85557d... M	cman/daemon/daemon.c
 cman/daemon/cman-preconfig.c |   12 +++++++++---
 cman/daemon/cmanconfig.c     |    2 +-
 cman/daemon/commands.c       |   10 ++++++++--
 cman/daemon/daemon.c         |    2 +-
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/cman/daemon/cman-preconfig.c b/cman/daemon/cman-preconfig.c
index fb534a9..9d52fde 100644
--- a/cman/daemon/cman-preconfig.c
+++ b/cman/daemon/cman-preconfig.c
@@ -458,7 +458,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 	/* If nodename (from uname) is domain-less, try to match against
 	   cluster.conf names which may have domainname specified */
 	nodes_handle = nodeslist_init(objdb, cluster_parent_handle, &find_handle);
-	do {
+	while (nodes_handle) {
 		int len;
 
 		if (objdb_get_string(objdb, nodes_handle, "name", &str)) {
@@ -480,7 +480,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 			return 0;
 		}
 		nodes_handle = nodeslist_next(objdb, find_handle);
-	} while (nodes_handle);
+	}
 	objdb->object_find_destroy(find_handle);
 
 
@@ -706,6 +706,12 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 	}
 	objdb->object_find_destroy(find_handle);
 
+	if (!nodeid_str) {
+		sprintf(error_reason, "This node has no nodeid in cluster.conf");
+		write_cman_pipe("This node has no nodeid in cluster.conf");
+		return -1;
+	}
+
 	nodeid = atoi(nodeid_str);
 	error = 0;
 
@@ -1024,7 +1030,7 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 			int keylen;
 			memset(tmp, 0, sizeof(tmp));
 
-			strcpy(tmp, cluster_name);
+			strncpy(tmp, cluster_name, sizeof(tmp) - 1);
 
 			/* Key length must be a multiple of 4 */
 			keylen = (strlen(cluster_name)+4) & 0xFC;
diff --git a/cman/daemon/cmanconfig.c b/cman/daemon/cmanconfig.c
index f23f386..d3009d0 100644
--- a/cman/daemon/cmanconfig.c
+++ b/cman/daemon/cmanconfig.c
@@ -238,7 +238,7 @@ static int get_cman_join_info(struct corosync_api_v1 *corosync)
 	if (!votes) {
 		unsigned int votestmp=-1;
 		objdb_get_int(corosync, node_object, "votes", &votestmp, 1);
-		if (votestmp < 0 || votestmp > 255) {
+		if (votestmp > 255) {
 			log_printf(LOG_ERR, "invalid votes value %d", votestmp);
 			write_cman_pipe("Found invalid votes for node in CCS");
 			return -EINVAL;
diff --git a/cman/daemon/commands.c b/cman/daemon/commands.c
index 02fe88d..1489f7c 100644
--- a/cman/daemon/commands.c
+++ b/cman/daemon/commands.c
@@ -457,8 +457,10 @@ int cman_join_cluster(struct corosync_api_v1 *api,
 	/* Make sure we have a node name */
 	if (nodename[0] == '\0') {
 		struct utsname un;
-		uname(&un);
-		strcpy(nodename, un.nodename);
+		if (uname(&un)) {
+			return -EINVAL;
+		}
+		strncpy(nodename, un.nodename, sizeof(nodename) - 1);
 	}
 
 	time(&join_time);
@@ -2314,6 +2316,10 @@ void add_ais_node(int nodeid, uint64_t incar, int total_members)
 		/* Emergency nodename */
 		sprintf(tempname, "Node%d", nodeid);
 		node = add_new_node(tempname, nodeid, 1, total_members, NODESTATE_DEAD);
+		if (!node) {
+			log_printf(LOG_ERR, "Unable to add newnode!\n");
+			return;
+		}
 	}
 
 	if (node->state == NODESTATE_DEAD || node->state == NODESTATE_LEAVING) {
diff --git a/cman/daemon/daemon.c b/cman/daemon/daemon.c
index 92cfd47..b85557d 100644
--- a/cman/daemon/daemon.c
+++ b/cman/daemon/daemon.c
@@ -353,7 +353,7 @@ static int process_rendezvous(hdb_handle_t handle, int fd, int revent, void *dat
 	return 0;
 }
 
-static int open_local_sock(const char *name, int name_len, mode_t mode, hdb_handle_t handle, int type)
+static int open_local_sock(const char *name, int name_len, mode_t mode, hdb_handle_t handle, con_type_t type)
 {
 	int local_socket;
 	struct sockaddr_un sockaddr;
-- 
1.7.4.4

[Cluster-devel] [PATCH 16/41] cman: drop unrequired/unused vars and functions

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 1489f7c... 7641294... M	cman/daemon/commands.c
:100644 100644 bbf2628... 4851387... M	cman/daemon/nodelist.h
 cman/daemon/commands.c |   18 ------------------
 cman/daemon/nodelist.h |    4 +---
 2 files changed, 1 insertions(+), 21 deletions(-)

diff --git a/cman/daemon/commands.c b/cman/daemon/commands.c
index 1489f7c..7641294 100644
--- a/cman/daemon/commands.c
+++ b/cman/daemon/commands.c
@@ -90,7 +90,6 @@ static int ccsd_timer_active = 0;
 static struct cluster_node *find_node_by_nodeid(int nodeid);
 static struct cluster_node *find_node_by_name(char *name);
 static int get_node_count(void);
-static int get_highest_nodeid(void);
 static int send_port_open_msg(unsigned char port);
 static int send_port_enquire(int nodeid);
 static void process_internal_message(char *data, int nodeid, int byteswap);
@@ -612,13 +611,10 @@ static int do_cmd_get_all_members(char *cmdbuf, char **retbuf, int retsize, int
 	char *outbuf = *retbuf + offset;
 	int num_nodes = 0;
 	int total_nodes = 0;
-	int highest_node;
 
 	if (!we_are_a_cluster_member)
 		return -ENOENT;
 
-	highest_node = get_highest_nodeid();
-
 	/* Count nodes */
 	list_iterate(nodelist, &cluster_members_list) {
 		total_nodes++;
@@ -1896,7 +1892,6 @@ void send_transition_msg(int last_memb_count, int first_trans)
 static void byteswap_internal_message(char *data)
 {
 	struct cl_protmsg *msg = (struct cl_protmsg *)data;
-	struct cl_barriermsg *barriermsg;
 	struct cl_killmsg *killmsg;
 	struct cl_leavemsg *leavemsg;
 	struct cl_transmsg *transmsg;
@@ -1934,7 +1929,6 @@ static void byteswap_internal_message(char *data)
 		break;
 
 	case CLUSTER_MSG_BARRIER:
-		barriermsg = (struct cl_barriermsg *)data;
 		break;
 
 	case CLUSTER_MSG_RECONFIGURE:
@@ -2388,18 +2382,6 @@ void del_ais_node(int nodeid)
 	}
 }
 
-static int get_highest_nodeid()
-{
-	int highest = 0;
-	struct cluster_node *node;
-
-	list_iterate_items(node, &cluster_members_list) {
-		if (node->node_id > highest)
-			highest = node->node_id;
-	}
-	return highest;
-}
-
 static int get_node_count()
 {
 	int count = 0;
diff --git a/cman/daemon/nodelist.h b/cman/daemon/nodelist.h
index bbf2628..4851387 100644
--- a/cman/daemon/nodelist.h
+++ b/cman/daemon/nodelist.h
@@ -2,10 +2,8 @@
 static inline int objdb_get_string(OBJDB_API *corosync, hdb_handle_t object_service_handle,
 				   const char *key, char **value)
 {
-	int res;
-
 	*value = NULL;
-	if ( !(res = corosync->object_key_get(object_service_handle,
+	if ( !(corosync->object_key_get(object_service_handle,
 					      key,
 					      strlen(key),
 					      (void *)value,
-- 
1.7.4.4

[Cluster-devel] [PATCH 17/41] cman: make 2 var const and allow backup defaults

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 9d52fde... 522994b... M	cman/daemon/cman-preconfig.c
 cman/daemon/cman-preconfig.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/cman/daemon/cman-preconfig.c b/cman/daemon/cman-preconfig.c
index 9d52fde..522994b 100644
--- a/cman/daemon/cman-preconfig.c
+++ b/cman/daemon/cman-preconfig.c
@@ -805,8 +805,8 @@ out:
 static void add_logging_overrides(struct objdb_iface_ver0 *objdb)
 {
 	char *logstr;
-	char *logfacility;
-	char *loglevel;
+	const char *logfacility;
+	const char *loglevel;
 	hdb_handle_t object_handle;
 	hdb_handle_t find_handle;
 
@@ -820,7 +820,11 @@ static void add_logging_overrides(struct objdb_iface_ver0 *objdb)
 	objdb->object_find_destroy(find_handle);
 
 	logfacility = facility_name_get(SYSLOGFACILITY);
+	if (!logfacility)
+		logfacility = "LOG_LOCAL4";
 	loglevel = priority_name_get(SYSLOGLEVEL);
+	if (!loglevel)
+		loglevel = "LOG_INFO";
 
 	/* enable timestamps on logging */
 	if (objdb_get_string(objdb, object_handle, "timestamp", &logstr)) {
-- 
1.7.4.4

[Cluster-devel] [PATCH 18/41] cman: move check of null at beginning

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 7fb3005... 37283ba... M	cman/daemon/barrier.c
 cman/daemon/barrier.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cman/daemon/barrier.c b/cman/daemon/barrier.c
index 7fb3005..37283ba 100644
--- a/cman/daemon/barrier.c
+++ b/cman/daemon/barrier.c
@@ -256,7 +256,7 @@ static int barrier_setattr_enabled(struct cl_barrier *barrier,
 	int status;
 
 	/* Can't disable a barrier */
-	if (!arg) {
+	if ((!barrier) || (!arg)) {
 		return -EINVAL;
 	}
 
@@ -285,7 +285,7 @@ static int barrier_setattr_enabled(struct cl_barrier *barrier,
 			return status;
 		}
 	}
-	if (barrier && barrier->state == BARRIER_STATE_COMPLETE) {
+	if (barrier->state == BARRIER_STATE_COMPLETE) {
 		return barrier->endreason;
 	}
 	return 0;	/* Nothing to propogate */
-- 
1.7.4.4

[Cluster-devel] [PATCH 19/41] cman: init structs before use

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 522994b... d632c17... M	cman/daemon/cman-preconfig.c
 cman/daemon/cman-preconfig.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/cman/daemon/cman-preconfig.c b/cman/daemon/cman-preconfig.c
index 522994b..d632c17 100644
--- a/cman/daemon/cman-preconfig.c
+++ b/cman/daemon/cman-preconfig.c
@@ -295,6 +295,10 @@ static int add_ifaddr(struct objdb_iface_ver0 *objdb, char *mcast, char *ifaddr,
 		[TX_MECH_RDMA] = "iba",
 	};
 
+	memset(&mcast_addr, 0, sizeof(mcast_addr));
+	memset(&localhost, 0, sizeof(localhost));
+	memset(&if_addr, 0, sizeof(if_addr));
+
 	/* Check the families match */
 	if (address_family(mcast, &mcast_addr, 0) !=
 	    address_family(ifaddr, &if_addr, mcast_addr.ss_family)) {
-- 
1.7.4.4

[Cluster-devel] [PATCH 20/41] cman: simplify code

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 7641294... 284b351... M	cman/daemon/commands.c
 cman/daemon/commands.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/cman/daemon/commands.c b/cman/daemon/commands.c
index 7641294..284b351 100644
--- a/cman/daemon/commands.c
+++ b/cman/daemon/commands.c
@@ -270,13 +270,12 @@ static struct cluster_node *add_new_node(char *name, int nodeid, int votes, int
 			newnode->incarnation = incarnation;
 	}
 	if (!newnode->name) {
-		newnode->name = malloc(strlen(name)+1);
+		newnode->name = strdup(name);
 		if (!newnode->name) {
 			if (newalloc)
 				free(newnode);
 			return NULL;
 		}
-		strcpy(newnode->name, name);
 	}
 
 	if (!newnode->node_id) /* Don't clobber existing nodeid */
-- 
1.7.4.4

[Cluster-devel] [PATCH 21/41] cman: fix free handle logic

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 284b351... 6b61332... M	cman/daemon/commands.c
 cman/daemon/commands.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cman/daemon/commands.c b/cman/daemon/commands.c
index 284b351..6b61332 100644
--- a/cman/daemon/commands.c
+++ b/cman/daemon/commands.c
@@ -563,8 +563,6 @@ static int do_cmd_get_extrainfo(char *cmdbuf, char **retbuf, int retsize, int *r
 	corosync->object_find_create(OBJECT_PARENT_HANDLE, "totem", strlen("totem"), &totem_find_handle);
 	if (corosync->object_find_next(totem_find_handle, &totem_object_handle) == 0) {
 
-		corosync->object_find_destroy(totem_find_handle);
-
 		corosync->object_find_create(totem_object_handle, "interface", strlen("interface"), &iface_find_handle);
 		while (corosync->object_find_next(iface_find_handle, &object_handle) == 0) {
 
@@ -589,8 +587,10 @@ static int do_cmd_get_extrainfo(char *cmdbuf, char **retbuf, int retsize, int *r
 			}
 			ptr += sizeof(struct sockaddr_storage);
 		}
+
+		corosync->object_find_destroy(iface_find_handle);
 	}
-	corosync->object_find_destroy(iface_find_handle);
+	corosync->object_find_destroy(totem_find_handle);
 
 	for (i=0; i<num_interfaces; i++) {
 		ss = (struct sockaddr_storage *)ptr;
-- 
1.7.4.4

[Cluster-devel] [PATCH 22/41] cman: fix several virtually impossible buffer overflows

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 6b5640a... ea5020c... M	cman/daemon/ais.c
:100644 100644 37283ba... c356abe... M	cman/daemon/barrier.c
:100644 100644 d632c17... baf94df... M	cman/daemon/cman-preconfig.c
:100644 100644 d3009d0... 7eb52e4... M	cman/daemon/cmanconfig.c
:100644 100644 6b61332... f397234... M	cman/daemon/commands.c
 cman/daemon/ais.c            |    2 +-
 cman/daemon/barrier.c        |    6 +-
 cman/daemon/cman-preconfig.c |  114 +++++++++++++++++++++---------------------
 cman/daemon/cmanconfig.c     |    2 +-
 cman/daemon/commands.c       |   18 +++---
 5 files changed, 71 insertions(+), 71 deletions(-)

diff --git a/cman/daemon/ais.c b/cman/daemon/ais.c
index 6b5640a..ea5020c 100644
--- a/cman/daemon/ais.c
+++ b/cman/daemon/ais.c
@@ -213,7 +213,7 @@ static int cman_exec_init_fn(struct corosync_api_v1 *api)
 	cman_init(api);
 
 	/* Let cman_tool know we are running and our PID */
-	sprintf(pipe_msg,"SUCCESS: %d", getpid());
+	snprintf(pipe_msg, sizeof(pipe_msg) - 1,"SUCCESS: %d", getpid());
 	write_cman_pipe(pipe_msg);
 	close(startup_pipe);
 	startup_pipe = 0;
diff --git a/cman/daemon/barrier.c b/cman/daemon/barrier.c
index 37283ba..c356abe 100644
--- a/cman/daemon/barrier.c
+++ b/cman/daemon/barrier.c
@@ -101,7 +101,7 @@ static void check_barrier_complete_phase1(struct cl_barrier *barrier)
 
 		bmsg.cmd = CLUSTER_MSG_BARRIER;
 		bmsg.subcmd = BARRIER_COMPLETE;
-		strcpy(bmsg.name, barrier->name);
+		strncpy(bmsg.name, barrier->name, MAX_BARRIER_NAME_LEN - 1);
 
 		log_printf(LOGSYS_LEVEL_DEBUG, "barrier: Sending COMPLETE for %s\n", barrier->name);
 		comms_send_message((char *) &bmsg, sizeof (bmsg),
@@ -160,7 +160,7 @@ static struct cl_barrier *alloc_barrier(char *name, int nodes)
 	}
 	memset(barrier, 0, sizeof (*barrier));
 
-	strcpy(barrier->name, name);
+	strncpy(barrier->name, name, MAX_BARRIER_NAME_LEN - 1);
 	barrier->flags = 0;
 	barrier->expected_nodes = nodes;
 	barrier->got_nodes = 0;
@@ -268,7 +268,7 @@ static int barrier_setattr_enabled(struct cl_barrier *barrier,
 		/* Send it to the rest of the cluster */
 		bmsg.cmd = CLUSTER_MSG_BARRIER;
 		bmsg.subcmd = BARRIER_WAIT;
-		strcpy(bmsg.name, barrier->name);
+		strncpy(bmsg.name, barrier->name, MAX_BARRIER_NAME_LEN - 1);
 
 		barrier->waitsent = 1;
 		barrier->phase = 1;
diff --git a/cman/daemon/cman-preconfig.c b/cman/daemon/cman-preconfig.c
index d632c17..baf94df 100644
--- a/cman/daemon/cman-preconfig.c
+++ b/cman/daemon/cman-preconfig.c
@@ -302,14 +302,14 @@ static int add_ifaddr(struct objdb_iface_ver0 *objdb, char *mcast, char *ifaddr,
 	/* Check the families match */
 	if (address_family(mcast, &mcast_addr, 0) !=
 	    address_family(ifaddr, &if_addr, mcast_addr.ss_family)) {
-		sprintf(error_reason, "Node address family does not match multicast address family");
+		snprintf(error_reason, sizeof(error_reason) - 1, "Node address family does not match multicast address family");
 		return -1;
 	}
 
 	/* Check it's not bound to localhost, sigh */
 	get_localhost(if_addr.ss_family, &localhost);
 	if (ipaddr_equal(&localhost, &if_addr)) {
-		sprintf(error_reason, "Node name resolves to localhost, please check /etc/hosts and assign this node a network IP address");
+		snprintf(error_reason, sizeof(error_reason) - 1, "Node name resolves to localhost, please check /etc/hosts and assign this node a network IP address");
 		return -1;
 	}
 
@@ -325,7 +325,7 @@ static int add_ifaddr(struct objdb_iface_ver0 *objdb, char *mcast, char *ifaddr,
 			objdb->object_key_create_typed(totem_object_handle, "transport",
 				tx_mech_to_str[transport], strlen(tx_mech_to_str[transport]) + 1, OBJDB_VALUETYPE_STRING);
 		} else {
-			sprintf(error_reason, "Transport should not be specified within <totem .../>, use <cman transport=\"...\" /> instead");
+			snprintf(error_reason, sizeof(error_reason) - 1, "Transport should not be specified within <totem .../>, use <cman transport=\"...\" /> instead");
 			return -1;
 		}
 	}
@@ -336,7 +336,7 @@ static int add_ifaddr(struct objdb_iface_ver0 *objdb, char *mcast, char *ifaddr,
 		struct sockaddr_in6 *in6= (struct sockaddr_in6 *)&if_addr;
 		void *addrptr;
 
-		sprintf(tmp, "%d", num_interfaces);
+		snprintf(tmp, sizeof(tmp) - 1, "%d", num_interfaces);
 		objdb->object_key_create_typed(interface_object_handle, "ringnumber",
 					       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
@@ -363,19 +363,19 @@ static int add_ifaddr(struct objdb_iface_ver0 *objdb, char *mcast, char *ifaddr,
 			break;
 		}
 
-		sprintf(tmp, "%d", port);
+		snprintf(tmp, sizeof(tmp) - 1, "%d", port);
 		objdb->object_key_create_typed(interface_object_handle, "mcastport",
 					       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
 		/* paranoia check. corosync already does it */
 		if ((ttl < 0) || (ttl > 255)) {
-			sprintf(error_reason, "TTL value (%u) out of range (0 - 255)", ttl);
+			snprintf(error_reason, sizeof(error_reason) - 1, "TTL value (%u) out of range (0 - 255)", ttl);
 			return -1;
 		}
 
 		/* add the key to the objdb only if value is not default */
 		if (ttl != 1) {
-			sprintf(tmp, "%d", ttl);
+			snprintf(tmp, sizeof(tmp) - 1, "%d", ttl);
 			objdb->object_key_create_typed(interface_object_handle, "ttl",
 						       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 		}
@@ -412,7 +412,7 @@ static char *default_mcast(char *node, int altiface)
 	   default a multicast address */
         ret = getaddrinfo(node, NULL, &ahints, &ainfo);
 	if (ret) {
-		sprintf(error_reason, "Can't determine address family of nodename %s\n", node);
+		snprintf(error_reason, sizeof(error_reason) - 1, "Can't determine address family of nodename %s\n", node);
 		write_cman_pipe("Can't determine address family of nodename");
 		return NULL;
 	}
@@ -448,13 +448,13 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 		return 0;
 
 	/* If nodename was from uname, try a domain-less version of it */
-	strcpy(nodename2, node);
+	strncpy(nodename2, node, sizeof(nodename2) - 1);
 	dot = strchr(nodename2, '.');
 	if (dot) {
 		*dot = '\0';
 
 		if (nodelist_byname(objdb, cluster_parent_handle, nodename2)) {
-			strcpy(node, nodename2);
+			strncpy(node, nodename2, MAX_CLUSTER_MEMBER_NAME_LEN - 1);
 			return 0;
 		}
 	}
@@ -466,12 +466,12 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 		int len;
 
 		if (objdb_get_string(objdb, nodes_handle, "name", &str)) {
-			sprintf(error_reason, "Cannot get node name");
+			snprintf(error_reason, sizeof(error_reason) - 1, "Cannot get node name");
 			nodes_handle = nodeslist_next(objdb, find_handle);
 			continue;
 		}
 
-		strcpy(nodename3, str);
+		strncpy(nodename3, str, sizeof(nodename3) - 1);
 		dot = strchr(nodename3, '.');
 		if (dot)
 			len = dot-nodename3;
@@ -480,7 +480,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 
 		if (strlen(nodename2) == len &&
 		    !strncmp(nodename2, nodename3, len)) {
-			strcpy(node, str);
+			strncpy(node, str, sizeof(nodename) - 1);
 			return 0;
 		}
 		nodes_handle = nodeslist_next(objdb, find_handle);
@@ -499,7 +499,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 		socklen_t salen = 0;
 
 		/* Restore this */
-		strcpy(nodename2, node);
+		strncpy(nodename2, node, sizeof(nodename2) - 1);
 		sa = ifa->ifa_addr;
 		if (!sa)
 			continue;
@@ -516,7 +516,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 		if (!error) {
 
 			if (nodelist_byname(objdb, cluster_parent_handle, nodename2)) {
-				strcpy(node, nodename2);
+				strncpy(node, nodename2, sizeof(nodename) - 1);
 				goto out;
 			}
 
@@ -526,7 +526,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 				*dot = '\0';
 
 				if (nodelist_byname(objdb, cluster_parent_handle, nodename2)) {
-					strcpy(node, nodename2);
+					strncpy(node, nodename2, sizeof(nodename) - 1);
 					goto out;
 				}
 			}
@@ -539,7 +539,7 @@ static int verify_nodename(struct objdb_iface_ver0 *objdb, char *node)
 			continue;
 
 		if (nodelist_byname(objdb, cluster_parent_handle, nodename2)) {
-			strcpy(node, nodename2);
+			strncpy(node, nodename2, sizeof(nodename) - 1);
 			goto out;
 		}
 	}
@@ -626,16 +626,16 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 		/* our nodename */
 		if (nodename_env != NULL) {
 			if (strlen(nodename_env) >= sizeof(nodename)) {
-				sprintf(error_reason, "Overridden node name %s is too long", nodename);
+				snprintf(error_reason, sizeof(error_reason) - 1, "Overridden node name %s is too long", nodename);
 				write_cman_pipe("Overridden node name is too long");
 				error = -1;
 				goto out;
 			}
 
-			strcpy(nodename, nodename_env);
+			strncpy(nodename, nodename_env, sizeof(nodename) - 1);
 
 			if (!(node_object_handle = nodelist_byname(objdb, cluster_parent_handle, nodename))) {
-				sprintf(error_reason, "Overridden node name %s is not in CCS", nodename);
+				snprintf(error_reason, sizeof(error_reason) - 1, "Overridden node name %s is not in CCS", nodename);
 				write_cman_pipe("Overridden node name is not in CCS");
 				error = -1;
 				goto out;
@@ -646,20 +646,20 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 
 			error = uname(&utsname);
 			if (error) {
-				sprintf(error_reason, "cannot get node name, uname failed");
+				snprintf(error_reason, sizeof(error_reason) - 1, "cannot get node name, uname failed");
 				write_cman_pipe("Can't determine local node name, uname failed");
 				error = -1;
 				goto out;
 			}
 
 			if (strlen(utsname.nodename) >= sizeof(nodename)) {
-				sprintf(error_reason, "node name from uname is too long");
+				snprintf(error_reason, sizeof(error_reason) - 1, "node name from uname is too long");
 				write_cman_pipe("local node name is too long");
 				error = -1;
 				goto out;
 			}
 
-			strcpy(nodename, utsname.nodename);
+			strncpy(nodename, utsname.nodename, sizeof(nodename) - 1);
 		}
 		if (verify_nodename(objdb, nodename)) {
 			write_cman_pipe("Cannot find node name in cluster.conf");
@@ -671,7 +671,7 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 	/* Add <cman> bits to pass down to the main module*/
 	if ( (node_object_handle = nodelist_byname(objdb, cluster_parent_handle, nodename))) {
 		if (objdb_get_string(objdb, node_object_handle, "nodeid", &nodeid_str)) {
-			sprintf(error_reason, "This node has no nodeid in cluster.conf");
+			snprintf(error_reason, sizeof(error_reason) - 1, "This node has no nodeid in cluster.conf");
 			write_cman_pipe("This node has no nodeid in cluster.conf");
 			return -1;
 		}
@@ -711,7 +711,7 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 	objdb->object_find_destroy(find_handle);
 
 	if (!nodeid_str) {
-		sprintf(error_reason, "This node has no nodeid in cluster.conf");
+		snprintf(error_reason, sizeof(error_reason) - 1, "This node has no nodeid in cluster.conf");
 		write_cman_pipe("This node has no nodeid in cluster.conf");
 		return -1;
 	}
@@ -746,7 +746,7 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 			if (transport != TX_MECH_UDPB) {
 				transport = TX_MECH_UDPU;
 			} else {
-				sprintf(error_reason, "Transport and broadcast option are mutually exclusive");
+				snprintf(error_reason, sizeof(error_reason) - 1, "Transport and broadcast option are mutually exclusive");
 				write_cman_pipe("Transport and broadcast option are mutually exclusive");
 				return -1;
 			}
@@ -754,12 +754,12 @@ static int get_nodename(struct objdb_iface_ver0 *objdb)
 			if (transport != TX_MECH_UDPB) {
 				transport = TX_MECH_RDMA;
 			} else {
-				sprintf(error_reason, "Transport and broadcast option are mutually exclusive");
+				snprintf(error_reason, sizeof(error_reason) - 1, "Transport and broadcast option are mutually exclusive");
 				write_cman_pipe("Transport and broadcast option are mutually exclusive");
 				return -1;
 			}
 		} else {
-			sprintf(error_reason, "Transport option value can be one of udp, udpb, udpu, rdma");
+			snprintf(error_reason, sizeof(error_reason) - 1, "Transport option value can be one of udp, udpb, udpu, rdma");
 			write_cman_pipe("Transport option value can be one of udp, udpb, udpu, rdma");
 			return -1;
 		}
@@ -931,7 +931,7 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 		objdb->object_key_create_typed(object_handle, "version",
 					       "2", 2, OBJDB_VALUETYPE_STRING);
 
-		sprintf(tmp, "%d", nodeid);
+		snprintf(tmp, sizeof(tmp) - 1, "%d", nodeid);
 		objdb->object_key_create_typed(object_handle, "nodeid",
 					       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
@@ -941,7 +941,7 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 		/* Set the token timeout is 10 seconds, but don't overrride anything that
 		   might be in cluster.conf */
 		if (objdb_get_string(objdb, object_handle, "token", &value)) {
-			snprintf(tmp, sizeof(tmp), "%d", DEFAULT_TOKEN_TIMEOUT);
+			snprintf(tmp, sizeof(tmp) - 1, "%d", DEFAULT_TOKEN_TIMEOUT);
 			objdb->object_key_create_typed(object_handle, "token",
 						       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 		}
@@ -976,7 +976,7 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 		if (objdb_get_string(objdb, object_handle, "consensus", &value)) {
 			unsigned int token=0;
 			unsigned int consensus;
-			char calc_consensus[32];
+			char calc_consensus[64];
 
 			objdb_get_int(objdb, object_handle, "token", &token, DEFAULT_TOKEN_TIMEOUT);
 
@@ -990,7 +990,7 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 					consensus = 2000;
 			}
 
-			snprintf(calc_consensus, sizeof(calc_consensus), "%d", consensus);
+			snprintf(calc_consensus, sizeof(calc_consensus) - 1, "%d", consensus);
 			objdb->object_key_create_typed(object_handle, "consensus",
 						       calc_consensus, strlen(calc_consensus)+1, OBJDB_VALUETYPE_STRING);
 		}
@@ -1015,7 +1015,7 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 		}
 
 		if (objdb_get_string(objdb, object_handle, "secauth", &value)) {
-			sprintf(tmp, "%d", 1);
+			snprintf(tmp, sizeof(tmp) - 1, "%d", 1);
 			objdb->object_key_create_typed(object_handle, "secauth",
 						       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 		}
@@ -1067,13 +1067,13 @@ static void add_cman_overrides(struct objdb_iface_ver0 *objdb)
 	{
 		char str[255];
 
-		sprintf(str, "%d", cluster_id);
+		snprintf(str, sizeof(str) - 1, "%d", cluster_id);
 
 		objdb->object_key_create_typed(object_handle, "cluster_id",
 					       str, strlen(str) + 1, OBJDB_VALUETYPE_STRING);
 
 		if (two_node) {
-			sprintf(str, "%d", 1);
+			snprintf(str, sizeof(str) - 1, "%d", 1);
 			objdb->object_key_create_typed(object_handle, "two_node",
 						       str, strlen(str) + 1, OBJDB_VALUETYPE_STRING);
 		}
@@ -1118,7 +1118,7 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
 	/* Enforce key */
 	key_filename = strdup(NOCCS_KEY_FILENAME);
 	if (!key_filename) {
-		sprintf(error_reason, "cannot allocate memory for key file name");
+		snprintf(error_reason, sizeof(error_reason) - 1, "cannot allocate memory for key file name");
 		write_cman_pipe("cannot allocate memory for key file name");
 		return -1;
 	}
@@ -1127,7 +1127,7 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
 		cluster_name = strdup(DEFAULT_CLUSTER_NAME);
 
 	if (!cluster_name) {
-		sprintf(error_reason, "cannot allocate memory for cluster_name");
+		snprintf(error_reason, sizeof(error_reason) - 1, "cannot allocate memory for cluster_name");
 		write_cman_pipe("cannot allocate memory for cluster_name");
 		return -1;
 	}
@@ -1138,7 +1138,7 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
 	    else
 	        cluster_id = generate_cluster_id(cluster_name);
 
-	    sprintf(error_reason, "Generated cluster id for '%s' is %d\n", cluster_name, cluster_id);
+	    snprintf(error_reason, sizeof(error_reason) - 1, "Generated cluster id for '%s' is %d\n", cluster_name, cluster_id);
 	}
 
 	if (!nodename_env) {
@@ -1147,14 +1147,14 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
 
 		error = uname(&utsname);
 		if (error) {
-			sprintf(error_reason, "cannot get node name, uname failed");
+			snprintf(error_reason, sizeof(error_reason) - 1, "cannot get node name, uname failed");
 			write_cman_pipe("Can't determine local node name");
 			return -1;
 		}
 
 		nodename_env = (char *)&utsname.nodename;
 	}
-	strcpy(nodename, nodename_env);
+	strncpy(nodename, nodename_env, sizeof(nodename) - 1);
 	num_nodenames = 1;
 
 	if (!mcast_name) {
@@ -1179,7 +1179,7 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
 		memset(&ahints, 0, sizeof(ahints));
 		ret = getaddrinfo(nodename, NULL, &ahints, &ainfo);
 		if (ret) {
-			sprintf(error_reason, "Can't determine address family of nodename %s\n", nodename);
+			snprintf(error_reason, sizeof(error_reason) - 1, "Can't determine address family of nodename %s\n", nodename);
 			write_cman_pipe("Can't determine address family of nodename");
 			return -1;
 		}
@@ -1203,11 +1203,11 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
 	objdb->object_key_create_typed(object_handle, "name",
 				       nodename, strlen(nodename)+1, OBJDB_VALUETYPE_STRING);
 
-	sprintf(tmp, "%d", votes);
+	snprintf(tmp, sizeof(tmp) - 1, "%d", votes);
 	objdb->object_key_create_typed(object_handle, "votes",
 				       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
-	sprintf(tmp, "%d", nodeid);
+	snprintf(tmp, sizeof(tmp) - 1, "%d", nodeid);
 	objdb->object_key_create_typed(object_handle, "nodeid",
 				       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
@@ -1222,11 +1222,11 @@ static int set_noccs_defaults(struct objdb_iface_ver0 *objdb)
                 objdb->object_create(cluster_parent_handle, &object_handle,
                                             "cman", strlen("cman"));
         }
-	sprintf(tmp, "%d", cluster_id);
+	snprintf(tmp, sizeof(tmp) - 1, "%d", cluster_id);
 	objdb->object_key_create_typed(object_handle, "cluster_id",
 				       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
-	sprintf(tmp, "%d", expected_votes);
+	snprintf(tmp, sizeof(tmp) - 1, "%d", expected_votes);
 	objdb->object_key_create_typed(object_handle, "expected_votes",
 				       tmp, strlen(tmp)+1, OBJDB_VALUETYPE_STRING);
 
@@ -1269,7 +1269,7 @@ static int copy_config_tree(struct objdb_iface_ver0 *objdb, hdb_handle_t source_
 	/* Create sub-objects */
 	res = objdb->object_find_create(source_object, NULL, 0, &find_handle);
 	if (res) {
-		sprintf(error_reason, "error resetting object iterator for object %ud: %d\n", (unsigned int)source_object, res);
+		snprintf(error_reason, sizeof(error_reason) - 1, "error resetting object iterator for object %ud: %d\n", (unsigned int)source_object, res);
 		return -1;
 	}
 
@@ -1310,13 +1310,13 @@ static int get_cman_globals(struct objdb_iface_ver0 *objdb)
 
 	objdb_get_string(objdb, cluster_parent_handle, "name", &cluster_name);
 	if (!cluster_name) {
-		sprintf(error_reason, "Unable to determine cluster name.\n");
+		snprintf(error_reason, sizeof(error_reason) - 1, "Unable to determine cluster name.\n");
 		write_cman_pipe("Unable to determine cluster name.\n");
 		return -1;
 	}
 
 	if (strlen(cluster_name) > 15) {
-		sprintf(error_reason, "%s\n", "Invalid cluster name. It must be 15 characters or fewer\n");
+		snprintf(error_reason, sizeof(error_reason) - 1, "Invalid cluster name. It must be 15 characters or fewer\n\n");
 		write_cman_pipe("Invalid cluster name. It must be 15 characters or fewer\n");
 		return -1;
 	}
@@ -1345,7 +1345,7 @@ static int get_cman_globals(struct objdb_iface_ver0 *objdb)
 		    else
 		        cluster_id = generate_cluster_id(cluster_name);
 
-		    sprintf(error_reason, "Generated cluster id for '%s' is %d\n", cluster_name, cluster_id);
+		    snprintf(error_reason, sizeof(error_reason) - 1, "Generated cluster id for '%s' is %d\n", cluster_name, cluster_id);
 		}
 	}
 	objdb->object_find_destroy(find_handle);
@@ -1363,7 +1363,7 @@ static int cmanpre_reloadconfig(struct objdb_iface_ver0 *objdb, int flush, const
 
 	/* don't reload if we've been told to run configless */
 	if (getenv("CMAN_NOCONFIG")) {
-		sprintf(error_reason, "Config not updated because we were run with cman_tool -X");
+		snprintf(error_reason, sizeof(error_reason) - 1, "Config not updated because we were run with cman_tool -X");
 		ret = 0;
 		goto err;
 	}
@@ -1372,12 +1372,12 @@ static int cmanpre_reloadconfig(struct objdb_iface_ver0 *objdb, int flush, const
 	objdb->object_find_create(OBJECT_PARENT_HANDLE, "cluster", strlen("cluster"), &find_handle);
 	objdb->object_find_next(find_handle, &cluster_parent_handle);
 	if (!cluster_parent_handle) {
-		sprintf (error_reason, "%s", "Cannot find old /cluster/ key in configuration\n");
+		snprintf (error_reason, sizeof(error_reason) - 1, "Cannot find old /cluster/ key in configuration\n");
 		goto err;
 	}
 	objdb->object_find_next(find_handle, &cluster_parent_handle_new);
 	if (!cluster_parent_handle_new) {
-		sprintf (error_reason, "%s", "Cannot find new /cluster/ key in configuration\n");
+		snprintf (error_reason, sizeof(error_reason) - 1, "Cannot find new /cluster/ key in configuration\n");
 		goto err;
 	}
 	objdb->object_find_destroy(find_handle);
@@ -1387,7 +1387,7 @@ static int cmanpre_reloadconfig(struct objdb_iface_ver0 *objdb, int flush, const
 			config_version = atoi(config_value);
 		} else {
 			/* it should never ever happen.. */
-			sprintf (error_reason, "%s", "Cannot find old /cluster/config_version key in configuration\n");
+			snprintf (error_reason, sizeof(error_reason) - 1, "Cannot find old /cluster/config_version key in configuration\n");
 			goto err;
 		}
 	}
@@ -1399,14 +1399,14 @@ static int cmanpre_reloadconfig(struct objdb_iface_ver0 *objdb, int flush, const
 			config_version_new = atoi(config_value);
 		} else {
 			objdb->object_destroy(cluster_parent_handle_new);
-			sprintf (error_reason, "%s", "Cannot find new /cluster/config_version key in configuration\n");
+			snprintf (error_reason, sizeof(error_reason) - 1,"Cannot find new /cluster/config_version key in configuration\n");
 			goto err;
 		}
 	}
 
 	if (config_version_new <= config_version) {
 		objdb->object_destroy(cluster_parent_handle_new);
-		sprintf (error_reason, "%s", "New configuration version has to be newer than current running configuration\n");
+		snprintf (error_reason, sizeof(error_reason) - 1, "New configuration version has to be newer than current running configuration\n");
 		goto err;
 	}
 
@@ -1600,11 +1600,11 @@ static int cmanpre_readconfig(struct objdb_iface_ver0 *objdb, const char **error
 
 
 	if (!ret) {
-		sprintf (error_reason, "%s", "Successfully parsed cman config\n");
+		snprintf(error_reason, sizeof(error_reason) - 1, "Successfully parsed cman config\n");
 	}
 	else {
 		if (error_reason[0] == '\0')
-			sprintf (error_reason, "%s", "Error parsing cman config\n");
+			snprintf(error_reason, sizeof(error_reason) - 1, "Error parsing cman config\n");
 	}
         *error_string = error_reason;
 
diff --git a/cman/daemon/cmanconfig.c b/cman/daemon/cmanconfig.c
index d3009d0..7eb52e4 100644
--- a/cman/daemon/cmanconfig.c
+++ b/cman/daemon/cmanconfig.c
@@ -175,7 +175,7 @@ static int get_cman_join_info(struct corosync_api_v1 *corosync)
 		goto out;
 	}
 
-	strcpy(cluster_name, cname);
+	strncpy(cluster_name, cname, sizeof(cluster_name) - 1);
 
 	expected_votes = 0;
 	if (getenv("CMAN_EXPECTEDVOTES")) {
diff --git a/cman/daemon/commands.c b/cman/daemon/commands.c
index 6b61332..f397234 100644
--- a/cman/daemon/commands.c
+++ b/cman/daemon/commands.c
@@ -396,7 +396,7 @@ static void copy_to_usernode(struct cluster_node *node,
 	struct totem_ip_address node_ifs[INTERFACE_MAX];
 	/* totempg_ifaces_get always copies INTERFACE_MAX addresses */
 
-	strcpy(unode->name, node->name);
+	strncpy(unode->name, node->name, MAX_CLUSTER_MEMBER_NAME_LEN - 1);
 	unode->jointime = node->join_time;
 	unode->size = sizeof(struct cl_cluster_node);
 	unode->votes = node->votes;
@@ -421,7 +421,7 @@ int cman_set_nodename(char *name)
 	if (ais_running)
 		return -EALREADY;
 
-	strncpy(nodename, name, MAX_CLUSTER_MEMBER_NAME_LEN);
+	strncpy(nodename, name, MAX_CLUSTER_MEMBER_NAME_LEN - 1);
 	return 0;
 }
 
@@ -1401,7 +1401,7 @@ static int do_cmd_update_fence_info(char *cmdbuf)
 	fence_msg->nodeid = f->nodeid;
 	fence_msg->timesec = f->fence_time;
 	fence_msg->fenced = 1;
-	strcpy(fence_msg->agent, f->fence_agent);
+	strncpy(fence_msg->agent, f->fence_agent, MAX_FENCE_AGENT_NAME_LEN - 1);
 	comms_send_message(msg, sizeof(msg), 0,0, 0, 0);
 
 	log_printf(LOGSYS_LEVEL_DEBUG, "memb: node %d fenced by %s\n", f->nodeid, f->fence_agent);
@@ -1428,7 +1428,7 @@ static int do_cmd_get_fence_info(char *cmdbuf, char **retbuf, int retsize, int *
 	f->flags = node->flags&NODE_FLAGS_FENCED;
 
 	if (node->fence_agent)
-		strcpy(f->fence_agent, node->fence_agent);
+		strncpy(f->fence_agent, node->fence_agent, MAX_FENCE_AGENT_NAME_LEN - 1);
 	else
 		f->fence_agent[0] = '\0';
 	*retlen = sizeof(struct cl_fence_info);
@@ -1867,10 +1867,10 @@ void send_transition_msg(int last_memb_count, int first_trans)
 	msg->flags = us->flags;
 	msg->fence_time = us->fence_time;
 	msg->join_time = join_time;
-	strcpy(msg->clustername, cluster_name);
+	memcpy(msg->clustername, cluster_name, MAX_CLUSTER_NAME_LEN);
 	if (us->fence_agent)
 	{
-		strcpy(msg->fence_agent, us->fence_agent);
+		strncpy(msg->fence_agent, us->fence_agent, MAX_FENCE_AGENT_NAME_LEN - 1);
 		len += strlen(us->fence_agent)+1;
 	}
 	else
@@ -2128,7 +2128,7 @@ static void do_process_transition(int nodeid, char *data)
 		fence_msg->nodeid = nodeid;
 		fence_msg->timesec = node->fence_time;
 		fence_msg->fenced = 0;
-		strcpy(fence_msg->agent, node->fence_agent);
+		strncpy(fence_msg->agent, node->fence_agent, MAX_FENCE_AGENT_NAME_LEN - 1);
 		comms_send_message(fencemsg, sizeof(fencemsg), 0,0, nodeid, 0);
 	}
 }
@@ -2307,7 +2307,7 @@ void add_ais_node(int nodeid, uint64_t incar, int total_members)
 		log_printf(LOG_ERR, "Got node from AIS id %d with no config entry\n", nodeid);
 
 		/* Emergency nodename */
-		sprintf(tempname, "Node%d", nodeid);
+		snprintf(tempname, sizeof(tempname) - 1, "Node%d", nodeid);
 		node = add_new_node(tempname, nodeid, 1, total_members, NODESTATE_DEAD);
 		if (!node) {
 			log_printf(LOG_ERR, "Unable to add newnode!\n");
@@ -2432,7 +2432,7 @@ static const char *killmsg_reason(int reason)
 		return "we rejoined the cluster without a full restart";
 
 	default:
-		sprintf(msg, "we got kill message number %d", reason);
+		snprintf(msg, sizeof(msg) - 1, "we got kill message number %d", reason);
 		return msg;
 	}
 }
-- 
1.7.4.4

[Cluster-devel] [PATCH 23/41] notifyd: check for pid errors and report them

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 133706f... 7b625c9... M	cman/notifyd/main.c
 cman/notifyd/main.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/cman/notifyd/main.c b/cman/notifyd/main.c
index 133706f..7b625c9 100644
--- a/cman/notifyd/main.c
+++ b/cman/notifyd/main.c
@@ -207,7 +207,7 @@ static void dispatch_notification(const char *str, int *quorum)
 	int envptr = 0;
 	int argvptr = 0;
 	char scratch[PATH_MAX];
-	pid_t notify_pid;
+	pid_t notify_pid, pid;
 	int pidstatus;
 	int err = 0;
 
@@ -248,7 +248,10 @@ static void dispatch_notification(const char *str, int *quorum)
 			break;
 
 		default: /* parent */
-			waitpid(notify_pid, &pidstatus, 0);
+			pid = waitpid(notify_pid, &pidstatus, 0);
+			if (pid < 0)
+				logt_print(LOG_ERR, "Error waiting for " SBINDIR "/cman_notify execution\n");
+
 			break;
 	}
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 24/41] notifyd: fix virtually impossible buffer overflows

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 7b625c9... 3091d2f... M	cman/notifyd/main.c
 cman/notifyd/main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cman/notifyd/main.c b/cman/notifyd/main.c
index 7b625c9..3091d2f 100644
--- a/cman/notifyd/main.c
+++ b/cman/notifyd/main.c
@@ -136,7 +136,7 @@ static void lockfile(void)
 		exit(EXIT_FAILURE);
 	}
 
-	sprintf(buf, "%d\n", getpid());
+	snprintf(buf, sizeof(buf) - 1, "%d\n", getpid());
 
 	error = write(fd, buf, strlen(buf));
 	if (error <= 0) {
@@ -182,7 +182,7 @@ static void init_logging(int reconf)
 	int logfile_priority = SYSLOGLEVEL;
 
 	memset(logfile, 0, PATH_MAX);
-	sprintf(logfile, LOGDIR "/cmannotifyd.log");
+	snprintf(logfile, sizeof(logfile) - 1, LOGDIR "/cmannotifyd.log");
 
 	ccs_handle = ccs_connect();
 	if (ccs_handle > 0) {
-- 
1.7.4.4

[Cluster-devel] [PATCH 25/41] qdiskd: use correct sizeof for memb_mask_t

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 d613d84... 01aa199... M	cman/qdisk/main.c
 cman/qdisk/main.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index d613d84..01aa199 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -609,12 +609,12 @@ check_cman(qd_ctx *ctx, memb_mask_t mask, memb_mask_t master_mask)
 			   &retnodes, nodes) <0 )
 		return;
 
-	memset(master_mask, 0, sizeof(master_mask));
+	memset(master_mask, 0, sizeof(memb_mask_t));
 	for (x = 0; x < retnodes; x++) {
-		if (is_bit_set(mask, nodes[x].cn_nodeid-1, sizeof(mask)) &&
+		if (is_bit_set(mask, nodes[x].cn_nodeid-1, sizeof(memb_mask_t)) &&
 		    nodes[x].cn_member) {
 			set_bit(master_mask, nodes[x].cn_nodeid-1,
-				sizeof(master_mask));
+				sizeof(memb_mask_t));
 		} else {
 			/* Not in CMAN output = not allowed */
 			clear_bit(master_mask, (nodes[x].cn_nodeid-1),
-- 
1.7.4.4

[Cluster-devel] [PATCH 26/41] qdiskd: report errors on cman_dispatch failures

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 01aa199... 6e9abea... M	cman/qdisk/main.c
 cman/qdisk/main.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index 01aa199..6e9abea 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -551,7 +551,9 @@ quorum_init(qd_ctx *ctx, node_info_t *ni, int max, struct h_data *h, int maxh)
 
 		tv.tv_sec = ctx->qc_interval;
 		tv.tv_usec = 0;
-		cman_wait(ctx->qc_cman_user, &tv);
+		if (cman_wait(ctx->qc_cman_user, &tv) < 0)
+			logt_print(LOG_ERR, "cman_dispatch: %s\n",
+				   strerror(errno));
 	}
 
 	if (!_running) {
@@ -1246,10 +1248,12 @@ quorum_loop(qd_ctx *ctx, node_info_t *ni, int max)
 		} else {
 			error_cycles = 0;
 		}
-		
+
 		/* Could hit a watchdog timer here if we wanted to */
 		if (_running) {
-			cman_wait(ctx->qc_cman_user, &sleeptime);
+			if (cman_wait(ctx->qc_cman_user, &sleeptime) < 0)
+				logt_print(LOG_ERR, "cman_dispatch: %s\n",
+					   strerror(errno));
 		}
 	}
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 27/41] qdiskd: check for sscanf return codes

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 20a77f2... 6bbf311... M	cman/qdisk/scandisk.c
 cman/qdisk/scandisk.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/cman/qdisk/scandisk.c b/cman/qdisk/scandisk.c
index 20a77f2..6bbf311 100644
--- a/cman/qdisk/scandisk.c
+++ b/cman/qdisk/scandisk.c
@@ -260,8 +260,10 @@ static int scanprocpart(struct devlisthead *devlisthead)
 
 		if (strlen(line) > 128 + (22))
 			continue;
-		sscanf(line, "%4d %4d %10llu %s",
-		       &major, &minor, &blkcnt, device);
+
+		if (sscanf(line, "%4d %4d %10llu %s",
+		       &major, &minor, &blkcnt, device) < 4)
+			continue;
 
 		/* careful here.. if there is no device, we are scanning the
 		 * first two lines that are not useful to us
@@ -327,8 +329,9 @@ static int scanmdstat(struct devlisthead *devlisthead)
 		if (!(line[0] == 'm' && line[1] == 'd'))
 			continue;
 
-		sscanf(line, "%s %s %s %s %s",
-		       device, separator, status, personality, firstdevice);
+		if (sscanf(line, "%s %s %s %s %s",
+		       device, separator, status, personality, firstdevice) < 5)
+			continue;
 
 		/* scan only raids that are active */
 		if (strcmp(status, "active"))
@@ -408,7 +411,8 @@ static int scanmapper(struct devlisthead *devlisthead)
 		if (!start)
 			continue;
 
-		sscanf(line, "%s %s", major, device);
+		if (sscanf(line, "%s %s", major, device) < 2)
+			continue;
 
 		if (!strncmp(device, "device-mapper", 13)) {
 			maj = atoi(major);
-- 
1.7.4.4

[Cluster-devel] [PATCH 28/41] qdisk: fix scandisk eval check

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 6bbf311... e962f74... M	cman/qdisk/scandisk.c
 cman/qdisk/scandisk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cman/qdisk/scandisk.c b/cman/qdisk/scandisk.c
index 6bbf311..e962f74 100644
--- a/cman/qdisk/scandisk.c
+++ b/cman/qdisk/scandisk.c
@@ -224,7 +224,7 @@ static int lsdev(struct devlisthead *devlisthead, const char *path)
 
 				if (dev_is_block(&sb, newpath))
 					if (!add_lsdev_block
-					    (devlisthead, &sb, newpath) < 0)
+					    (devlisthead, &sb, newpath))
 						return -2;
 			}
 		}
-- 
1.7.4.4

[Cluster-devel] [PATCH 29/41] qdiskd: add strlen check to avoid memory corruption

[Fabio M. Di Nitto]

and fix a few impossible buffer overflows

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 d5926f9... 929b152... M	cman/qdisk/daemon_init.c
 cman/qdisk/daemon_init.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/cman/qdisk/daemon_init.c b/cman/qdisk/daemon_init.c
index d5926f9..929b152 100644
--- a/cman/qdisk/daemon_init.c
+++ b/cman/qdisk/daemon_init.c
@@ -48,11 +48,12 @@ check_pid_valid(pid_t pid, char *prog)
 	char dirpath[PATH_MAX];
 	char proc_cmdline[64];	/* yank this from kernel somewhere */
 	char *s = NULL;
+	size_t proc_cmdline_len = 0;
 
 	memset(filename, 0, PATH_MAX);
 	memset(dirpath, 0, PATH_MAX);
 
-	snprintf(dirpath, sizeof (dirpath), "/proc/%d", pid);
+	snprintf(dirpath, sizeof (dirpath) - 1, "/proc/%d", pid);
 	if ((dir = opendir(dirpath)) == NULL) {
 		closedir(dir);
 		return 0;	/* Pid has gone away. */
@@ -63,7 +64,7 @@ check_pid_valid(pid_t pid, char *prog)
 	 * proc-pid directory exists.  Now check to see if this
 	 * PID corresponds to the daemon we want to start.
 	 */
-	snprintf(filename, sizeof (filename), "/proc/%d/cmdline", pid);
+	snprintf(filename, sizeof (filename) - 1, "/proc/%d/cmdline", pid);
 	fp = fopen(filename, "r");
 	if (fp == NULL) {
 		perror("check_pid_valid");
@@ -83,9 +84,12 @@ check_pid_valid(pid_t pid, char *prog)
 	}
 	fclose(fp);
 
-	s = &(proc_cmdline[strlen(proc_cmdline)]);
-	if (*s == '\n')
-		*s = 0;
+	proc_cmdline_len = strlen(proc_cmdline);
+	if (proc_cmdline_len) {
+		s = &(proc_cmdline[proc_cmdline_len]);
+		if (*s == '\n')
+			*s = 0;
+	}
 
 	/*
 	 * Check to see if this is the same executable.
-- 
1.7.4.4

[Cluster-devel] [PATCH 30/41] qdiskd: warn users when we cannot write eviction notice to disk

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 6e9abea... c4db713... M	cman/qdisk/main.c
 cman/qdisk/main.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index 6e9abea..c4db713 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -300,8 +300,10 @@ check_transitions(qd_ctx *ctx, node_info_t *ni, int max, memb_mask_t mask)
 				logt_print(LOG_NOTICE,
 				       "Writing eviction notice for node %d\n",
 				       ni[x].ni_status.ps_nodeid);
-				qd_write_status(ctx, ni[x].ni_status.ps_nodeid,
-						S_EVICT, NULL, NULL, NULL);
+				if (qd_write_status(ctx, ni[x].ni_status.ps_nodeid,
+						S_EVICT, NULL, NULL, NULL) != 0)
+					logt_print(LOG_CRIT, "Unable to write eviction notice for node %d!\n",
+						   ni[x].ni_status.ps_nodeid);
 				if (ctx->qc_flags & RF_ALLOW_KILL) {
 					logt_print(LOG_DEBUG, "Telling CMAN to "
 						"kill the node\n");
@@ -335,8 +337,10 @@ check_transitions(qd_ctx *ctx, node_info_t *ni, int max, memb_mask_t mask)
 			logt_print(LOG_ALERT,
  			       "Writing eviction notice (again) for node %d\n",
 			       ni[x].ni_status.ps_nodeid);
-			qd_write_status(ctx, ni[x].ni_status.ps_nodeid,
-					S_EVICT, NULL, NULL, NULL);
+			if (qd_write_status(ctx, ni[x].ni_status.ps_nodeid,
+					S_EVICT, NULL, NULL, NULL) != 0)
+				logt_print(LOG_CRIT, "Unable to write eviction notice for node %d!\n",
+					   ni[x].ni_status.ps_nodeid);
 			ni[x].ni_status.ps_state = S_EVICT;
 
 			/* XXX Need to fence it again */
@@ -1850,7 +1854,7 @@ get_config_data(qd_ctx *ctx, struct h_data *h, int maxh, int *cfh)
 		ctx->qc_sched_prio = 1;
 		ctx->qc_max_error_cycles = 0;
 	}
-	
+
 	if (get_dynamic_config_data(ctx, ccsfd) < 0) {
 		goto out;
 	}
-- 
1.7.4.4

[Cluster-devel] [PATCH 31/41] qdiskd: don�t deference null variable

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 929b152... 3cbac37... M	cman/qdisk/daemon_init.c
 cman/qdisk/daemon_init.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cman/qdisk/daemon_init.c b/cman/qdisk/daemon_init.c
index 929b152..3cbac37 100644
--- a/cman/qdisk/daemon_init.c
+++ b/cman/qdisk/daemon_init.c
@@ -54,10 +54,9 @@ check_pid_valid(pid_t pid, char *prog)
 	memset(dirpath, 0, PATH_MAX);
 
 	snprintf(dirpath, sizeof (dirpath) - 1, "/proc/%d", pid);
-	if ((dir = opendir(dirpath)) == NULL) {
-		closedir(dir);
-		return 0;	/* Pid has gone away. */
-	}
+	dir = opendir(dirpath);
+	if (!dir)
+		return 0; /* Pid has gone away. */
 	closedir(dir);
 
 	/*
-- 
1.7.4.4

[Cluster-devel] [PATCH 32/41] qdiskd: clean code around case

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 c4db713... 51d9a39... M	cman/qdisk/main.c
 cman/qdisk/main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index c4db713..51d9a39 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -146,9 +146,11 @@ check_self(qd_ctx *ctx, status_block_t *sb)
 	case S_EVICT:
 		/* Someone told us to die. */
 		reboot(RB_AUTOBOOT);
+		break;
 	default:
 		logt_print(LOG_EMERG, "Unhandled state: %d\n", sb->ps_state);
 		raise(SIGSTOP);
+		break;
 	}
 }
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 32/41] qdiskd: clean code around case

[Lon Hohberger]

On 11/23/2011 05:15 AM, Fabio M. Di Nitto wrote:
> Spotted by Coverity Scan
>
> Signed-off-by: Fabio M. Di Nitto<fdinitto@redhat.com>
> ---
> :100644 100644 c4db713... 51d9a39... M	cman/qdisk/main.c
>   cman/qdisk/main.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
> index c4db713..51d9a39 100644
> --- a/cman/qdisk/main.c
> +++ b/cman/qdisk/main.c
> @@ -146,9 +146,11 @@ check_self(qd_ctx *ctx, status_block_t *sb)
>   	case S_EVICT:
>   		/* Someone told us to die. */
>   		reboot(RB_AUTOBOOT);
> +		break;

This fallthrough to 'default' was deliberate.

>   	default:
>   		logt_print(LOG_EMERG, "Unhandled state: %d\n", sb->ps_state);
>   		raise(SIGSTOP);
> +		break;
>   	}
>   }
>

[Cluster-devel] [PATCH 32/41] qdiskd: clean code around case

[Fabio M. Di Nitto]

On 11/29/2011 06:59 PM, Lon Hohberger wrote:
> On 11/23/2011 05:15 AM, Fabio M. Di Nitto wrote:
>> Spotted by Coverity Scan
>>
>> Signed-off-by: Fabio M. Di Nitto<fdinitto@redhat.com>
>> ---
>> :100644 100644 c4db713... 51d9a39... M    cman/qdisk/main.c
>>   cman/qdisk/main.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
>> index c4db713..51d9a39 100644
>> --- a/cman/qdisk/main.c
>> +++ b/cman/qdisk/main.c
>> @@ -146,9 +146,11 @@ check_self(qd_ctx *ctx, status_block_t *sb)
>>       case S_EVICT:
>>           /* Someone told us to die. */
>>           reboot(RB_AUTOBOOT);
>> +        break;
> 
> This fallthrough to 'default' was deliberate.

hmm ok, but isn't reboot going to make it useless either way?

Fabio

[Cluster-devel] [PATCH 32/41] qdiskd: clean code around case

[Lon Hohberger]

On 11/29/2011 01:24 PM, Fabio M. Di Nitto wrote:
> On 11/29/2011 06:59 PM, Lon Hohberger wrote:
>> On 11/23/2011 05:15 AM, Fabio M. Di Nitto wrote:
>>> Spotted by Coverity Scan
>>>
>>> Signed-off-by: Fabio M. Di Nitto<fdinitto@redhat.com>
>>> ---
>>> :100644 100644 c4db713... 51d9a39... M    cman/qdisk/main.c
>>>    cman/qdisk/main.c |    2 ++
>>>    1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
>>> index c4db713..51d9a39 100644
>>> --- a/cman/qdisk/main.c
>>> +++ b/cman/qdisk/main.c
>>> @@ -146,9 +146,11 @@ check_self(qd_ctx *ctx, status_block_t *sb)
>>>        case S_EVICT:
>>>            /* Someone told us to die. */
>>>            reboot(RB_AUTOBOOT);
>>> +        break;
>>
>> This fallthrough to 'default' was deliberate.
>
> hmm ok, but isn't reboot going to make it useless either way?
>

Yes, presuming reboot works. ;)

(There are cases where it doesn't; there was a bugzilla open about this 
when using KVM guests recently ...)

SIGSTOP
will prevent qdiskd from continuing and casting votes in this case.

-- Lon

[Cluster-devel] [PATCH 33/41] qdiskd: don't try the impossible if we can't open /dev/null

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 3cbac37... c260cb8... M	cman/qdisk/daemon_init.c
:100644 100644 51d9a39... d21c209... M	cman/qdisk/main.c
 cman/qdisk/daemon_init.c |    2 +-
 cman/qdisk/main.c        |    5 +++++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/cman/qdisk/daemon_init.c b/cman/qdisk/daemon_init.c
index 3cbac37..c260cb8 100644
--- a/cman/qdisk/daemon_init.c
+++ b/cman/qdisk/daemon_init.c
@@ -93,7 +93,7 @@ check_pid_valid(pid_t pid, char *prog)
 	/*
 	 * Check to see if this is the same executable.
 	 */
-	if ((s = strstr(proc_cmdline, prog)) == NULL) {
+	if (!strstr(proc_cmdline, prog)) {
 		return 0;
 	} else {
 		return 1;
diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index 51d9a39..d21c209 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -1995,6 +1995,11 @@ main(int argc, char **argv)
 		case 'Q':
 			/* Make qdisk very quiet */
 			nfd = open("/dev/null", O_RDWR);
+			if (nfd < 0) {
+				fprintf(stderr, "Could not open /dev/null!\n");
+				ret = -1;
+				goto out;
+			}
 			close(0);
 			close(1);
 			close(2);
-- 
1.7.4.4

[Cluster-devel] [PATCH 34/41] qdiskd: avoid a potential crash in case config state is invalid

[Fabio M. Di Nitto]

Spotted by Coverify Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 d21c209... 079af01... M	cman/qdisk/main.c
 cman/qdisk/main.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index d21c209..079af01 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -73,6 +73,7 @@ static void update_local_status(qd_ctx *ctx, node_info_t *ni, int max, int score
 static int get_config_data(qd_ctx *ctx, struct h_data *h, int maxh, int *cfh);
 static int cman_wait(cman_handle_t ch, struct timeval *_tv);
 
+static int fake_cfh = 0;
 
 static void
 int_handler(int sig)
@@ -934,7 +935,7 @@ process_cman_event(cman_handle_t handle, void *private, int reason, int arg)
 		_cman_shutdown = 1;
 		break;
 	case CMAN_REASON_CONFIG_UPDATE:
-		get_config_data(ctx, NULL, 0, NULL);
+		get_config_data(ctx, NULL, 0, &fake_cfh);
 		break;
 	case CMAN_REASON_PORTCLOSED:
 		break;
@@ -978,7 +979,7 @@ quorum_loop(qd_ctx *ctx, node_info_t *ni, int max)
 	_running = 1;
 	while (_running) {
 		if (_reconfig) {
-			get_config_data(ctx, NULL, 0, NULL);
+			get_config_data(ctx, NULL, 0, &fake_cfh);
 			_reconfig = 0;
 		}
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 35/41] qdiskd: change variable type

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 9884ea7... d0d05f0... M	cman/qdisk/disk.c
 cman/qdisk/disk.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cman/qdisk/disk.c b/cman/qdisk/disk.c
index 9884ea7..d0d05f0 100644
--- a/cman/qdisk/disk.c
+++ b/cman/qdisk/disk.c
@@ -628,7 +628,8 @@ qdisk_write(target_info_t *disk, __off64_t offset, const void *buf, int count)
 	shared_header_t *hdrp;
 	void *ptr;
 	char *data;
-	size_t total = 0, rv = -1, psz = disk->d_blksz; //sysconf(_SC_PAGESIZE);
+	size_t total = 0, psz = disk->d_blksz; //sysconf(_SC_PAGESIZE);
+	int rv = -1;
 
 	maxsize = psz - (sizeof(shared_header_t));
 	if (count >= (maxsize + sizeof(shared_header_t))) {
@@ -677,7 +678,7 @@ qdisk_write(target_info_t *disk, __off64_t offset, const void *buf, int count)
 
 	if (rv == -1)
 		logt_print(LOG_ERR, "diskRawWriteShadow");
-	
+
 	free((char *)hdrp);
 	if (rv == -1)
 		return -1;
-- 
1.7.4.4

[Cluster-devel] [PATCH 36/41] qdiskd: implement better string handling

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 e962f74... 477b8de... M	cman/qdisk/scandisk.c
 cman/qdisk/scandisk.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/cman/qdisk/scandisk.c b/cman/qdisk/scandisk.c
index e962f74..477b8de 100644
--- a/cman/qdisk/scandisk.c
+++ b/cman/qdisk/scandisk.c
@@ -311,7 +311,7 @@ static int scanmdstat(struct devlisthead *devlisthead)
 	if (!fp)
 		return 0;
 
-	while (fgets(line, sizeof(line), fp) != NULL) {
+	while (fgets(line, sizeof(line) - 1, fp) != NULL) {
 
 		/* i like things to be absolutely clean */
 		memset(device, 0, sizeof(device));
@@ -321,7 +321,7 @@ static int scanmdstat(struct devlisthead *devlisthead)
 		memset(firstdevice, 0, sizeof(firstdevice));
 		memset(devices, 0, sizeof(devices));
 
-		if (strlen(line) > sizeof(line))
+		if (strlen(line) >= sizeof(line))
 			continue;
 
 		/* we only parse stuff that starts with ^md
@@ -347,8 +347,15 @@ static int scanmdstat(struct devlisthead *devlisthead)
 
 		/* trunkate the string from sdaX[Y] to sdaX and
 		 * copy the whole device string over */
-		memset(strstr(firstdevice, "["), 0, 1);
-		strcpy(devices, strstr(line, firstdevice));
+		tmp = strstr(firstdevice, "[");
+		if (!tmp)
+			continue;
+		memset(tmp, 0, 1);
+
+		tmp = strstr(line, firstdevice);
+		if (!tmp)
+			continue;
+		strncpy(devices, tmp, sizeof(devices) - 1);
 
 		/* if we don't find any slave (for whatever reason)
 		 * keep going */
@@ -357,8 +364,11 @@ static int scanmdstat(struct devlisthead *devlisthead)
 
 		tmp = devices;
 		while ((tmp) && ((next = strstr(tmp, " ")) || strlen(tmp))) {
+			char *tmp2;
 
-			memset(strstr(tmp, "["), 0, 1);
+			tmp2 = strstr(tmp, "[");
+			if (tmp2)
+				memset(tmp2, 0, 1);
 
 			startnode =
 			    find_dev_by_path(devlisthead->devnode, tmp, 1);
-- 
1.7.4.4

[Cluster-devel] [PATCH 36/41] qdiskd: implement better string handling

[Lon Hohberger]

On 11/23/2011 05:15 AM, Fabio M. Di Nitto wrote:
> Spotted by Coverity Scan
>
> Signed-off-by: Fabio M. Di Nitto<fdinitto@redhat.com>
> ---
> :100644 100644 e962f74... 477b8de... M	cman/qdisk/scandisk.c
>   cman/qdisk/scandisk.c |   20 +++++++++++++++-----
>   1 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/cman/qdisk/scandisk.c b/cman/qdisk/scandisk.c
> index e962f74..477b8de 100644
> --- a/cman/qdisk/scandisk.c
> +++ b/cman/qdisk/scandisk.c
> @@ -311,7 +311,7 @@ static int scanmdstat(struct devlisthead *devlisthead)
>   	if (!fp)
>   		return 0;
>
> -	while (fgets(line, sizeof(line), fp) != NULL) {
> +	while (fgets(line, sizeof(line) - 1, fp) != NULL) {

fgets reads size-1, this isn't needed.

-- Lon

[Cluster-devel] [PATCH 37/41] qdiskd: add failure paths to check_process_running and drop duplicate check

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 c260cb8... 2ba6fa0... M	cman/qdisk/daemon_init.c
:100644 100644 079af01... 175bdb6... M	cman/qdisk/main.c
 cman/qdisk/daemon_init.c |   27 ++++++++++++---------------
 cman/qdisk/main.c        |    9 ++++++++-
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/cman/qdisk/daemon_init.c b/cman/qdisk/daemon_init.c
index c260cb8..2ba6fa0 100644
--- a/cman/qdisk/daemon_init.c
+++ b/cman/qdisk/daemon_init.c
@@ -113,6 +113,11 @@ check_process_running(char *prog, pid_t * pid)
 
 	*pid = -1;
 
+	if (!prog) {
+		errno = EINVAL;
+		return -1;
+	}
+
 	/*
 	 * Now see if there is a pidfile associated with this cmd in /var/run
 	 */
@@ -120,7 +125,10 @@ check_process_running(char *prog, pid_t * pid)
 	memset(filename, 0, PATH_MAX);
 
 	cmd = basename(prog);
-	snprintf(filename, sizeof (filename), "/var/run/%s.pid", cmd);
+	if (!cmd)
+		return -1;
+
+	snprintf(filename, sizeof (filename) - 1, "/var/run/%s.pid", cmd);
 
 	ret = stat(filename, &st);
 	if ((ret < 0) || (!st.st_size))
@@ -130,9 +138,8 @@ check_process_running(char *prog, pid_t * pid)
 	 * Read the pid from the file.
 	 */
 	fp = fopen(filename, "r");
-	if (fp == NULL) {	/* error */
+	if (fp == NULL)		/* error */
 		return 0;
-	}
 
 	ret = fscanf(fp, "%d\n", &oldpid);
 	fclose(fp);
@@ -144,6 +151,7 @@ check_process_running(char *prog, pid_t * pid)
 		*pid = oldpid;
 		return 1;
 	}
+
 	return 0;
 }
 
@@ -200,22 +208,12 @@ setup_sigmask(void)
 void
 daemon_init(char *prog)
 {
-	uid_t uid;
-	pid_t pid;
-
-	uid = getuid();
-	if (uid) {
+	if (getuid()) {
 		logt_print(LOG_ERR,
 			"daemon_init: Sorry, only root wants to run this.\n");
 		exit(1);
 	}
 
-	if (check_process_running(prog, &pid) && (pid != getpid())) {
-		logt_print(LOG_ERR,
-			"daemon_init: Process \"%s\" already running.\n",
-			prog);
-		exit(1);
-	}
 	if (setup_sigmask() < 0) {
 		logt_print(LOG_ERR, "daemon_init: Unable to set signal mask.\n");
 		exit(1);
@@ -226,7 +224,6 @@ daemon_init(char *prog)
 		exit(1);
 	}
 
-
 	update_pidfile(prog);
 }
 
diff --git a/cman/qdisk/main.c b/cman/qdisk/main.c
index 079af01..175bdb6 100644
--- a/cman/qdisk/main.c
+++ b/cman/qdisk/main.c
@@ -1980,7 +1980,14 @@ main(int argc, char **argv)
 	quorum_header_t qh;
 	qd_priv_t qp;
 
-	if (check_process_running(argv[0], &pid) && pid !=getpid()) {
+
+	rv = check_process_running(argv[0], &pid);
+	if (rv < 0) {
+		fprintf(stderr, "Unable to determin if %s is already running: %s\n",
+			argv[0], strerror(errno));
+		return -1;
+	}
+	if (rv && pid !=getpid()) {
 		printf("QDisk services already running\n");
 		return 0;
 	}
-- 
1.7.4.4

[Cluster-devel] [PATCH 37/41] qdiskd: add failure paths to check_process_running and drop duplicate check

[Lon Hohberger]

On 11/23/2011 05:15 AM, Fabio M. Di Nitto wrote:
> +		fprintf(stderr, "Unable to determin if %s is already running: %s\n",
> +			argv[0], strerror(errno));

Typo (determine)

-- Lon

[Cluster-devel] [PATCH 38/41] qdiskd: fix possible resource leak in scandisk

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 477b8de... b99a672... M	cman/qdisk/scandisk.c
 cman/qdisk/scandisk.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/cman/qdisk/scandisk.c b/cman/qdisk/scandisk.c
index 477b8de..b99a672 100644
--- a/cman/qdisk/scandisk.c
+++ b/cman/qdisk/scandisk.c
@@ -252,11 +252,12 @@ static int scanprocpart(struct devlisthead *devlisthead)
 	unsigned long long blkcnt;
 	char device[128];
 	struct devnode *startnode;
+
 	fp = fopen("/proc/partitions", "r");
 	if (!fp)
 		return 0;
-	while (fgets(line, sizeof(line), fp)
-	       != NULL) {
+
+	while (fgets(line, sizeof(line) - 1, fp) != NULL) {
 
 		if (strlen(line) > 128 + (22))
 			continue;
@@ -270,16 +271,19 @@ static int scanprocpart(struct devlisthead *devlisthead)
 		 */
 		if (!strlen(device))
 			continue;
+
 		startnode =
 		    find_dev_by_majmin(devlisthead->devnode, major, minor);
 		if (!startnode) {
 			startnode = alloc_list_obj(devlisthead, major, minor);
-			if (!startnode)
+			if (!startnode) {
+				fclose(fp);
 				return -2;
+			}
 		}
 
 		startnode->procpart = 1;
-		strcpy(startnode->procname, device);
+		strncpy(startnode->procname, device, sizeof(startnode->procname) - 1);
 	}
 
 	fclose(fp);
-- 
1.7.4.4

[Cluster-devel] [PATCH 38/41] qdiskd: fix possible resource leak in scandisk

[Lon Hohberger]

On 11/23/2011 05:15 AM, Fabio M. Di Nitto wrote:
> Spotted by Coverity Scan
>

> -	while (fgets(line, sizeof(line), fp)
> -	       != NULL) {
> +
> +	while (fgets(line, sizeof(line) - 1, fp) != NULL) {

fgets takes at most size-1, this isn't needed

-- Lon

[Cluster-devel] [PATCH 39/41] qdiskd: don't leak memory if we fail to read from disk

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 d0d05f0... 7c6e114... M	cman/qdisk/disk.c
 cman/qdisk/disk.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/cman/qdisk/disk.c b/cman/qdisk/disk.c
index d0d05f0..7c6e114 100644
--- a/cman/qdisk/disk.c
+++ b/cman/qdisk/disk.c
@@ -604,6 +604,7 @@ qdisk_read(target_info_t *disk, __off64_t offset, void *bufin, int count)
 	rv = diskRawReadShadow(disk, offset, (char *)hdrp, disk->d_blksz);
 	
 	if (rv == -1) {
+		free(hdrp);
 		return -1;
 	}
 	
-- 
1.7.4.4

[Cluster-devel] [PATCH 40/41] qdiskd: fix uninitialized values

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 b99a672... b80046b... M	cman/qdisk/scandisk.c
 cman/qdisk/scandisk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cman/qdisk/scandisk.c b/cman/qdisk/scandisk.c
index b99a672..b80046b 100644
--- a/cman/qdisk/scandisk.c
+++ b/cman/qdisk/scandisk.c
@@ -642,7 +642,7 @@ static int sysfs_is_disk(char *path)
 static int scansysfs(struct devlisthead *devlisthead, const char *path, int level, int parent_holder)
 {
 	struct devnode *startnode;
-	int i, n, maj, min, has_holder;
+	int i, n, maj = -1, min = -1, has_holder;
 	struct dirent **namelist;
 	struct stat sb;
 	char newpath[MAXPATHLEN];
-- 
1.7.4.4

[Cluster-devel] [PATCH 41/41] qdiskd: fix more uninizialized values

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 7c6e114... ec47a5f... M	cman/qdisk/disk.c
 cman/qdisk/disk.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/cman/qdisk/disk.c b/cman/qdisk/disk.c
index 7c6e114..ec47a5f 100644
--- a/cman/qdisk/disk.c
+++ b/cman/qdisk/disk.c
@@ -756,19 +756,10 @@ qdisk_init(char *partname, char *label)
 
 	time(&t);
 
+	memset(&ps, 0, sizeof(status_block_t));
+
 	ps.ps_magic = STATE_MAGIC_NUMBER;
-	ps.ps_updatenode = 0;
-	ps.pad0 = 0;
 	ps.ps_timestamp = (uint64_t)t;
-	ps.ps_state = (uint8_t)S_NONE;
-	ps.pad1[0] = 0;
-	ps.ps_flags = 0;
-	ps.ps_score = 0;
-	ps.ps_scoremax = 0;
-	ps.ps_ca_sec = 0;
-	ps.ps_ca_usec = 0;
-	ps.ps_lc_sec = 0;
-	ps.ps_ca_usec = 0;
 
 	/* Node IDs 1..N */
 	for (nid = 1; nid <= MAX_NODES_DISK; nid++) {
-- 
1.7.4.4