Merge lp:~cbehrens/agent-smith/error-fixes into lp:agent-smith

[Chris Behrens]

Unfortunately I don't have a way to compile this right now, but the diff looks sane. This contains a bunch of error checking fixes. It also fixes at least 1 memory leak under normal conditions and a few under error conditions...

[lp:~cbehrens/agent-smith/error-fixes]

71. By Chris Behrens on 2010-08-24

fix so the tests at least build...
missing argument to strbuf_free
autoreconf requirement dropped a rev since I'm using older ubuntu for now

[Chris Behrens (cbehrens)]

Looks ok now after fixing compile issues and making tests build.

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2010-08-09 13:17:54 +0000
+++ configure.ac	2010-08-24 15:57:43 +0000
@@ -1,4 +1,4 @@
-AC_PREREQ([2.65])
+AC_PREREQ([2.64])
 AC_INIT([agentsmith], [0.1], [[e-mail]])
 AC_CONFIG_SRCDIR([src/xen.h])
 AC_CONFIG_HEADERS([config.h])

=== modified file 'src/slist.c'
--- src/slist.c	2010-07-07 09:26:52 +0000
+++ src/slist.c	2010-08-24 15:57:43 +0000
@@ -19,6 +19,9 @@
 SList *slist_append(SList *list, void *data) {
     SList *elem = malloc(sizeof(SList));
 
+    if (elem == NULL)
+        return NULL;
+
     elem->data = data;
     elem->next = NULL;
 

=== modified file 'src/spool.c'
--- src/spool.c	2010-08-23 23:18:52 +0000
+++ src/spool.c	2010-08-24 15:57:43 +0000
@@ -43,7 +43,7 @@
 int spool_init(void) {
     char *tmp;
 
-    if (spool_ensure_dir(tmp = spool_incoming_dir()) == 0)
+    if (spool_ensure_dir(tmp = spool_incoming_dir()) < 0)
         free(tmp);
     else {
         syslog(LOG_DAEMON | LOG_CRIT, "Failed to create incoming spool dir: %s", tmp);
@@ -51,7 +51,7 @@
         return -1;
     }
 
-    if (spool_ensure_dir(tmp = spool_outgoing_dir()) == 0)
+    if (spool_ensure_dir(tmp = spool_outgoing_dir()) < 0)
         free(tmp);
     else {
         syslog(LOG_DAEMON | LOG_CRIT, "Failed to create outgoing spool dir: %s", tmp);
@@ -90,7 +90,12 @@
 
     basedir = spool_basedir();
     retval = util_join_paths(basedir, in ? INCOMING_DIR : OUTGOING_DIR);
-    spool_ensure_dir(retval);
+    if (retval == NULL)
+        return NULL;
+    if (spool_ensure_dir(retval) < 0) {
+        free(retval);
+        return NULL;
+    }
     return retval;
 }
 
@@ -207,14 +212,24 @@
     char *dir, *msgpath;
 
     dir = spool_outgoing_dir();
+    if (dir == NULL)
+        return -1;
 
-    if (spool_ensure_dir(dir) < 0)
+    if (spool_ensure_dir(dir) < 0) {
+        free(dir);
         return -1;
+    }
 
     msgpath = util_join_paths(dir, msgid);
 
-    if (spool_dump_buf_to_file(buf, buflen, msgpath) < 0)
+    free(dir);
+
+    if (spool_dump_buf_to_file(buf, buflen, msgpath) < 0) {
+        free(msgpath);
         return -1;
+    }
+
+    free(msgpath);
 
     return 0;
 }
@@ -229,26 +244,36 @@
     if (inotify_fd == -1) {
         inotify_fd = inotify_init();
         if (inotify_fd < 0)
-            return 0;
+            return -1;
 
         if (fcntl(inotify_fd, F_SETFL, O_NONBLOCK) < 0) {
             close(inotify_fd);
-            return 0;
+            inotify_fd = -1;
+            return -1;
         }
     }
 
     incoming_dir = spool_incoming_dir();
-    if (spool_ensure_dir(incoming_dir) < 0)
-        return 0;
+    if (spool_ensure_dir(incoming_dir) < 0) {
+        free(incoming_dir);
+        return -1;
+    }
 
-    if (inotify_add_watch(inotify_fd, incoming_dir, IN_CLOSE_WRITE) < 0)
+    if (inotify_add_watch(inotify_fd, incoming_dir, IN_CLOSE_WRITE) < 0) {
+        free(incoming_dir);
         perror("inotify_add_watch failed");
+        return -1;
+    }
+
+    free(incoming_dir);
 
     callbacks = slist_append(callbacks, cb);
-
-    free(incoming_dir);
-
-    return 1;
+    if (callbacks == NULL)
+    {
+        return -1;
+    }
+
+    return 0;
 }
 
 #define INOTIFY_BUFF_SIZE ((sizeof(struct inotify_event)+FILENAME_MAX))

=== modified file 'src/util.c'
--- src/util.c	2010-08-20 15:18:18 +0000
+++ src/util.c	2010-08-24 15:57:43 +0000
@@ -25,17 +25,18 @@
  * @returns: a pointer to a strbuf on success, NULL otherwise
  */
 strbuf *strbuf_init() {
-	strbuf *str;
-	if ((str = malloc(sizeof(strbuf))) == NULL)
-            return NULL;
-	
-	if ((str->str = strdup("")) == NULL) {
-            free(str);
-            return NULL;
-        }
-
-	str->len = 0;
-	return str;
+    strbuf *str;
+
+    if ((str = malloc(sizeof(strbuf))) == NULL)
+        return NULL;
+    
+    if ((str->str = strdup("")) == NULL) {
+        free(str);
+        return NULL;
+    }
+
+    str->len = 0;
+    return str;
 }
 
 /*
@@ -46,35 +47,41 @@
  *                 freed, otherwise it is returned.
  */
 char *strbuf_free(strbuf *str, char free_it) {
-	char *s = str->str;
-
-	free(str);
-
-	if (free_it) {
-		free(s);
-		return NULL;
-	} else {
-		return s;
-	}
+    char *s = str->str;
+
+    free(str);
+
+    if (free_it) {
+        free(s);
+        return NULL;
+    } else {
+        return s;
+    }
 }
 
 strbuf *strbuf_append_printf(strbuf *str, const char *fmt, ...) {
-	char *s = NULL;
-	size_t strlen;
-	va_list ap;
-
-	va_start(ap, fmt);
-	strlen = vsnprintf(s, 0, fmt, ap);
-	va_end(ap);
-
-	str->str = realloc(str->str, str->len + strlen + 1);
-	va_start(ap, fmt);
-	vsnprintf(str->str+str->len, strlen+1, fmt, ap);
-	va_end(ap);
-
-	str->len+=strlen;
-
-	return str;
+    char *s = NULL;
+    void *vptr;
+    size_t strlen;
+    va_list ap;
+
+    va_start(ap, fmt);
+    strlen = vsnprintf(s, 0, fmt, ap);
+    va_end(ap);
+
+    vptr = realloc(str->str, str->len + strlen + 1);
+    if (vptr == NULL)
+        return NULL;
+
+    str->str = vptr;
+
+    va_start(ap, fmt);
+    vsnprintf(str->str+str->len, strlen+1, fmt, ap);
+    va_end(ap);
+
+    str->len+=strlen;
+
+    return str;
 }
 
 /**
@@ -85,9 +92,17 @@
  * @return Newly allocated string: part1 + "/" + part2
  */
 char *util_join_paths(const char *part1, const char *part2) {
-	strbuf *str;
+    strbuf *str;
+    strbuf *nstr;
 
-	str = strbuf_init();
-	str = strbuf_append_printf(str, "%s/%s", part1, part2);
-	return strbuf_free(str, 0);
+    str = strbuf_init();
+    if (str == NULL)
+        return NULL;
+    nstr = strbuf_append_printf(str, "%s/%s", part1, part2);
+    if (nstr == NULL) {
+        strbuf_free(str, 1);
+        return NULL;
+    }
+    str = nstr;
+    return strbuf_free(str, 0);
 }

=== modified file 'tests/Makefile.am'
--- tests/Makefile.am	2010-07-20 17:18:16 +0000
+++ tests/Makefile.am	2010-08-24 15:57:43 +0000
@@ -8,13 +8,13 @@
 check_slist_LDADD = $(top_builddir)/src/slist.o @CHECK_LIBS@
 
 check_xen_SOURCES = check_xen.c $(top_builddir)/src/slist.h $(top_builddir)/src/xen.h mock_xenstore.c
-check_xen_LDADD = $(top_builddir)/src/xen.o $(top_builddir)/src/slist.o @CHECK_LIBS@
+check_xen_LDADD = $(top_builddir)/src/xen.o $(top_builddir)/src/slist.o $(top_builddir)/src/log.o @CHECK_LIBS@
 
 check_mock_xenstore_SOURCES = check_mock_xenstore.c mock_xenstore.c
 check_mock_xenstore_LDADD = @CHECK_LIBS@ $(top_builddir)/src/slist.o
 
 check_spool_SOURCES = check_spool.c $(top_builddir)/src/spool.h $(top_builddir)/src/util.h
-check_spool_LDADD = $(top_builddir)/src/slist.o $(top_builddir)/src/spool.o $(top_builddir)/src/util.o @CHECK_LIBS@
+check_spool_LDADD = $(top_builddir)/src/slist.o $(top_builddir)/src/spool.o $(top_builddir)/src/util.o $(top_builddir)/src/log.o @CHECK_LIBS@
 
 check_util_SOURCES = check_util.c $(top_builddir)/src/util.h
 check_util_LDADD = $(top_builddir)/src/util.o @CHECK_LIBS@
@@ -23,4 +23,4 @@
 check_json_LDADD = $(top_builddir)/src/json.o $(top_builddir)/src/util.o @CHECK_LIBS@ -ljson
 
 check_agent_SOURCES = check_agent.c $(top_builddir)/src/agent.h $(top_builddir)/src/xen.h $(top_builddir)/src/util.h $(top_builddir)/src/slist.h mock_xenstore.c
-check_agent_LDADD = $(top_builddir)/src/json.o $(top_builddir)/src/agent.o $(top_builddir)/src/spool.o $(top_builddir)/src/util.o $(top_builddir)/src/xen.o $(top_builddir)/src/slist.o @CHECK_LIBS@ -ljson
+check_agent_LDADD = $(top_builddir)/src/json.o $(top_builddir)/src/agent.o $(top_builddir)/src/spool.o $(top_builddir)/src/util.o $(top_builddir)/src/xen.o $(top_builddir)/src/slist.o $(top_builddir)/src/log.o @CHECK_LIBS@ -ljson

=== modified file 'tests/check_spool.c'
--- tests/check_spool.c	2010-07-07 09:26:52 +0000
+++ tests/check_spool.c	2010-08-24 15:57:43 +0000
@@ -112,7 +112,7 @@
     FILE *fp;
 
     setenv("AGENT_MSG_SPOOL", testdir, 1);
-    fail_unless(spool_watch(spool_cb) == 1, "Failed to add watch.");
+    fail_unless(spool_watch(spool_cb) == 0, "Failed to add watch.");
     spool_process_pending_events();
     fail_unless(spool_cb_called == 0, "Spool callback got called even though nothing had happened yet.");
     fp = fopen(util_join_paths(spool_incoming_dir(), "testfile"), "a+");