[Cluster-devel] [coverity] liblogthread

[Cluster-devel] [coverity] liblogthread

[Fabio M. Di Nitto]

This is the first patchset to address some issues spotted by Coverity scan.

Original errors/warnings:

Analysis summary report:
------------------------
Files analyzed                 : 1
Total LoC input to cov-analyze : 12453
Functions analyzed             : 11
Paths analyzed                 : 243
New defects found              : 8 Total
                                 1 ATOMICITY
                                 3 BUFFER_SIZE_WARNING
                                 1 MISSING_LOCK
                                 1 NULL_RETURNS
                                 1 PW.NOT_COMPATIBLE_WITH_PREVIOUS_DECL
                                 1 SECURE_CODING

Elapsed time: 00:00:00
Processing 8 C/C++ errors.
 
0%   10   20   30   40   50   60   70   80   90   100%
|----|----|----|----|----|----|----|----|----|----|
***************************************************
Processed 8 C/C++ errors.

After fixes:

Analysis summary report:
------------------------
Files analyzed                 : 1
Total LoC input to cov-analyze : 12475
Functions analyzed             : 12
Paths analyzed                 : 250
New defects found              : 2 Total
                                 1 ATOMICITY
                                 1 PW.NOT_COMPATIBLE_WITH_PREVIOUS_DECL

Elapsed time: 00:00:01
Processing 2 C/C++ errors.

0%   10   20   30   40   50   60   70   80   90   100%
|----|----|----|----|----|----|----|----|----|----|
***************************************************
Processed 2 C/C++ errors.

NOTES:

PW.NOT_COMPATIBLE_WITH_PREVIOUS_DECL can be safely ignored as it is an incompatible
declaration between system gcc and coverity headers.

The remaining ATOMICITY error appears to be bogus.

[Cluster-devel] [PATCH 1/4] liblogthread: call to localtime needs return value check

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 ba96a2a... 4f8354a... M	common/liblogthread/liblogthread.c
 common/liblogthread/liblogthread.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/common/liblogthread/liblogthread.c b/common/liblogthread/liblogthread.c
index ba96a2a..4f8354a 100644
--- a/common/liblogthread/liblogthread.c
+++ b/common/liblogthread/liblogthread.c
@@ -42,8 +42,15 @@ static FILE *logt_logfile_fp;
 static char *_time(time_t *t)
 {
 	static char buf[64];
+	struct tm *tm;
+
+	tm = localtime(t);
+	if (!tm) {
+		strncpy(buf, "unknown time", sizeof(buf) - 1);
+	} else {
+		strftime(buf, sizeof(buf), "%b %d %T", tm);
+	}
 
-	strftime(buf, sizeof(buf), "%b %d %T", localtime(t));
 	return buf;
 }
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 2/4] liblogthread: make sure there is space for \0 end string

[Fabio M. Di Nitto]

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 4f8354a... 384f042... M	common/liblogthread/liblogthread.c
 common/liblogthread/liblogthread.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/common/liblogthread/liblogthread.c b/common/liblogthread/liblogthread.c
index 4f8354a..384f042 100644
--- a/common/liblogthread/liblogthread.c
+++ b/common/liblogthread/liblogthread.c
@@ -128,7 +128,7 @@ static void _logt_print(int level, char *buf)
 	head_ent = head_ent % num_ents;
 	pending_ents++;
 
-	strncpy(e->str, buf, ENTRY_STR_LEN);
+	strncpy(e->str, buf, ENTRY_STR_LEN - 1);
 	e->level = level;
 	e->time = time(NULL);
  out:
@@ -172,9 +172,9 @@ static void _conf(const char *name, int mode, int syslog_facility,
 	logt_syslog_priority = syslog_priority;
 	logt_logfile_priority = logfile_priority;
 	if (name)
-		strncpy(logt_name, name, PATH_MAX);
+		strncpy(logt_name, name, PATH_MAX - 1);
 	if (logfile)
-		strncpy(logt_logfile, logfile, PATH_MAX);
+		strncpy(logt_logfile, logfile, PATH_MAX - 1);
 
 	if (logt_mode & LOG_MODE_OUTPUT_FILE && logt_logfile[0]) {
 		if (logt_logfile_fp) {
@@ -255,11 +255,11 @@ int logt_reinit(void)
 	memset(name_tmp, 0, sizeof(name_tmp));
 	memset(file_tmp, 0, sizeof(file_tmp));
 
-	strncpy(name_tmp, logt_name, sizeof(name_tmp));
+	strncpy(name_tmp, logt_name, sizeof(name_tmp) - 1);
 	if (!strlen(name_tmp))
 		return -1;
 	if (strlen(logt_logfile))
-		strncpy(file_tmp, logt_logfile, sizeof(file_tmp));
+		strncpy(file_tmp, logt_logfile, sizeof(file_tmp) - 1);
 
 	return logt_init(name_tmp, logt_mode, logt_syslog_facility,
 			 logt_syslog_priority, logt_logfile_priority,
-- 
1.7.4.4

[Cluster-devel] [PATCH 3/4] liblogthread: use snprintf as it is considered safer

[Fabio M. Di Nitto]

avoid security warning

Spotted by Coverity Scan

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 384f042... 7230b2f... M	common/liblogthread/liblogthread.c
 common/liblogthread/liblogthread.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/common/liblogthread/liblogthread.c b/common/liblogthread/liblogthread.c
index 384f042..7230b2f 100644
--- a/common/liblogthread/liblogthread.c
+++ b/common/liblogthread/liblogthread.c
@@ -69,7 +69,7 @@ static void write_entry(int level, time_t *t, char *str)
 static void write_dropped(int level, time_t *t, int num)
 {
 	char str[ENTRY_STR_LEN];
-	sprintf(str, "dropped %d entries", num);
+	snprintf(str, sizeof(str) - 1, "dropped %d entries", num);
 	write_entry(level, t, str);
 }
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 4/4] liblogthread: make library thread safer

[Fabio M. Di Nitto]

- all public API should lock/unlock appropriately
- internal _conf function is now lock-free, locking must
  be provided by callers. this saves different unlock/race/lock
  conditions.
- make init and re-init thread safe and move common code to _init

Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
---
:100644 100644 7230b2f... 088d171... M	common/liblogthread/liblogthread.c
 common/liblogthread/liblogthread.c |   51 ++++++++++++++++++++++++-----------
 1 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/common/liblogthread/liblogthread.c b/common/liblogthread/liblogthread.c
index 7230b2f..088d171 100644
--- a/common/liblogthread/liblogthread.c
+++ b/common/liblogthread/liblogthread.c
@@ -166,7 +166,6 @@ static void _conf(const char *name, int mode, int syslog_facility,
 {
 	int fd;
 
-	pthread_mutex_lock(&mutex);
 	logt_mode = mode;
 	logt_syslog_facility = syslog_facility;
 	logt_syslog_priority = syslog_priority;
@@ -197,24 +196,22 @@ static void _conf(const char *name, int mode, int syslog_facility,
 		closelog();
 		openlog(logt_name, LOG_CONS | LOG_PID, logt_syslog_facility);
 	}
-	pthread_mutex_unlock(&mutex);
 }
 
 void logt_conf(const char *name, int mode, int syslog_facility, int syslog_priority,
 	       int logfile_priority, const char *logfile)
 {
-	if (!init)
-		return;
+	pthread_mutex_lock(&mutex);
+	if (init)
+		_conf(name, mode, syslog_facility, syslog_priority, logfile_priority,
+		      logfile);
 
-	_conf(name, mode, syslog_facility, syslog_priority, logfile_priority,
-	      logfile);
+	pthread_mutex_unlock(&mutex);
 }
 
-int logt_init(const char *name, int mode, int syslog_facility, int syslog_priority,
+static int _init(const char *name, int mode, int syslog_facility, int syslog_priority,
 	      int logfile_priority, const char *logfile)
 {
-	int rv;
-
 	if (init)
 		return -1;
 
@@ -224,18 +221,30 @@ int logt_init(const char *name, int mode, int syslog_facility, int syslog_priori
 	ents = malloc(num_ents * sizeof(struct entry));
 	if (!ents)
 		return -1;
+
 	memset(ents, 0, num_ents * sizeof(struct entry));
 
-	rv = pthread_create(&thread_handle, NULL, thread_fn, NULL);
-	if (rv) {
+	if (pthread_create(&thread_handle, NULL, thread_fn, NULL)) {
 		free(ents);
 		return -1;
 	}
 	done = 0;
 	init = 1;
+
 	return 0;
 }
 
+int logt_init(const char *name, int mode, int syslog_facility, int syslog_priority,
+              int logfile_priority, const char *logfile)
+{
+	int rv = 0;
+
+	pthread_mutex_lock(&mutex);
+	rv = _init(name, mode, syslog_facility, syslog_priority,
+			 logfile_priority, logfile);
+	pthread_mutex_unlock(&mutex);
+	return rv;
+}
 
 /*
  * Reinitialize logt w/ previous values (e.g. use after
@@ -247,23 +256,33 @@ int logt_reinit(void)
 {
 	char name_tmp[PATH_MAX];
 	char file_tmp[PATH_MAX];
+	int rv = 0;
 
-	if (!done || init)
-		return -1;
+	pthread_mutex_lock(&mutex);
+	if (!done || init) {
+		rv = -1;
+		goto out;
+	}
 
 	/* Use copies on the stack for these */
 	memset(name_tmp, 0, sizeof(name_tmp));
 	memset(file_tmp, 0, sizeof(file_tmp));
 
 	strncpy(name_tmp, logt_name, sizeof(name_tmp) - 1);
-	if (!strlen(name_tmp))
-		return -1;
+	if (!strlen(name_tmp)) {
+		rv = -1;
+		goto out;
+	}
 	if (strlen(logt_logfile))
 		strncpy(file_tmp, logt_logfile, sizeof(file_tmp) - 1);
 
-	return logt_init(name_tmp, logt_mode, logt_syslog_facility,
+	rv = _init(name_tmp, logt_mode, logt_syslog_facility,
 			 logt_syslog_priority, logt_logfile_priority,
 			 file_tmp);
+
+out:
+	pthread_mutex_unlock(&mutex);
+	return rv;
 }
 
 
-- 
1.7.4.4

[Cluster-devel] [PATCH 1/4] liblogthread: call to localtime needs return value check

[Steven Whitehouse]

Hi,

On Mon, 2011-10-10 at 10:45 +0200, Fabio M. Di Nitto wrote:
> Spotted by Coverity Scan
> 
> Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> ---
> :100644 100644 ba96a2a... 4f8354a... M	common/liblogthread/liblogthread.c
>  common/liblogthread/liblogthread.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/common/liblogthread/liblogthread.c b/common/liblogthread/liblogthread.c
> index ba96a2a..4f8354a 100644
> --- a/common/liblogthread/liblogthread.c
> +++ b/common/liblogthread/liblogthread.c
> @@ -42,8 +42,15 @@ static FILE *logt_logfile_fp;
>  static char *_time(time_t *t)
>  {
>  	static char buf[64];
> +	struct tm *tm;
> +
> +	tm = localtime(t);
> +	if (!tm) {
> +		strncpy(buf, "unknown time", sizeof(buf) - 1);
> +	} else {
> +		strftime(buf, sizeof(buf), "%b %d %T", tm);
> +	}
>  
> -	strftime(buf, sizeof(buf), "%b %d %T", localtime(t));
>  	return buf;
>  }
>  

I'm wondering under just what circumstances localtime would return
NULL... since the returned buffer is static, there is no allocation that
could fail. Since the time_t is just seconds since the epoc, all
possible values are valid. Maybe if it was passed a NULL time_t *
perhaps, but that is, I suspect impossible.

So I'm wondering whether this is a false positive,

Steve.

[Cluster-devel] [coverity] liblogthread

[David Teigland]

On Mon, Oct 10, 2011 at 10:45:17AM +0200, Fabio M. Di Nitto wrote:
> This is the first patchset to address some issues spotted by Coverity scan.

look fine