[PATCH 0/3] Fix userspace audit compiler warnings

[PATCH 0/3] Fix userspace audit compiler warnings

[Tyler Hicks]

These patches fix the majority of compiler warnings seen when building audit on 
Ubuntu.

The audit codebase rarely checked the return value of asprintf(), which can 
cause problems if memory allocation fails. Most of the missing checks are
in libauparse, which is probably fine but who knows what will use it in the 
future. Also, all of these warnings clutter up the build output and make it
more difficult to see more serious warnings.

The second patch contains some other ignored return value warnings fixes, as well. These show up on Ubuntu when the -D_FORTIFY_SOURCE=2 and -Wunused-return flags are in use. They don't seem to show up on Fedora in the same situation, but I believe that they should, despite casting the function's return value to (void). Despite that, they looked to be areas in the code that should at least log a message or even prevent auditd from running in some situations.  

The last patch cleans up two const warnings.

There are still a number of warnings in the zos-remote audispd plugin that I
didn't get to and a few that look ok to me in the embedded libev code.

Tyler

[PATCH 1/3] Don't ignore the return value of asprintf()

[Tyler Hicks]

If an error occurs in asprintf(), the contents of the strp variable are
undefined. asprintf()'s return value must be checked and the parameter
passed into asprintf() should be set to NULL upon error.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 audisp/audispd.c                        |    1 +
 audisp/plugins/prelude/audisp-prelude.c |    4 +-
 auparse/ellist.c                        |    3 +-
 auparse/expression.c                    |   87 +++++-----
 auparse/interpret.c                     |  265 +++++++++++++++++++------------
 lib/audit_logging.c                     |    6 +-
 src/auditctl.c                          |   22 +--
 7 files changed, 232 insertions(+), 156 deletions(-)

diff --git a/audisp/audispd.c b/audisp/audispd.c
index 9216fb0..749128f 100644
--- a/audisp/audispd.c
+++ b/audisp/audispd.c
@@ -613,6 +613,7 @@ static int event_loop(void)
 			len = asprintf(&v, "type=%s msg=%.*s\n", 
 				type, e->hdr.size, e->data);
 		if (len <= 0) {
+			v = NULL;
 			free(e); /* Either corrupted event or no memory */
 			continue;
 		}
diff --git a/audisp/plugins/prelude/audisp-prelude.c b/audisp/plugins/prelude/audisp-prelude.c
index 5cf1bef..0724b21 100644
--- a/audisp/plugins/prelude/audisp-prelude.c
+++ b/audisp/plugins/prelude/audisp-prelude.c
@@ -967,7 +967,9 @@ static int add_execve_data(auparse_state_t *au, idmef_alert_t *alert)
 			int len2;
 			len2 = asprintf(&ptr, "%s=%s ", var,
 					auparse_interpret_field(au));
-			if (len2 > 0 && (len2 + len + 1) < sizeof(msg)) {
+			if (len2 < 0) {
+				ptr = NULL;
+			} else if (len2 > 0 && (len2 + len + 1) < sizeof(msg)) {
 				strcat(msg, ptr);
 				len += len2;
 			}
diff --git a/auparse/ellist.c b/auparse/ellist.c
index eafcfee..81d2d1e 100644
--- a/auparse/ellist.c
+++ b/auparse/ellist.c
@@ -91,7 +91,8 @@ static char *escape(const char *tmp)
 		}
 		p++;
 	}
-	asprintf(&name, "\"%s\"", tmp);
+	if (asprintf(&name, "\"%s\"", tmp) < 0)
+		name = NULL;
 	return name;
 }
 
diff --git a/auparse/expression.c b/auparse/expression.c
index 038fb8a..b94079c 100644
--- a/auparse/expression.c
+++ b/auparse/expression.c
@@ -191,9 +191,10 @@ lex(struct parsing *p)
 			if (*p->src == '\\') {
 				p->src++;
 				if (*p->src != '\\' && *p->src != delimiter) {
-					*p->error = NULL;
-					asprintf(p->error, "Unknown escape "
-						 "sequence ``\\%c''", *p->src);
+					if (asprintf(p->error, "Unknown escape "
+						     "sequence ``\\%c''",
+						     *p->src) < 0)
+						*p->error = NULL;
 					free(buf);
 					return -1;
 				}
@@ -381,17 +382,17 @@ parse_timestamp_value(struct expr *dest, struct parsing *p)
 	if (sscanf(p->token_value, "ts:%jd.%u", &sec,
 		   &dest->v.p.value.timestamp.milli)
 	    != 2) {
-		*p->error = NULL;
-		asprintf(p->error, "Invalid timestamp value `%.*s'",
-			 p->token_len, p->token_start);
+		if (asprintf(p->error, "Invalid timestamp value `%.*s'",
+			     p->token_len, p->token_start) < 0)
+			*p->error = NULL;
 		return -1;
 	}
 	/* FIXME: validate milli */
 	dest->v.p.value.timestamp.sec = sec;
 	if (dest->v.p.value.timestamp.sec != sec) {
-		*p->error = NULL;
-		asprintf(p->error, "Timestamp overflow in `%.*s'", p->token_len,
-			 p->token_start);
+		if (asprintf(p->error, "Timestamp overflow in `%.*s'",
+			     p->token_len, p->token_start) < 0)
+			*p->error = NULL;
 		return -1;
 	}
 	dest->precomputed_value = 1;
@@ -410,9 +411,9 @@ parse_record_type_value(struct expr *dest, struct parsing *p)
 	assert(p->token == T_STRING);
 	type = audit_name_to_msg_type(p->token_value);
 	if (type < 0) {
-		*p->error = NULL;
-		asprintf(p->error, "Invalid record type `%.*s'", p->token_len,
-			 p->token_start);
+		if (asprintf(p->error, "Invalid record type `%.*s'",
+			     p->token_len, p->token_start) < 0)
+			*p->error = NULL;
 		return -1;
 	}
 	dest->v.p.value.int_value = type;
@@ -452,9 +453,9 @@ parse_comparison_regexp(struct parsing *p, struct expr *res)
 	if (lex(p) != 0)
 		goto err_res;
 	if (p->token != T_STRING && p->token != T_REGEXP) {
-		*p->error = NULL;
-		asprintf(p->error, "Regexp expected, got `%.*s'", p->token_len,
-			 p->token_start);
+		if (asprintf(p->error, "Regexp expected, got `%.*s'",
+			     p->token_len, p->token_start) < 0)
+			*p->error = NULL;
 		goto err_res;
 	}
 	res->v.regexp = parser_malloc(p, sizeof(*res->v.regexp));
@@ -470,8 +471,8 @@ parse_comparison_regexp(struct parsing *p, struct expr *res)
 		if (err_msg == NULL)
 			goto err_res_regexp;
 		regerror(err, res->v.regexp, err_msg, err_size);
-		*p->error = NULL;
-		asprintf(p->error, "Invalid regexp: %s", err_msg);
+		if (asprintf(p->error, "Invalid regexp: %s", err_msg) < 0)
+			*p->error = NULL;
 		free(err_msg);
 		goto err_res_regexp;
 	}
@@ -514,9 +515,10 @@ parse_comparison(struct parsing *p)
 		res->virtual_field = 1;
 		if (parse_escaped_field_name(&res->v.p.field.id, p->token_value)
 		    != 0) {
-			*p->error = NULL;
-			asprintf(p->error, "Unknown escaped field name `%.*s'",
-				 p->token_len, p->token_start);
+			if (asprintf(p->error,
+				     "Unknown escaped field name `%.*s'",
+				     p->token_len, p->token_start) < 0)
+				*p->error = NULL;
 			goto err_res;
 		}
 	} else {
@@ -534,9 +536,9 @@ parse_comparison(struct parsing *p)
 		if (lex(p) != 0)
 			goto err_field;
 		if (p->token != T_STRING) {
-			*p->error = NULL;
-			asprintf(p->error, "Value expected, got `%.*s'",
-				 p->token_len, p->token_start);
+			if (asprintf(p->error, "Value expected, got `%.*s'",
+				     p->token_len, p->token_start) < 0)
+				*p->error = NULL;
 			goto err_field;
 		}
 		res->precomputed_value = 0;
@@ -554,16 +556,16 @@ parse_comparison(struct parsing *p)
 		if (lex(p) != 0)
 			goto err_field;
 		if (p->token != T_STRING) {
-			*p->error = NULL;
-			asprintf(p->error, "Value expected, got `%.*s'",
-				 p->token_len, p->token_start);
+			if (asprintf(p->error, "Value expected, got `%.*s'",
+				     p->token_len, p->token_start) < 0)
+				*p->error = NULL;
 			goto err_field;
 		}
 		if (res->virtual_field == 0) {
-			*p->error = NULL;
-			asprintf (p->error, "Field `%s' does not support "
-				  "value comparison",
-				  res->v.p.field.name);
+			if (asprintf(p->error, "Field `%s' does not support "
+				     "value comparison",
+				     res->v.p.field.name) < 0)
+				*p->error = NULL;
 			goto err_field;
 		} else {
 			if (parse_virtual_field_value(res, p) != 0)
@@ -576,9 +578,9 @@ parse_comparison(struct parsing *p)
 		break;
 
 	default:
-		*p->error = NULL;
-		asprintf(p->error, "Operator expected, got `%.*s'",
-			 p->token_len, p->token_start);
+		if (asprintf(p->error, "Operator expected, got `%.*s'",
+			     p->token_len, p->token_start) < 0)
+			*p->error = NULL;
 		goto err_field;
 	}
 	return res;
@@ -624,9 +626,10 @@ parse_primary(struct parsing *p)
 		if (e == NULL)
 			return NULL;
 		if (p->token != T_RIGHT_PAREN) {
-			*p->error = NULL;
-			asprintf(p->error, "Right paren expected, got `%.*s'",
-				 p->token_len, p->token_start);
+			if (asprintf(p->error,
+				     "Right paren expected, got `%.*s'",
+				     p->token_len, p->token_start) < 0)
+				*p->error = NULL;
 			goto err_e;
 		}
 		if (lex(p) != 0)
@@ -638,9 +641,9 @@ parse_primary(struct parsing *p)
 		return parse_comparison(p);
 
 	default:
-		*p->error = NULL;
-		asprintf(p->error, "Unexpected token `%.*s'", p->token_len,
-			 p->token_start);
+		if (asprintf(p->error, "Unexpected token `%.*s'", p->token_len,
+			     p->token_start) < 0)
+			*p->error = NULL;
 		return NULL;
 	}
 err_e:
@@ -744,9 +747,9 @@ expr_parse(const char *string, char **error)
 	res = parse_or(&p);
 	if (res != NULL && p.token != T_EOF) {
 		expr_free(res);
-		*error = NULL;
-		asprintf(error, "Unexpected trailing token `%.*s'",
-			 p.token_len, p.token_start);
+		if (asprintf(error, "Unexpected trailing token `%.*s'",
+			     p.token_len, p.token_start) < 0)
+			*error = NULL;
 		goto err;
 	}
 	free(p.token_value);
diff --git a/auparse/interpret.c b/auparse/interpret.c
index 9bb4f84..334ec4c 100644
--- a/auparse/interpret.c
+++ b/auparse/interpret.c
@@ -292,7 +292,8 @@ static const char *print_uid(const char *val, unsigned int base)
         uid = strtoul(val, NULL, base);
         if (errno) {
 		char *out;
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -308,7 +309,8 @@ static const char *print_gid(const char *val, unsigned int base)
         gid = strtoul(val, NULL, base);
         if (errno) {
 		char *out;
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -321,14 +323,16 @@ static const char *print_arch(const char *val, int machine)
 	char *out;
 
         if (machine < 0) {
-                asprintf(&out, "unknown elf type(%s)", val);
+		if (asprintf(&out, "unknown elf type(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
         ptr = audit_machine_to_name(machine);
 	if (ptr)
 	        return strdup(ptr);
 	else {
-                asprintf(&out, "unknown machine type(%d)", machine);
+		if (asprintf(&out, "unknown machine type(%d)", machine) < 0)
+			out = NULL;
                 return out;
 	}
 }
@@ -355,13 +359,15 @@ static const char *print_syscall(const char *val, const rnode *r)
                 } else if (strcmp(sys, "ipc") == 0)
 			if ((int)a0 == a0)
 				func = ipc_i2s(a0);
-                if (func)
-                        asprintf(&out, "%s(%s)", sys, func);
-                else
+                if (func) {
+			if (asprintf(&out, "%s(%s)", sys, func) < 0)
+				out = NULL;
+		} else
                         return strdup(sys);
-        }
-        else
-                asprintf(&out, "unknown syscall(%d)", syscall);
+        } else {
+		if (asprintf(&out, "unknown syscall(%d)", syscall) < 0)
+			out = NULL;
+	}
 
 	return out;
 }
@@ -374,12 +380,14 @@ static const char *print_exit(const char *val)
         errno = 0;
         ival = strtol(val, NULL, 10);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
         if (ival < 0) {
-                asprintf(&out, "%d(%s)", ival, strerror(-ival));
+		if (asprintf(&out, "%d(%s)", ival, strerror(-ival)) < 0)
+			out = NULL;
 		return out;
         }
         return strdup(val);
@@ -428,7 +436,8 @@ static const char *print_perm(const char *val)
         ival = strtol(val, NULL, 10);
         if (errno) {
 		char *out;
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -473,7 +482,8 @@ static const char *print_mode(const char *val, unsigned int base)
         errno = 0;
         ival = strtoul(val, NULL, base);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -498,7 +508,9 @@ static const char *print_mode(const char *val, unsigned int base)
                 strcat(buf, ",sticky");
 
 	// and the read, write, execute flags in octal
-        asprintf(&out, "%s,%03o",  buf, (S_IRWXU|S_IRWXG|S_IRWXO) & ival);
+	if (asprintf(&out, "%s,%03o", buf,
+		     (S_IRWXU|S_IRWXG|S_IRWXO) & ival) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -510,7 +522,8 @@ static const char *print_mode_short(const char *val)
         errno = 0;
         ival = strtoul(val, NULL, 16);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -525,9 +538,13 @@ static const char *print_mode_short(const char *val)
 
 	// and the read, write, execute flags in octal
 	if (buf[0] == 0)
-	        asprintf(&out, "0%03o", (S_IRWXU|S_IRWXG|S_IRWXO) & ival);
+		if (asprintf(&out, "0%03o",
+			     (S_IRWXU|S_IRWXG|S_IRWXO) & ival) < 0)
+			out = NULL;
 	else
-	        asprintf(&out, "%s,%03o", buf, (S_IRWXU|S_IRWXG|S_IRWXO)&ival);
+		if (asprintf(&out, "%s,%03o", buf,
+			     (S_IRWXU|S_IRWXG|S_IRWXO) & ival) < 0)
+			out = NULL;
 	return out;
 }
 
@@ -540,12 +557,14 @@ static const char *print_socket_domain(const char *val)
 	errno = 0;
         i = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
         str = fam_i2s(i);
         if (str == NULL) {
-                asprintf(&out, "unknown family(%s)", val);
+		if (asprintf(&out, "unknown family(%s)", val) < 0)
+			out = NULL;
 		return out;
 	} else
 		return strdup(str);
@@ -560,12 +579,14 @@ static const char *print_socket_type(const char *val)
 	errno = 0;
         type = 0xFF & strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
         str = sock_type_i2s(type);
         if (str == NULL) {
-                asprintf(&out, "unknown type(%s)", val);
+		if (asprintf(&out, "unknown type(%s)", val) < 0)
+			out = NULL;
 		return out;
 	} else
 		return strdup(str);
@@ -580,12 +601,14 @@ static const char *print_socket_proto(const char *val)
 	errno = 0;
         proto = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
         p = getprotobynumber(proto);
         if (p == NULL) {
-                asprintf(&out, "unknown proto(%s)", val);
+		if (asprintf(&out, "unknown proto(%s)", val) < 0)
+			out = NULL;
 		return out;
 	} else
 		return strdup(p->p_name);
@@ -593,7 +616,7 @@ static const char *print_socket_proto(const char *val)
 
 static const char *print_sockaddr(const char *val)
 {
-        int slen;
+        int slen, rc = 0;
         const struct sockaddr *saddr;
         char name[NI_MAXHOST], serv[NI_MAXSERV];
         const char *host;
@@ -603,7 +626,8 @@ static const char *print_sockaddr(const char *val)
         slen = strlen(val)/2;
         host = au_unescape((char *)val);
 	if (host == NULL) {
-                asprintf(&out, "malformed host(%s)", val);
+		if (asprintf(&out, "malformed host(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
         saddr = (struct sockaddr *)host;
@@ -611,7 +635,8 @@ static const char *print_sockaddr(const char *val)
 
         str = fam_i2s(saddr->sa_family);
         if (str == NULL) {
-                asprintf(&out, "unknown family(%d)", saddr->sa_family);
+		if (asprintf(&out, "unknown family(%d)", saddr->sa_family) < 0)
+			out = NULL;
 		return out;
 	}
 
@@ -622,94 +647,96 @@ static const char *print_sockaddr(const char *val)
                                 const struct sockaddr_un *un =
                                         (struct sockaddr_un *)saddr;
                                 if (un->sun_path[0])
-                                        asprintf(&out, "%s %s", str,
-							un->sun_path);
+					rc = asprintf(&out, "%s %s", str,
+						      un->sun_path);
                                 else // abstract name
-                                        asprintf(&out, "%s %.108s", str,
-							&un->sun_path[1]);
+					rc = asprintf(&out, "%s %.108s", str,
+						      &un->sun_path[1]);
                         }
                         break;
                 case AF_INET:
                         if (slen < sizeof(struct sockaddr_in)) {
-                                asprintf(&out, "%s sockaddr len too short",
-						 str);
-                                free((char *)host);
-                                return out;
+				rc = asprintf(&out, "%s sockaddr len too short",
+					      str);
+				break;
                         }
                         slen = sizeof(struct sockaddr_in);
                         if (getnameinfo(saddr, slen, name, NI_MAXHOST, serv,
                                 NI_MAXSERV, NI_NUMERICHOST |
                                         NI_NUMERICSERV) == 0 ) {
-                                asprintf(&out, "%s host:%s serv:%s", str,
-						name, serv);
+				rc = asprintf(&out, "%s host:%s serv:%s", str,
+					      name, serv);
                         } else
-                                asprintf(&out, "%s (error resolving addr)",
-						 str);
+				rc = asprintf(&out, "%s (error resolving addr)",
+					      str);
                         break;
                 case AF_AX25:
                         {
                                 const struct sockaddr_ax25 *x =
                                                 (struct sockaddr_ax25 *)saddr;
-                                asprintf(&out, "%s call:%c%c%c%c%c%c%c", str,
-                                        x->sax25_call.ax25_call[0],
-                                        x->sax25_call.ax25_call[1],
-                                        x->sax25_call.ax25_call[2],
-                                        x->sax25_call.ax25_call[3],
-                                        x->sax25_call.ax25_call[4],
-                                        x->sax25_call.ax25_call[5],
-                                        x->sax25_call.ax25_call[6]
-                                );
+				rc = asprintf(&out, "%s call:%c%c%c%c%c%c%c",
+					      str,
+					      x->sax25_call.ax25_call[0],
+					      x->sax25_call.ax25_call[1],
+					      x->sax25_call.ax25_call[2],
+					      x->sax25_call.ax25_call[3],
+					      x->sax25_call.ax25_call[4],
+					      x->sax25_call.ax25_call[5],
+					      x->sax25_call.ax25_call[6]);
                         }
                         break;
                 case AF_IPX:
                         {
                                 const struct sockaddr_ipx *ip =
                                                 (struct sockaddr_ipx *)saddr;
-                                asprintf(&out, "%s port:%d net:%u", str,
-                                        ip->sipx_port, ip->sipx_network);
+				rc = asprintf(&out, "%s port:%d net:%u", str,
+					      ip->sipx_port, ip->sipx_network);
                         }
                         break;
                 case AF_ATMPVC:
                         {
                                 const struct sockaddr_atmpvc* at =
                                         (struct sockaddr_atmpvc *)saddr;
-                                asprintf(&out, "%s int:%d", str, 
-						at->sap_addr.itf);
+				rc = asprintf(&out, "%s int:%d", str,
+					      at->sap_addr.itf);
                         }
                         break;
                 case AF_X25:
                         {
                                 const struct sockaddr_x25* x =
                                         (struct sockaddr_x25 *)saddr;
-                                asprintf(&out, "%s addr:%.15s", str,
-						x->sx25_addr.x25_addr);
+				rc = asprintf(&out, "%s addr:%.15s", str,
+					      x->sx25_addr.x25_addr);
                         }
                         break;
                 case AF_INET6:
                         if (slen < sizeof(struct sockaddr_in6)) {
-                                asprintf(&out, "%s sockaddr6 len too short", 
-						str);
-                                free((char *)host);
-                                return out;
+				rc = asprintf(&out,
+					      "%s sockaddr6 len too short",
+					      str);
+				break;
                         }
                         slen = sizeof(struct sockaddr_in6);
                         if (getnameinfo(saddr, slen, name, NI_MAXHOST, serv,
                                 NI_MAXSERV, NI_NUMERICHOST |
                                         NI_NUMERICSERV) == 0 ) {
-                                asprintf(&out, "%s host:%s serv:%s", str,
-						name, serv);
+				rc = asprintf(&out, "%s host:%s serv:%s", str,
+					      name, serv);
                         } else
-                                asprintf(&out, "%s (error resolving addr)",
-						str);
+				rc = asprintf(&out, "%s (error resolving addr)",
+					      str);
                         break;
                 case AF_NETLINK:
                         {
                                 const struct sockaddr_nl *n =
                                                 (struct sockaddr_nl *)saddr;
-                                asprintf(&out, "%s pid:%u", str, n->nl_pid);
+				rc = asprintf(&out, "%s pid:%u", str,
+					      n->nl_pid);
                         }
                         break;
         }
+	if (rc < 0)
+		out = NULL;
         free((char *)host);
 	return out;
 }
@@ -723,11 +750,13 @@ static const char *print_flags(const char *val)
         errno = 0;
         flags = strtoul(val, NULL, 16);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
         if (flags == 0) {
-                asprintf(&out, "none");
+		if (asprintf(&out, "none") < 0)
+			out = NULL;
                 return out;
         }
 	buf[0] = 0;
@@ -755,7 +784,8 @@ static const char *print_promiscuous(const char *val)
         ival = strtol(val, NULL, 10);
         if (errno) {
 		char *out;
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -774,14 +804,16 @@ static const char *print_capabilities(const char *val)
         errno = 0;
         cap = strtoul(val, NULL, 10);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
 	s = cap_i2s(cap);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown capability(%s)", val);
+	if (asprintf(&out, "unknown capability(%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -797,7 +829,8 @@ static const char *print_cap_bitmap(const char *val)
 	temp = strtoull(val, NULL, 16);
 	if (errno) {
 		char *out;
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
 	}
 
@@ -829,7 +862,8 @@ static const char *print_success(const char *val)
         	res = strtoul(val, NULL, 10);
 	        if (errno) {
 			char *out;
-                	asprintf(&out, "conversion error(%s)", val);
+			if (asprintf(&out, "conversion error(%s)", val) < 0)
+				out = NULL;
 	                return out;
         	}
 
@@ -848,7 +882,8 @@ static const char *print_open_flags(const char *val)
 	errno = 0;
 	flags = strtoul(val, NULL, 16);
         if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                	return out;
        	}
 
@@ -883,7 +918,8 @@ static const char *print_clone_flags(const char *val)
 	errno = 0;
 	flags = strtoul(val, NULL, 16);
         if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                	return out;
        	}
 
@@ -915,14 +951,16 @@ static const char *print_fcntl_cmd(const char *val)
 	errno = 0;
 	cmd = strtoul(val, NULL, 16);
         if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
        	}
 
 	s = fcntl_i2s(cmd);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown fcntl command(%d)", cmd);
+	if (asprintf(&out, "unknown fcntl command(%d)", cmd) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -935,14 +973,16 @@ static const char *print_epoll_ctl(const char *val)
 	errno = 0;
 	cmd = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 
 	s = epoll_ctl_i2s(cmd);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown epoll_ctl operation (%d)", cmd);
+	if (asprintf(&out, "unknown epoll_ctl operation (%d)", cmd) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -954,7 +994,8 @@ static const char *print_clock_id(const char *val)
 	errno = 0;
         i = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	else if (i < 7) {
@@ -962,7 +1003,8 @@ static const char *print_clock_id(const char *val)
 		if (s != NULL)
 			return strdup(s);
 	}
-	asprintf(&out, "unknown clk_id (%s)", val);
+	if (asprintf(&out, "unknown clk_id (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -976,7 +1018,8 @@ static const char *print_prot(const char *val, unsigned int is_mmap)
 	errno = 0;
         prot = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	buf[0] = 0;
@@ -1015,7 +1058,8 @@ static const char *print_mmap(const char *val)
 	errno = 0;
         maps = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	buf[0] = 0;
@@ -1049,7 +1093,8 @@ static const char *print_personality(const char *val)
         errno = 0;
         pers = strtoul(val, NULL, 16);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
@@ -1057,12 +1102,14 @@ static const char *print_personality(const char *val)
 	s = person_i2s(pers2);
 	if (s != NULL) {
 		if (pers & ADDR_NO_RANDOMIZE) {
-			asprintf(&out, "%s|~ADDR_NO_RANDOMIZE", s);
+			if (asprintf(&out, "%s|~ADDR_NO_RANDOMIZE", s) < 0)
+				out = NULL;
 			return out;
 		} else
 			return strdup(s);
 	}
-	asprintf(&out, "unknown personality (%s)", val);
+	if (asprintf(&out, "unknown personality (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -1075,14 +1122,16 @@ static const char *print_ptrace(const char *val)
         errno = 0;
         trace = strtoul(val, NULL, 16);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
 	s = ptrace_i2s(trace);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown ptrace (%s)", val);
+	if (asprintf(&out, "unknown ptrace (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -1096,7 +1145,8 @@ static const char *print_mount(const char *val)
 	errno = 0;
         mounts = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	buf[0] = 0;
@@ -1124,7 +1174,8 @@ static const char *print_rlimit(const char *val)
 	errno = 0;
         i = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	else if (i < 17) {
@@ -1132,7 +1183,8 @@ static const char *print_rlimit(const char *val)
 		if (s != NULL)
 			return strdup(s);
 	}
-	asprintf(&out, "unknown rlimit (%s)", val);
+	if (asprintf(&out, "unknown rlimit (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -1146,7 +1198,8 @@ static const char *print_recv(const char *val)
 	errno = 0;
         rec = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	buf[0] = 0;
@@ -1258,7 +1311,9 @@ static const char *print_a2(const char *val, const rnode *r)
 			errno = 0;
 			ival = strtoul(val, NULL, 16);
 		        if (errno) {
-                		asprintf(&out, "conversion error(%s)", val);
+				if (asprintf(&out, "conversion error(%s)",
+					     val) < 0)
+					out = NULL;
 	                	return out;
 	        	}
 			switch (r->a1)
@@ -1330,7 +1385,8 @@ static const char *print_signals(const char *val, unsigned int base)
 	errno = 0;
         i = strtoul(val, NULL, base);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	else if (i < 32) {
@@ -1338,7 +1394,8 @@ static const char *print_signals(const char *val, unsigned int base)
 		if (s != NULL)
 			return strdup(s);
 	}
-	asprintf(&out, "unknown signal (%s)", val);
+	if (asprintf(&out, "unknown signal (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -1351,14 +1408,16 @@ static const char *print_nfproto(const char *val)
         errno = 0;
         proto = strtoul(val, NULL, 10);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
 	s = nfproto_i2s(proto);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown netfilter protocol (%s)", val);
+	if (asprintf(&out, "unknown netfilter protocol (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -1371,14 +1430,16 @@ static const char *print_icmptype(const char *val)
         errno = 0;
         icmptype = strtoul(val, NULL, 10);
         if (errno) {
-                asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
                 return out;
         }
 
 	s = icmptype_i2s(icmptype);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown icmp type (%s)", val);
+	if (asprintf(&out, "unknown icmp type (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
@@ -1390,7 +1451,8 @@ static const char *print_protocol(const char *val)
 	errno = 0;
         i = strtoul(val, NULL, 10);
 	if (errno) 
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 	else {
 		struct protoent *p = getprotobynumber(i);
 		if (p)
@@ -1415,7 +1477,8 @@ static const char *print_list(const char *val)
 	errno = 0;
         i = strtoul(val, NULL, 10);
 	if (errno) 
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 	else
 		out = strdup(audit_flag_to_name(i));
 	return out;
@@ -1569,13 +1632,15 @@ static const char *print_seccomp_code(const char *val)
 	errno = 0;
         code = strtoul(val, NULL, 16);
 	if (errno) {
-		asprintf(&out, "conversion error(%s)", val);
+		if (asprintf(&out, "conversion error(%s)", val) < 0)
+			out = NULL;
 		return out;
 	}
 	s = seccomp_i2s(code & SECCOMP_RET_ACTION);
 	if (s != NULL)
 		return strdup(s);
-	asprintf(&out, "unknown seccomp code (%s)", val);
+	if (asprintf(&out, "unknown seccomp code (%s)", val) < 0)
+		out = NULL;
 	return out;
 }
 
diff --git a/lib/audit_logging.c b/lib/audit_logging.c
index 4b59e30..85d1338 100644
--- a/lib/audit_logging.c
+++ b/lib/audit_logging.c
@@ -133,12 +133,14 @@ char *audit_encode_nv_string(const char *name, const char *value,
 		char *tmp = malloc(2*vlen + 1);
 		if (tmp) {
 			audit_encode_value(tmp, value, vlen);
-			asprintf(&str, "%s=%s", name, tmp);
+			if (asprintf(&str, "%s=%s", name, tmp) < 0)
+				str = NULL;
 			free(tmp);
 		} else
 			str = NULL;
 	} else
-		asprintf(&str, "%s=\"%s\"", name, value ? value : "?");
+		if (asprintf(&str, "%s=\"%s\"", name, value ? value : "?") < 0)
+			str = NULL;
 	return str;
 }
 
diff --git a/src/auditctl.c b/src/auditctl.c
index 62a8c51..b062e8b 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -930,16 +930,16 @@ static int setopt(int count, int lineno, char *vars[])
 		flags = del & AUDIT_FILTER_MASK;
 
 	/* Build the command */
-	asprintf(&cmd, "key=%s", key);
-	if (cmd) {
+	if (asprintf(&cmd, "key=%s", key) < 0) {
+		cmd = NULL;
+		fprintf(stderr, "Out of memory adding key\n");
+		retval = -1;
+	} else {
 		/* Add this to the rule */
 		int ret = audit_rule_fieldpair_data(&rule_new, cmd, flags);
 		if (ret < 0)
 			retval = -1;
 		free(cmd);
-	} else {
-		fprintf(stderr, "Out of memory adding key\n");
-		retval = -1;
 	}
     }
     if (retval == -1 && errno == ECONNREFUSED)
@@ -1351,8 +1351,9 @@ int key_match(struct audit_reply *rep)
 		int field = rep->ruledata->fields[i] & ~AUDIT_OPERATORS;
 		if (field == AUDIT_FILTERKEY) {
 			char *keyptr;
-			asprintf(&keyptr, "%.*s", rep->ruledata->values[i],
-				&rep->ruledata->buf[boffset]);
+			if (asprintf(&keyptr, "%.*s", rep->ruledata->values[i],
+				     &rep->ruledata->buf[boffset]) < 0)
+				keyptr = NULL;
 			if (strstr(keyptr, key)) {
 				free(keyptr);
 				return 1;
@@ -1467,9 +1468,10 @@ static int audit_print_reply(struct audit_reply *rep)
 						    rep->ruledata->values[i];
 					} else if (field == AUDIT_FILTERKEY) {
 						char *rkey, *ptr;
-						asprintf(&rkey, "%.*s",
-						rep->ruledata->values[i],
-						&rep->ruledata->buf[boffset]);
+						if (asprintf(&rkey, "%.*s",
+						      rep->ruledata->values[i],
+						      &rep->ruledata->buf[boffset]) < 0)
+							rkey = NULL;
 						boffset +=
 						    rep->ruledata->values[i];
 						ptr = strtok(rkey, key_sep);
-- 
1.7.10.4

Re: [PATCH 1/3] Don't ignore the return value of asprintf()

[Steve Grubb]

On Friday, February 08, 2013 07:12:33 PM Tyler Hicks wrote:
> If an error occurs in asprintf(), the contents of the strp variable are
> undefined. asprintf()'s return value must be checked and the parameter
> passed into asprintf() should be set to NULL upon error.

Applied. I think there are a couple places where it will obviously segfault in 
auditctl.c. But the patch does clean up a bunch of warnings.

-Steve

[PATCH 2/3] Fix Wunused-return warnings

[Tyler Hicks]

When building with -D_FORTIFY_SOURCE=2 and -W-unused-return, there are a
number of warnings caused by return values of functions marked with the
warn_unused_result attribute being ignored. The audit codebase makes an
attempt to suppress these warnings by casting the return value to void, but
that does not work when D_FORITY_SOURCE is in use.

Here's an explanation of how this patch fixes the warnings and how the
potential error conditions are handled:

Errors writing to the auditd pid file should be logged since errors opening
the pid file are logged. These write() errors aren't treated as fatal.

Problems adjusting auditd's out of memory score should be logged, if simply
to catch a change to the kernel interface. These errors aren't treated as
fatal.

Auditd refuses to start when nice() fails during initialization, so it should
take disk_error_action whenever nice() fails during a reconfigure.

Failure to chdir("/") while daemonizing should be logged and treated as fatal
since errors while redirecting stdin, stdout, and stderr are logged and
considered fatal.

All nice() return values are handled sufficiently by relying on errno.
However, they still throw warnings when D_FORTIFY_SOURCE is used. This patch
quiets those warnings by capturing the return value and using it and errno to
determine if nice() failed.

Failure to adjust audit log file owner (fchown) and permissions (fchmod) are
logged and considered fatal when opening the log file for the first time. They
are not treated as fatal when the operations fail on during log rotation since
we made sure that they file owner and permissions were correct when originally
opening the log file.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 audisp/audispd.c   |    6 ++++--
 src/auditd-event.c |   45 +++++++++++++++++++++++++++++++++++++--------
 src/auditd.c       |   52 +++++++++++++++++++++++++++++++++-------------------
 src/autrace.c      |    6 +++++-
 4 files changed, 79 insertions(+), 30 deletions(-)

diff --git a/audisp/audispd.c b/audisp/audispd.c
index 749128f..0ef4b8e 100644
--- a/audisp/audispd.c
+++ b/audisp/audispd.c
@@ -369,9 +369,11 @@ int main(int argc, char *argv[])
 
 	/* Now boost priority to make sure we are getting time slices */
 	if (daemon_config.priority_boost != 0) {
+		int rc;
+
 		errno = 0;
-		(void) nice((int)-daemon_config.priority_boost);
-		if (errno) {
+		rc = nice((int)-daemon_config.priority_boost);
+		if (rc == -1 && errno) {
 			syslog(LOG_ERR, "Cannot change priority (%s)",
 					strerror(errno));
 			/* Stay alive as this is better than stopping */
diff --git a/src/auditd-event.c b/src/auditd-event.c
index acf5aa1..2965be3 100644
--- a/src/auditd-event.c
+++ b/src/auditd-event.c
@@ -708,10 +708,18 @@ static void rotate_logs(struct auditd_consumer_data *data,
 	if (data->config->num_logs < 2)
 		return;
 
-	/* Close audit file */
-	fchmod(data->log_fd, 
-			data->config->log_group ? S_IRUSR|S_IRGRP : S_IRUSR);
-	fchown(data->log_fd, 0, data->config->log_group);
+	/* Close audit file. fchmod and fchown errors are not fatal because we
+	 * already adjusted log file permissions and ownership when opening the
+	 * log file. */
+	if (fchmod(data->log_fd, data->config->log_group ? S_IRUSR|S_IRGRP :
+								S_IRUSR) < 0) {
+		audit_msg(LOG_NOTICE, "Couldn't change permissions while "
+			"rotating log file (%s)", strerror(errno));
+	}
+	if (fchown(data->log_fd, 0, data->config->log_group) < 0) {
+		audit_msg(LOG_NOTICE, "Couldn't change ownership while "
+			"rotating log file (%s)", strerror(errno));
+	}
 	fclose(data->log_file);
 	
 	/* Rotate */
@@ -924,9 +932,20 @@ retry:
 			return 1;
 		}
 	}
-	fchmod(lfd, data->config->log_group ? S_IRUSR|S_IWUSR|S_IRGRP : 
-						S_IRUSR|S_IWUSR);
-	fchown(lfd, 0, data->config->log_group);
+	if (fchmod(lfd, data->config->log_group ? S_IRUSR|S_IWUSR|S_IRGRP :
+							S_IRUSR|S_IWUSR) < 0) {
+		audit_msg(LOG_ERR,
+			"Couldn't change permissions of log file (%s)",
+			strerror(errno));
+		close(lfd);
+		return 1;
+	}
+	if (fchown(lfd, 0, data->config->log_group) < 0) {
+		audit_msg(LOG_ERR, "Couldn't change ownership of log file (%s)",
+			strerror(errno));
+		close(lfd);
+		return 1;
+	}
 
 	data->log_fd = lfd;
 	data->log_file = fdopen(lfd, "a");
@@ -1089,8 +1108,18 @@ static void reconfigure(struct auditd_consumer_data *data)
 
 	// priority boost
 	if (oconf->priority_boost != nconf->priority_boost) {
+		int rc;
+
 		oconf->priority_boost = nconf->priority_boost;
-		nice(-oconf->priority_boost);
+		errno = 0;
+		rc = nice(-oconf->priority_boost);
+		if (rc == -1 && errno) {
+			int saved_errno = errno;
+			audit_msg(LOG_NOTICE, "Cannot change priority in "
+					"reconfigure (%s)", strerror(errno));
+			do_disk_error_action("reconfig", data->config,
+						saved_errno);
+		}
 	}
 
 	// log format
diff --git a/src/auditd.c b/src/auditd.c
index 84fdea2..52ec5df 100644
--- a/src/auditd.c
+++ b/src/auditd.c
@@ -240,7 +240,7 @@ int send_audit_event(int type, const char *str)
 
 static int write_pid_file(void)
 {
-	int pidfd, len;
+	int pidfd, len, rc;
 	char val[16];
 
 	len = snprintf(val, sizeof(val), "%u\n", getpid());
@@ -256,29 +256,38 @@ static int write_pid_file(void)
 		pidfile = 0;
 		return 1;
 	}
-	(void)write(pidfd, val, (unsigned int)len);
+	if (write(pidfd, val, (unsigned int)len) != len) {
+		audit_msg(LOG_ERR, "Unable to write pidfile (%s)",
+			strerror(errno));
+		close(pidfd);
+		pidfile = 0;
+		return 1;
+	}
 	close(pidfd);
 	return 0;
 }
 
 static void avoid_oom_killer(void)
 {
-	int oomfd;
+	int oomfd, len, rc;
+	char *score = NULL;
 
 	/* New kernels use different technique */	
-	oomfd = open("/proc/self/oom_score_adj", O_NOFOLLOW | O_WRONLY);
-	if (oomfd >= 0) {
-		(void)write(oomfd, "-1000", 5);
-		close(oomfd);
-		return;
-	}
-	oomfd = open("/proc/self/oom_adj", O_NOFOLLOW | O_WRONLY);
-	if (oomfd >= 0) {
-		(void)write(oomfd, "-17", 3);
-		close(oomfd);
-		return;
-	}
-	// Old style kernel...perform another action here
+	if ((oomfd = open("/proc/self/oom_score_adj",
+				O_NOFOLLOW | O_WRONLY)) >= 0) {
+		score = "-1000";
+	} else if ((oomfd = open("/proc/self/oom_adj",
+				O_NOFOLLOW | O_WRONLY)) >= 0) {
+		score = "-17";
+	} else
+		audit_msg(LOG_NOTICE, "Cannot open out of memory adjuster");
+
+	len = strlen(score);
+	rc = write(oomfd, score, len);
+	if (rc != len)
+		audit_msg(LOG_NOTICE, "Unable to adjust out of memory score");
+
+	close(oomfd);
 }
 
 /*
@@ -328,7 +337,12 @@ static int become_daemon(void)
 			close(fd);
 
 			/* Change to '/' */
-			chdir("/");
+			rc = chdir("/");
+			if (rc < 0) {
+				audit_msg(LOG_ERR,
+					"Cannot change working directory to /");
+				return -1;
+			}
 
 			/* Become session/process group leader */
 			setsid();
@@ -540,8 +554,8 @@ int main(int argc, char *argv[])
 
 	if (config.priority_boost != 0) {
 		errno = 0;
-		(void) nice((int)-config.priority_boost);
-		if (errno) {
+		rc = nice((int)-config.priority_boost);
+		if (rc == -1 && errno) {
 			audit_msg(LOG_ERR, "Cannot change priority (%s)", 
 					strerror(errno));
 			return 1;
diff --git a/src/autrace.c b/src/autrace.c
index e124660..47c6c4f 100644
--- a/src/autrace.c
+++ b/src/autrace.c
@@ -245,7 +245,11 @@ int main(int argc, char *argv[])
 					exit(1);
 				}
 				sleep(1);
-				(void)write(fd[1],"1", 1);
+				if (write(fd[1],"1", 1) != 1) {
+					kill(pid,SIGTERM);
+					(void)delete_all_rules(audit_fd);
+					exit(1);
+				}
 				waitpid(pid, NULL, 0);
 				close(fd[1]);
 				puts("Cleaning up...");
-- 
1.7.10.4

Re: [PATCH 2/3] Fix Wunused-return warnings

[Steve Grubb]

On Friday, February 08, 2013 07:12:34 PM Tyler Hicks wrote:
> When building with -D_FORTIFY_SOURCE=2 and -W-unused-return, there are a
> number of warnings caused by return values of functions marked with the
> warn_unused_result attribute being ignored. The audit codebase makes an
> attempt to suppress these warnings by casting the return value to void, but
> that does not work when D_FORITY_SOURCE is in use.
> 
> Here's an explanation of how this patch fixes the warnings and how the
> potential error conditions are handled:
> 
> Errors writing to the auditd pid file should be logged since errors opening
> the pid file are logged. These write() errors aren't treated as fatal.
> 
> Problems adjusting auditd's out of memory score should be logged, if simply
> to catch a change to the kernel interface. These errors aren't treated as
> fatal.
> 
> Auditd refuses to start when nice() fails during initialization, so it
> should take disk_error_action whenever nice() fails during a reconfigure.

During a reconfigure, I would not consider this fatal. Its better to stay 
running than exit. I'll adjust the patch.

-Steve

> Failure to chdir("/") while daemonizing should be logged and treated as
> fatal since errors while redirecting stdin, stdout, and stderr are logged
> and considered fatal.
>
> All nice() return values are handled sufficiently by relying on errno.
> However, they still throw warnings when D_FORTIFY_SOURCE is used. This patch
> quiets those warnings by capturing the return value and using it and errno
> to determine if nice() failed.
> 
> Failure to adjust audit log file owner (fchown) and permissions (fchmod) are
> logged and considered fatal when opening the log file for the first time.
> They are not treated as fatal when the operations fail on during log
> rotation since we made sure that they file owner and permissions were
> correct when originally opening the log file.

Re: [PATCH 2/3] Fix Wunused-return warnings

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 2166 bytes --]

On 2013-02-09 11:57:32, Steve Grubb wrote:
> On Friday, February 08, 2013 07:12:34 PM Tyler Hicks wrote:
> > When building with -D_FORTIFY_SOURCE=2 and -W-unused-return, there are a
> > number of warnings caused by return values of functions marked with the
> > warn_unused_result attribute being ignored. The audit codebase makes an
> > attempt to suppress these warnings by casting the return value to void, but
> > that does not work when D_FORITY_SOURCE is in use.
> > 
> > Here's an explanation of how this patch fixes the warnings and how the
> > potential error conditions are handled:
> > 
> > Errors writing to the auditd pid file should be logged since errors opening
> > the pid file are logged. These write() errors aren't treated as fatal.
> > 
> > Problems adjusting auditd's out of memory score should be logged, if simply
> > to catch a change to the kernel interface. These errors aren't treated as
> > fatal.
> > 
> > Auditd refuses to start when nice() fails during initialization, so it
> > should take disk_error_action whenever nice() fails during a reconfigure.
> 
> During a reconfigure, I would not consider this fatal. Its better to stay 
> running than exit. I'll adjust the patch.

That's probably the best way to go.

Thanks for getting these applied.

Tyler

> 
> -Steve
> 
> > Failure to chdir("/") while daemonizing should be logged and treated as
> > fatal since errors while redirecting stdin, stdout, and stderr are logged
> > and considered fatal.
> >
> > All nice() return values are handled sufficiently by relying on errno.
> > However, they still throw warnings when D_FORTIFY_SOURCE is used. This patch
> > quiets those warnings by capturing the return value and using it and errno
> > to determine if nice() failed.
> > 
> > Failure to adjust audit log file owner (fchown) and permissions (fchmod) are
> > logged and considered fatal when opening the log file for the first time.
> > They are not treated as fatal when the operations fail on during log
> > rotation since we made sure that they file owner and permissions were
> > correct when originally opening the log file.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 3/3] Fix discards 'const' qualifier from pointer target type warnings

[Tyler Hicks]

The event_note_list pointer is reassigned and its members are also
reassigned. It should not be declared with the const qualifier.

The ptr variable, in unescape(), cannot be used to modify a string since
it is initialized to the const char *buf input parameter. Rather than
modifying buf, we can use ptr as a placeholder and use strndup() to
allocate str. Later in the function a new, non-const pointer is used to
modify str. These changes allow unescape() to still take a const char *
as its input parameter.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 src/aureport-options.c |    2 +-
 src/ausearch-common.h  |    2 +-
 src/ausearch-lookup.c  |   16 +++++++---------
 src/ausearch-options.c |    2 +-
 4 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/aureport-options.c b/src/aureport-options.c
index 72a1d15..2af8714 100644
--- a/src/aureport-options.c
+++ b/src/aureport-options.c
@@ -42,7 +42,7 @@ int force_logs = 0;
 
 /* These are for compatibility with parser */
 unsigned int event_id = -1;
-const slist *event_node_list = NULL;
+slist *event_node_list = NULL;
 const char *event_key = NULL;
 const char *event_filename = NULL;
 const char *event_exe = NULL;
diff --git a/src/ausearch-common.h b/src/ausearch-common.h
index 2ee1f33..95c9980 100644
--- a/src/ausearch-common.h
+++ b/src/ausearch-common.h
@@ -35,7 +35,7 @@ extern gid_t event_gid, event_egid;
 extern pid_t event_pid;
 extern int event_exact_match;
 extern uid_t event_uid, event_euid, event_loginuid;
-const slist *event_node_list;
+slist *event_node_list;
 extern const char *event_comm;
 extern const char *event_filename;
 extern const char *event_hostname;
diff --git a/src/ausearch-lookup.c b/src/ausearch-lookup.c
index cb13003..0c8f4bf 100644
--- a/src/ausearch-lookup.c
+++ b/src/ausearch-lookup.c
@@ -318,7 +318,8 @@ static unsigned char x2c(unsigned char *buf)
 char *unescape(const char *buf)
 {
 	int len, i;
-	char saved, *ptr = buf, *str;
+	char *str, *strptr;
+	const char *ptr = buf;
 
 	/* Find the end of the name */
 	if (*ptr == '(') {
@@ -331,10 +332,7 @@ char *unescape(const char *buf)
 		while (isxdigit(*ptr))
 			ptr++;
 	}
-	saved = *ptr;
-	*ptr = 0;
-	str = strdup(buf);
-	*ptr = saved;
+	str = strndup(buf, ptr - buf);
 
 	if (*buf == '(')
 		return str;
@@ -347,12 +345,12 @@ char *unescape(const char *buf)
 		free(str);
 		return NULL;
 	}
-	ptr = str;
+	strptr = str;
 	for (i=0; i<len; i+=2) {
-		*ptr = x2c((unsigned char *)&str[i]);
-		ptr++;
+		*strptr = x2c((unsigned char *)&str[i]);
+		strptr++;
 	}
-	*ptr = 0;
+	*strptr = 0;
 	return str;
 }
 
diff --git a/src/ausearch-options.c b/src/ausearch-options.c
index 769d8fa..06a85bb 100644
--- a/src/ausearch-options.c
+++ b/src/ausearch-options.c
@@ -68,7 +68,7 @@ const char *event_vmname = NULL;
 report_t report_format = RPT_DEFAULT;
 ilist *event_type;
 
-const slist *event_node_list = NULL;
+slist *event_node_list = NULL;
 
 struct nv_pair {
     int        value;
-- 
1.7.10.4

Re: [PATCH 3/3] Fix discards 'const' qualifier from pointer target type warnings

[Steve Grubb]

On Friday, February 08, 2013 07:12:35 PM Tyler Hicks wrote:
> The event_note_list pointer is reassigned and its members are also
> reassigned. It should not be declared with the const qualifier.
> 
> The ptr variable, in unescape(), cannot be used to modify a string since
> it is initialized to the const char *buf input parameter. Rather than
> modifying buf, we can use ptr as a placeholder and use strndup() to
> allocate str. Later in the function a new, non-const pointer is used to
> modify str. These changes allow unescape() to still take a const char *
> as its input parameter.
> 
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

Applied. Thanks for all 3 patches.

-Steve