[ulogd2 PATCH 00/26] Compiler Warning Fixes

[ulogd2 PATCH 00/26] Compiler Warning Fixes

[Jeremy Sowden]

Patch 1 adds the `format` GCC attribute to ulogd_log.
Patches 2-5 fix the format errors revealed by the patch 1.
Patches 6-8 fix fall-through warnings.
Patches 9-10 are flow-control improvements related to patch 8.
Patch 11 replaces malloc+memset with calloc.
Patches 12-14 fix string-truncation warnings.
Patch 15 fixes a possible unaligned pointer access.
Patch 16 fixes DBI deprecation warnings.
Patches 17-20 fix more truncation warnings.
Patch 21 adds error-checking to sqlite SQL preparation.
Patches 22-26 fix more truncation and format warnings.

Jeremy Sowden (26):
  include: add format attribute to __ulogd_log declaration
  ulog: remove empty log-line
  ulog: fix order of log arguments
  ulog: correct log specifiers
  output: IPFIX: correct format-specifiers.
  jhash: add "fall through" comments to switch cases.
  db: add missing `break` to switch-case.
  filter: HWHDR: replace `switch` with `if`.
  filter: HWHDR: re-order KEY_RAW_MAC checks.
  filter: HWHDR: remove zero-initialization of MAC type.
  Replace malloc+memset with calloc
  filter: PWSNIFF: replace malloc+strncpy with strndup.
  input: UNIXSOCK: stat socket-path first before creating the socket.
  input: UNIXSOCK: fix possible truncation of socket path
  input: UNIXSOCK: prevent unaligned pointer access.
  output: DBI: fix deprecation warnings.
  output: DBI: fix string truncation warnings
  output: MYSQL: Fix string truncation warning
  output: PGSQL: Fix string truncation warning
  output: SQLITE3: Fix string truncation warnings and possible buffer
    overruns.
  output: SQLITE3: catch errors creating SQL statement
  util: db: fix possible string truncation.
  output: JSON: fix output of GMT offset.
  output: JSON: fix printf truncation warnings.
  output: JSON: optimize appending of newline to output.
  output: JSON: fix possible truncation of socket path

 filter/ulogd_filter_HWHDR.c           | 54 +++++++++++-------------
 filter/ulogd_filter_PWSNIFF.c         | 26 ++++++------
 include/ulogd/jhash.h                 | 24 +++++------
 include/ulogd/ulogd.h                 |  3 +-
 input/packet/ulogd_inppkt_UNIXSOCK.c  | 55 +++++++++++++-----------
 output/dbi/ulogd_output_DBI.c         | 60 ++++++++++++---------------
 output/ipfix/ipfix.c                  |  4 +-
 output/ipfix/ulogd_output_IPFIX.c     |  7 ++--
 output/mysql/ulogd_output_MYSQL.c     | 19 ++++-----
 output/pgsql/ulogd_output_PGSQL.c     | 19 ++++-----
 output/sqlite3/ulogd_output_SQLITE3.c | 58 +++++++++++++-------------
 output/ulogd_output_JSON.c            | 42 ++++++++++---------
 src/ulogd.c                           |  6 +--
 util/db.c                             | 24 +++++------
 14 files changed, 192 insertions(+), 209 deletions(-)

-- 
2.33.0

[ulogd2 PATCH 01/26] include: add format attribute to __ulogd_log declaration

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/ulogd/ulogd.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/ulogd/ulogd.h b/include/ulogd/ulogd.h
index 1636a8caa854..09f4a09cc56e 100644
--- a/include/ulogd/ulogd.h
+++ b/include/ulogd/ulogd.h
@@ -299,7 +299,8 @@ void ulogd_register_plugin(struct ulogd_plugin *me);
 /* allocate a new ulogd_key */
 struct ulogd_key *alloc_ret(const uint16_t type, const char*);
 
-/* write a message to the daemons' logfile */
+/* write a message to the daemon's logfile */
+__attribute__ ((format (printf, 4, 5)))
 void __ulogd_log(int level, char *file, int line, const char *message, ...);
 /* macro for logging including filename and line number */
 #define ulogd_log(level, format, args...) \
-- 
2.33.0

[ulogd2 PATCH 02/26] ulog: remove empty log-line

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/ulogd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index 9cd64e8e19b2..a31b35592a87 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -965,7 +965,6 @@ static int create_stack(const char *option)
 		load_all_plugins();
 
 	if (!buf) {
-		ulogd_log(ULOGD_ERROR, "");
 		ret = -ENOMEM;
 		goto out_buf;
 	}
-- 
2.33.0

[ulogd2 PATCH 03/26] ulog: fix order of log arguments

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/ulogd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index a31b35592a87..97da4fc0018f 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1569,7 +1569,7 @@ int main(int argc, char* argv[])
 	if (daemonize){
 		if (daemon(0, 0) < 0) {
 			ulogd_log(ULOGD_FATAL, "can't daemonize: %s (%d)\n",
-				  errno, strerror(errno));
+				  strerror(errno), errno);
 			warn_and_exit(daemonize);
 		}
 	}
-- 
2.33.0

[ulogd2 PATCH 04/26] ulog: correct log specifiers

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 input/packet/ulogd_inppkt_UNIXSOCK.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index 39944bf5cdb1..f97c2e174b2d 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 */
 
+#include <inttypes.h>
 #include <unistd.h>
 #include <stdlib.h>
 #include <netinet/ether.h>
@@ -633,7 +634,7 @@ static int unixsock_instance_read_cb(int fd, unsigned int what, void *param)
 		if (packet_sig != ULOGD_SOCKET_MARK) {
 			ulogd_log(ULOGD_ERROR,
 				"ulogd2: invalid packet marked received "
-				"(read %lx, expected %lx), closing socket.\n",
+				"(read %" PRIx32 ", expected %" PRIx32 "), closing socket.\n",
 				packet_sig, ULOGD_SOCKET_MARK);
 			_disconnect_client(ui);
 			return -1;
@@ -663,7 +664,7 @@ static int unixsock_instance_read_cb(int fd, unsigned int what, void *param)
 			}
 
 		} else {
-			ulogd_log(ULOGD_DEBUG, "  We have %d bytes, but need %d. Requesting more\n",
+			ulogd_log(ULOGD_DEBUG, "  We have %u bytes, but need %zu. Requesting more\n",
 					ui->unixsock_buf_avail, needed_len + sizeof(uint32_t));
 			return 0;
 		}
-- 
2.33.0

[ulogd2 PATCH 05/26] output: IPFIX: correct format-specifiers.

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/ipfix/ulogd_output_IPFIX.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/output/ipfix/ulogd_output_IPFIX.c b/output/ipfix/ulogd_output_IPFIX.c
index 5b5900363853..508d5f4974fc 100644
--- a/output/ipfix/ulogd_output_IPFIX.c
+++ b/output/ipfix/ulogd_output_IPFIX.c
@@ -198,7 +198,8 @@ static int send_msgs(struct ulogd_pluginstance *pi)
 	struct ipfix_priv *priv = (struct ipfix_priv *) &pi->private;
 	struct llist_head *curr, *tmp;
 	struct ipfix_msg *msg;
-	int ret = ULOGD_IRET_OK, sent;
+	int ret = ULOGD_IRET_OK;
+	ssize_t sent;
 
 	llist_for_each_prev(curr, &priv->list) {
 		msg = llist_entry(curr, struct ipfix_msg, link);
@@ -212,7 +213,7 @@ static int send_msgs(struct ulogd_pluginstance *pi)
 
 		/* TODO handle short send() for other protocols */
 		if ((size_t) sent < ipfix_msg_len(msg))
-			ulogd_log(ULOGD_ERROR, "short send: %d < %d\n",
+			ulogd_log(ULOGD_ERROR, "short send: %zd < %zu\n",
 					sent, ipfix_msg_len(msg));
 	}
 
@@ -242,7 +243,7 @@ static int ipfix_ufd_cb(int fd, unsigned what, void *arg)
 			ulogd_log(ULOGD_INFO, "connection reset by peer\n");
 			ulogd_unregister_fd(&priv->ufd);
 		} else
-			ulogd_log(ULOGD_INFO, "unexpected data (%d bytes)\n", nread);
+			ulogd_log(ULOGD_INFO, "unexpected data (%zd bytes)\n", nread);
 	}
 
 	return 0;
-- 
2.33.0

[ulogd2 PATCH 06/26] jhash: add "fall through" comments to switch cases.

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/ulogd/jhash.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/ulogd/jhash.h b/include/ulogd/jhash.h
index 38b87801a795..e5ca287e7e58 100644
--- a/include/ulogd/jhash.h
+++ b/include/ulogd/jhash.h
@@ -66,18 +66,18 @@ static inline u32 jhash(const void *key, u32 length, u32 initval)
 
 	c += length;
 	switch (len) {
-	case 11: c += ((u32)k[10]<<24);
-	case 10: c += ((u32)k[9]<<16);
-	case 9 : c += ((u32)k[8]<<8);
-	case 8 : b += ((u32)k[7]<<24);
-	case 7 : b += ((u32)k[6]<<16);
-	case 6 : b += ((u32)k[5]<<8);
-	case 5 : b += k[4];
-	case 4 : a += ((u32)k[3]<<24);
-	case 3 : a += ((u32)k[2]<<16);
-	case 2 : a += ((u32)k[1]<<8);
-	case 1 : a += k[0];
-	};
+	case 11: c += ((u32)k[10]<<24);	// fall through
+	case 10: c += ((u32)k[9]<<16);	// fall through
+	case 9 : c += ((u32)k[8]<<8);	// fall through
+	case 8 : b += ((u32)k[7]<<24);	// fall through
+	case 7 : b += ((u32)k[6]<<16);	// fall through
+	case 6 : b += ((u32)k[5]<<8);	// fall through
+	case 5 : b += k[4];		// fall through
+	case 4 : a += ((u32)k[3]<<24);	// fall through
+	case 3 : a += ((u32)k[2]<<16);	// fall through
+	case 2 : a += ((u32)k[1]<<8);	// fall through
+	case 1 : a += k[0];		// fall through
+	}
 
 	__jhash_mix(a,b,c);
 
-- 
2.33.0

[ulogd2 PATCH 07/26] db: add missing `break` to switch-case.

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 util/db.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/db.c b/util/db.c
index c9aec418e9ed..f0711146867f 100644
--- a/util/db.c
+++ b/util/db.c
@@ -388,6 +388,7 @@ static void __format_query_db(struct ulogd_pluginstance *upi, char *start)
 		case ULOGD_RET_RAW:
 			ulogd_log(ULOGD_NOTICE,
 				"Unsupported RAW type is unsupported in SQL output");
+			break;
 		default:
 			ulogd_log(ULOGD_NOTICE,
 				"unknown type %d for %s\n",
-- 
2.33.0

[ulogd2 PATCH 08/26] filter: HWHDR: replace `switch` with `if`.

[Jeremy Sowden]

The `switch` has one case falling through to a default.  Simplify the
flow-control.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 filter/ulogd_filter_HWHDR.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/filter/ulogd_filter_HWHDR.c b/filter/ulogd_filter_HWHDR.c
index 10c95c4e9bb0..d756d35577f0 100644
--- a/filter/ulogd_filter_HWHDR.c
+++ b/filter/ulogd_filter_HWHDR.c
@@ -207,19 +207,17 @@ static int interp_mac2str(struct ulogd_pluginstance *pi)
 		okey_set_u16(&ret[KEY_MAC_TYPE], type);
 	}
 
-	switch (type) {
-		case ARPHRD_ETHER:
-			parse_ethernet(ret, inp);
-		default:
-			if (!pp_is_valid(inp, KEY_RAW_MAC))
-				return ULOGD_IRET_OK;
-			/* convert raw header to string */
-			return parse_mac2str(ret,
-					    ikey_get_ptr(&inp[KEY_RAW_MAC]),
-					    KEY_MAC_ADDR,
-					    ikey_get_u16(&inp[KEY_RAW_MACLEN]));
-	}
-	return ULOGD_IRET_OK;
+	if (type == ARPHRD_ETHER)
+		parse_ethernet(ret, inp);
+
+	if (!pp_is_valid(inp, KEY_RAW_MAC))
+		return ULOGD_IRET_OK;
+
+	/* convert raw header to string */
+	return parse_mac2str(ret,
+			     ikey_get_ptr(&inp[KEY_RAW_MAC]),
+			     KEY_MAC_ADDR,
+			     ikey_get_u16(&inp[KEY_RAW_MACLEN]));
 }
 
 
-- 
2.33.0

[ulogd2 PATCH 09/26] filter: HWHDR: re-order KEY_RAW_MAC checks.

[Jeremy Sowden]

Currently we have:

  if (/* KEY_RAW_MAC is valid */) {
     /*
      * set mac type
      */
  }

  if (/* mac type is ethernet */)
    // parse ethernet

  if (/* KEY_RAW_MAC is not valid */)
    // return early.

The MAC type will not be set to ethernet unless KEY_RAW_MAC is valid,
so we can move the last check up and drop the first one:

  if (/* KEY_RAW_MAC is not valid */)
    // return early.

  /*
   * set mac type
   */

  if (/* mac type is ethernet */)
    // parse ethernet

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 filter/ulogd_filter_HWHDR.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/filter/ulogd_filter_HWHDR.c b/filter/ulogd_filter_HWHDR.c
index d756d35577f0..015121511b08 100644
--- a/filter/ulogd_filter_HWHDR.c
+++ b/filter/ulogd_filter_HWHDR.c
@@ -191,28 +191,26 @@ static int interp_mac2str(struct ulogd_pluginstance *pi)
 		okey_set_u16(&ret[KEY_MAC_TYPE], ARPHRD_VOID);
 	}
 
-	if (pp_is_valid(inp, KEY_RAW_MAC)) {
-		if (! pp_is_valid(inp, KEY_RAW_MACLEN))
-			return ULOGD_IRET_ERR;
-		if (pp_is_valid(inp, KEY_RAW_TYPE)) {
-			/* NFLOG with Linux >= 2.6.27 case */
-			type = ikey_get_u16(&inp[KEY_RAW_TYPE]);
-		} else {
-			/* ULOG case, treat ethernet encapsulation */
-			if (ikey_get_u16(&inp[KEY_RAW_MACLEN]) == ETH_HLEN)
-				type = ARPHRD_ETHER;
-			else
-				type = ARPHRD_VOID;
-		}
-		okey_set_u16(&ret[KEY_MAC_TYPE], type);
-	}
+	if (!pp_is_valid(inp, KEY_RAW_MAC))
+		return ULOGD_IRET_OK;
+
+	if (!pp_is_valid(inp, KEY_RAW_MACLEN))
+		return ULOGD_IRET_ERR;
+
+	if (pp_is_valid(inp, KEY_RAW_TYPE))
+		/* NFLOG with Linux >= 2.6.27 case */
+		type = ikey_get_u16(&inp[KEY_RAW_TYPE]);
+	else if (ikey_get_u16(&inp[KEY_RAW_MACLEN]) == ETH_HLEN)
+		/* ULOG case, treat ethernet encapsulation */
+		type = ARPHRD_ETHER;
+	else
+		type = ARPHRD_VOID;
+
+	okey_set_u16(&ret[KEY_MAC_TYPE], type);
 
 	if (type == ARPHRD_ETHER)
 		parse_ethernet(ret, inp);
 
-	if (!pp_is_valid(inp, KEY_RAW_MAC))
-		return ULOGD_IRET_OK;
-
 	/* convert raw header to string */
 	return parse_mac2str(ret,
 			     ikey_get_ptr(&inp[KEY_RAW_MAC]),
-- 
2.33.0

[ulogd2 PATCH 10/26] filter: HWHDR: remove zero-initialization of MAC type.

[Jeremy Sowden]

We don't need to initialize `type`, and even if we did the right
value would be `ARPHDR_VOID`, not `0` which is a valid MAC type
(`ARPHDR_NETROM`).

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 filter/ulogd_filter_HWHDR.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/filter/ulogd_filter_HWHDR.c b/filter/ulogd_filter_HWHDR.c
index 015121511b08..bbca5e9b92f2 100644
--- a/filter/ulogd_filter_HWHDR.c
+++ b/filter/ulogd_filter_HWHDR.c
@@ -171,7 +171,7 @@ static int interp_mac2str(struct ulogd_pluginstance *pi)
 {
 	struct ulogd_key *ret = pi->output.keys;
 	struct ulogd_key *inp = pi->input.keys;
-	uint16_t type = 0;
+	uint16_t type;
 
 	if (pp_is_valid(inp, KEY_OOB_PROTOCOL))
 		okey_set_u16(&ret[KEY_MAC_PROTOCOL],
-- 
2.33.0

[ulogd2 PATCH 11/26] Replace malloc+memset with calloc

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/dbi/ulogd_output_DBI.c     | 6 +-----
 output/ipfix/ipfix.c              | 4 +---
 output/mysql/ulogd_output_MYSQL.c | 6 +-----
 output/pgsql/ulogd_output_PGSQL.c | 6 +-----
 src/ulogd.c                       | 3 +--
 5 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index d2a968293314..23cc9c8fb492 100644
--- a/output/dbi/ulogd_output_DBI.c
+++ b/output/dbi/ulogd_output_DBI.c
@@ -129,8 +129,7 @@ static int get_columns_dbi(struct ulogd_pluginstance *upi)
 	upi->input.num_keys = dbi_result_get_numfields(pi->result);
 	ulogd_log(ULOGD_DEBUG, "%u fields in table\n", upi->input.num_keys);
 
-	upi->input.keys = malloc(sizeof(struct ulogd_key) *
-						upi->input.num_keys);
+	upi->input.keys = calloc(upi->input.num_keys, sizeof(*upi->input.keys));
 	if (!upi->input.keys) {
 		upi->input.num_keys = 0;
 		ulogd_log(ULOGD_ERROR, "ENOMEM\n");
@@ -138,9 +137,6 @@ static int get_columns_dbi(struct ulogd_pluginstance *upi)
 		return -ENOMEM;
 	}
 
-	memset(upi->input.keys, 0, sizeof(struct ulogd_key) *
-						upi->input.num_keys);
-
 	for (ui=1; ui<=upi->input.num_keys; ui++) {
 		char buf[ULOGD_MAX_KEYLEN+1];
 		char *underscore;
diff --git a/output/ipfix/ipfix.c b/output/ipfix/ipfix.c
index 4bb432a73d14..b2719fd1d8a3 100644
--- a/output/ipfix/ipfix.c
+++ b/output/ipfix/ipfix.c
@@ -85,8 +85,7 @@ struct ipfix_msg *ipfix_msg_alloc(size_t len, uint32_t oid, int tid)
 	    (len < IPFIX_HDRLEN + IPFIX_SET_HDRLEN))
 		return NULL;
 
-	msg = malloc(sizeof(struct ipfix_msg) + len);
-	memset(msg, 0, sizeof(struct ipfix_msg));
+	msg = calloc(1, sizeof(struct ipfix_msg) + len);
 	msg->tid = tid;
 	msg->end = msg->data + len;
 	msg->tail = msg->data + IPFIX_HDRLEN;
@@ -95,7 +94,6 @@ struct ipfix_msg *ipfix_msg_alloc(size_t len, uint32_t oid, int tid)
 
 	/* Initialize message header */
 	hdr = ipfix_msg_hdr(msg);
-	memset(hdr, 0, IPFIX_HDRLEN);
 	hdr->version = htons(IPFIX_VERSION);
 	hdr->oid = htonl(oid);
 
diff --git a/output/mysql/ulogd_output_MYSQL.c b/output/mysql/ulogd_output_MYSQL.c
index 643320ce724c..66151feb4939 100644
--- a/output/mysql/ulogd_output_MYSQL.c
+++ b/output/mysql/ulogd_output_MYSQL.c
@@ -127,16 +127,12 @@ static int get_columns_mysql(struct ulogd_pluginstance *upi)
 
 	upi->input.num_keys = mysql_num_fields(result);
 	ulogd_log(ULOGD_DEBUG, "%u fields in table\n", upi->input.num_keys);
-	upi->input.keys = malloc(sizeof(struct ulogd_key) * 
-						upi->input.num_keys);
+	upi->input.keys = calloc(upi->input.num_keys, sizeof(*upi->input.keys));
 	if (!upi->input.keys) {
 		upi->input.num_keys = 0;
 		ulogd_log(ULOGD_ERROR, "ENOMEM\n");
 		return -ENOMEM;
 	}
-	
-	memset(upi->input.keys, 0, sizeof(struct ulogd_key) *
-						upi->input.num_keys);
 
 	for (i = 0; (field = mysql_fetch_field(result)); i++) {
 		char buf[ULOGD_MAX_KEYLEN+1];
diff --git a/output/pgsql/ulogd_output_PGSQL.c b/output/pgsql/ulogd_output_PGSQL.c
index fda289eca776..f5a2823a7e1d 100644
--- a/output/pgsql/ulogd_output_PGSQL.c
+++ b/output/pgsql/ulogd_output_PGSQL.c
@@ -181,8 +181,7 @@ static int get_columns_pgsql(struct ulogd_pluginstance *upi)
 
 	upi->input.num_keys = PQntuples(pi->pgres);
 	ulogd_log(ULOGD_DEBUG, "%u fields in table\n", upi->input.num_keys);
-	upi->input.keys = malloc(sizeof(struct ulogd_key) *
-						upi->input.num_keys);
+	upi->input.keys = calloc(upi->input.num_keys, sizeof(*upi->input.keys));
 	if (!upi->input.keys) {
 		upi->input.num_keys = 0;
 		ulogd_log(ULOGD_ERROR, "ENOMEM\n");
@@ -190,9 +189,6 @@ static int get_columns_pgsql(struct ulogd_pluginstance *upi)
 		return -ENOMEM;
 	}
 
-	memset(upi->input.keys, 0, sizeof(struct ulogd_key) *
-						upi->input.num_keys);
-
 	for (i = 0; i < PQntuples(pi->pgres); i++) {
 		char buf[ULOGD_MAX_KEYLEN+1];
 		char *underscore;
diff --git a/src/ulogd.c b/src/ulogd.c
index 97da4fc0018f..b02f2602a895 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -661,12 +661,11 @@ pluginstance_alloc_init(struct ulogd_plugin *pl, char *pi_id,
 	}
 	size += pl->input.num_keys * sizeof(struct ulogd_key);
 	size += pl->output.num_keys * sizeof(struct ulogd_key);
-	pi = malloc(size);
+	pi = calloc(1, size);
 	if (!pi)
 		return NULL;
 
 	/* initialize */
-	memset(pi, 0, size);
 	INIT_LLIST_HEAD(&pi->list);
 	INIT_LLIST_HEAD(&pi->plist);
 	pi->plugin = pl;
-- 
2.33.0

[ulogd2 PATCH 12/26] filter: PWSNIFF: replace malloc+strncpy with strndup.

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 filter/ulogd_filter_PWSNIFF.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/filter/ulogd_filter_PWSNIFF.c b/filter/ulogd_filter_PWSNIFF.c
index 934ff0e09c4f..9060d16feac6 100644
--- a/filter/ulogd_filter_PWSNIFF.c
+++ b/filter/ulogd_filter_PWSNIFF.c
@@ -35,10 +35,14 @@
 #define DEBUGP(format, args...)
 #endif
 
-
 #define PORT_POP3	110
 #define PORT_FTP	21
 
+enum pwsniff_output_keys {
+	PWSNIFF_OUT_KEY_USER,
+	PWSNIFF_OUT_KEY_PASS,
+};
+
 static uint16_t pwsniff_ports[] = {
 	PORT_POP3,
 	PORT_FTP,
@@ -93,11 +97,11 @@ static int interp_pwsniff(struct ulogd_pluginstance *pi)
 		return ULOGD_IRET_STOP;
 
 	DEBUGP("----> pwsniff detected, tcplen=%d, struct=%d, iphtotlen=%d, "
-		"ihl=%d\n", tcplen, sizeof(struct tcphdr), ntohs(iph->tot_len),
-		iph->ihl);
+	       "ihl=%d\n", tcplen, sizeof(struct tcphdr), ntohs(iph->tot_len),
+	       iph->ihl);
 
 	for (ptr = (unsigned char *) tcph + sizeof(struct tcphdr); 
-			ptr < (unsigned char *) tcph + tcplen; ptr++)
+	     ptr < (unsigned char *) tcph + tcplen; ptr++)
 	{
 		if (!strncasecmp((char *)ptr, "USER ", 5)) {
 			begp = ptr+5;
@@ -108,7 +112,7 @@ static int interp_pwsniff(struct ulogd_pluginstance *pi)
 		if (!strncasecmp((char *)ptr, "PASS ", 5)) {
 			pw_begp = ptr+5;
 			pw_endp = _get_next_blank(pw_begp, 
-					(unsigned char *)tcph + tcplen);
+						  (unsigned char *)tcph + tcplen);
 			if (pw_endp)
 				pw_len = pw_endp - pw_begp + 1;
 		}
@@ -116,21 +120,17 @@ static int interp_pwsniff(struct ulogd_pluginstance *pi)
 
 	if (len) {
 		char *ptr;
-		ptr = (char *) malloc(len+1);
+		ptr = strndup((char *)begp, len);
 		if (!ptr)
 			return ULOGD_IRET_ERR;
-		strncpy(ptr, (char *)begp, len);
-		ptr[len] = '\0';
-		okey_set_ptr(&ret[0], ptr);
+		okey_set_ptr(&ret[PWSNIFF_OUT_KEY_USER], ptr);
 	}
 	if (pw_len) {
 		char *ptr;
-		ptr = (char *) malloc(pw_len+1);
+		ptr = strndup((char *)pw_begp, pw_len);
 		if (!ptr)
 			return ULOGD_IRET_ERR;
-		strncpy(ptr, (char *)pw_begp, pw_len);
-		ptr[pw_len] = '\0';
-		okey_set_ptr(&ret[1], ptr);
+		okey_set_ptr(&ret[PWSNIFF_OUT_KEY_PASS], ptr);
 	}
 	return ULOGD_IRET_OK;
 }
-- 
2.33.0

[ulogd2 PATCH 13/26] input: UNIXSOCK: stat socket-path first before creating the socket.

[Jeremy Sowden]

If the path is already bound, we close the socket immediately.

A couple of error message fixes.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 input/packet/ulogd_inppkt_UNIXSOCK.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index f97c2e174b2d..d88609f203c4 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path)
 	int s;
 	struct stat st_dummy;
 
+	if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) {
+		ulogd_log(ULOGD_ERROR,
+			  "ulogd2: unix socket '%s' already exists\n",
+			  unix_path);
+		return -1;
+	}
+
 	s = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (s < 0) {
 		ulogd_log(ULOGD_ERROR,
-				"ulogd2: could not create unix socket\n");
+			  "ulogd2: could not create unix socket\n");
 		return -1;
 	}
 
@@ -490,19 +497,11 @@ static int _create_unix_socket(const char *unix_path)
 	strncpy(server_sock.sun_path, unix_path, sizeof(server_sock.sun_path));
 	server_sock.sun_path[sizeof(server_sock.sun_path)-1] = '\0';
 
-	if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) {
-		ulogd_log(ULOGD_ERROR,
-				"ulogd2: unix socket \'%s\' already exists\n",
-				unix_path);
-		close(s);
-		return -1;
-	}
-
 	ret = bind(s, (struct sockaddr *)&server_sock, sizeof(server_sock));
 	if (ret < 0) {
 		ulogd_log(ULOGD_ERROR,
-				"ulogd2: could not bind to unix socket \'%s\'\n",
-				server_sock.sun_path);
+			  "ulogd2: could not bind to unix socket '%s'\n",
+			  server_sock.sun_path);
 		close(s);
 		return -1;
 	}
@@ -510,8 +509,8 @@ static int _create_unix_socket(const char *unix_path)
 	ret = listen(s, 10);
 	if (ret < 0) {
 		ulogd_log(ULOGD_ERROR,
-				"ulogd2: could not bind to unix socket \'%s\'\n",
-				server_sock.sun_path);
+			  "ulogd2: could not listen to unix socket '%s'\n",
+			  server_sock.sun_path);
 		close(s);
 		return -1;
 	}
-- 
2.33.0

Re: [ulogd2 PATCH 13/26] input: UNIXSOCK: stat socket-path first before creating the socket.

[Jan Engelhardt]

On Saturday 2021-10-30 18:44, Jeremy Sowden wrote:
>If the path is already bound, we close the socket immediately.
>diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
>index f97c2e174b2d..d88609f203c4 100644
>--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
>+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
>@@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path)
> 	int s;
> 	struct stat st_dummy;
> 
>+	if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) {
>+		ulogd_log(ULOGD_ERROR,
>+			  "ulogd2: unix socket '%s' already exists\n",
>+			  unix_path);
>+		return -1;
>+	}
>+

That stat call should just be entirely deleted.

I fully expect that Coverity's static analyzer (or something like it)
is going to flag this piece of code as running afoul of TOCTOU.

A foreign event may cause the socket to come into existence between stat() and
bind(), and therefore the code needs to handle the bind(2) failure _in any
case_, and can report "Oh but it exists" at that time.

So even if the stat call is fine from a security POV, it is redundant code.

Re: [ulogd2 PATCH 13/26] input: UNIXSOCK: stat socket-path first before creating the socket.

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On 2021-10-30, at 19:33:13 +0200, Jan Engelhardt wrote:
> On Saturday 2021-10-30 18:44, Jeremy Sowden wrote:
> > If the path is already bound, we close the socket immediately.
> > diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
> > index f97c2e174b2d..d88609f203c4 100644
> > --- a/input/packet/ulogd_inppkt_UNIXSOCK.c
> > +++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
> > @@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path)
> >  	int s;
> >  	struct stat st_dummy;
> >
> > +	if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) {
> > +		ulogd_log(ULOGD_ERROR,
> > +			  "ulogd2: unix socket '%s' already exists\n",
> > +			  unix_path);
> > +		return -1;
> > +	}
> > +
>
> That stat call should just be entirely deleted.
>
> I fully expect that Coverity's static analyzer (or something like it)
> is going to flag this piece of code as running afoul of TOCTOU.

Good point.  Will remove it instead.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[ulogd2 PATCH 14/26] input: UNIXSOCK: fix possible truncation of socket path

[Jeremy Sowden]

Verify that the path is short enough, and replace `strncpy` with `strcpy`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 input/packet/ulogd_inppkt_UNIXSOCK.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index d88609f203c4..af2fbeca1f4c 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -475,10 +475,19 @@ static int handle_packet(struct ulogd_pluginstance *upi, struct ulogd_unixsock_p
 static int _create_unix_socket(const char *unix_path)
 {
 	int ret = -1;
-	struct sockaddr_un server_sock;
+	struct sockaddr_un server_sock = { .sun_family = AF_UNIX };
 	int s;
 	struct stat st_dummy;
 
+	if (sizeof(server_sock.sun_path) <= strlen(unix_path)) {
+		ulogd_log(ULOGD_ERROR,
+			  "ulogd2: unix socket path '%s' too long\n",
+			  unix_path);
+		return -1;
+	}
+
+	strcpy(server_sock.sun_path, unix_path);
+
 	if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) {
 		ulogd_log(ULOGD_ERROR,
 			  "ulogd2: unix socket '%s' already exists\n",
@@ -493,10 +502,6 @@ static int _create_unix_socket(const char *unix_path)
 		return -1;
 	}
 
-	server_sock.sun_family = AF_UNIX;
-	strncpy(server_sock.sun_path, unix_path, sizeof(server_sock.sun_path));
-	server_sock.sun_path[sizeof(server_sock.sun_path)-1] = '\0';
-
 	ret = bind(s, (struct sockaddr *)&server_sock, sizeof(server_sock));
 	if (ret < 0) {
 		ulogd_log(ULOGD_ERROR,
-- 
2.33.0

[ulogd2 PATCH 15/26] input: UNIXSOCK: prevent unaligned pointer access.

[Jeremy Sowden]

`struct ulogd_unixsock_packet_t` is packed, so taking the address of its
`struct iphdr payload` member may yield an unaligned pointer value.
Copy it to a local variable instead.

Remove a couple of stray semicolons.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 input/packet/ulogd_inppkt_UNIXSOCK.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index af2fbeca1f4c..f7611189363c 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -371,7 +371,7 @@ struct ulogd_unixsock_option_t  {
 static int handle_packet(struct ulogd_pluginstance *upi, struct ulogd_unixsock_packet_t *pkt, uint16_t total_len)
 {
 	char *data = NULL;
-	struct iphdr *ip;
+	struct iphdr ip = pkt->payload;
 	struct ulogd_key *ret = upi->output.keys;
 	uint8_t oob_family;
 	uint16_t payload_len;
@@ -387,22 +387,22 @@ static int handle_packet(struct ulogd_pluginstance *upi, struct ulogd_unixsock_p
 
 	payload_len = ntohs(pkt->payload_length);
 
-	ip = &pkt->payload;
-	if (ip->version == 4)
+	if (ip.version == 4)
 		oob_family = AF_INET;
-	else if (ip->version == 6)
+	else if (ip.version == 6)
 		oob_family = AF_INET6;
-	else oob_family = 0;
+	else
+		oob_family = 0;
 
 	okey_set_u8(&ret[UNIXSOCK_KEY_OOB_FAMILY], oob_family);
-	okey_set_ptr(&ret[UNIXSOCK_KEY_RAW_PCKT], ip);
+	okey_set_ptr(&ret[UNIXSOCK_KEY_RAW_PCKT], &ip);
 	okey_set_u32(&ret[UNIXSOCK_KEY_RAW_PCKTLEN], payload_len);
 
 	/* options */
 	if (total_len > payload_len + sizeof(uint16_t)) {
 		/* option starts at the next aligned address after the payload */
 		new_offset = USOCK_ALIGN(payload_len);
-		options_start = (void*)ip + new_offset;
+		options_start = (char *) &ip + new_offset;
 		data = options_start;
 		total_len -= (options_start - (char*)pkt);
 
@@ -460,7 +460,7 @@ static int handle_packet(struct ulogd_pluginstance *upi, struct ulogd_unixsock_p
 						"ulogd2: unknown option %d\n",
 						option_number);
 				break;
-			};
+			}
 		}
 	}
 
@@ -674,7 +674,7 @@ static int unixsock_instance_read_cb(int fd, unsigned int what, void *param)
 		}
 
 		/* handle_packet has shifted data in buffer */
-	};
+	}
 
 	return 0;
 }
-- 
2.33.0

Re: [ulogd2 PATCH 15/26] input: UNIXSOCK: prevent unaligned pointer access.

[Jan Engelhardt]

On Saturday 2021-10-30 18:44, Jeremy Sowden wrote:

>`struct ulogd_unixsock_packet_t` is packed, so taking the address of its
>`struct iphdr payload` member may yield an unaligned pointer value.

That may not be a problem. Dereferencing through a pointer to
a packed struct generates very pessimistic code even when there is no
padding internally:

» cat >>x.cpp <<-EOF
struct ethhdr { unsigned long long a, b; } __attribute__((packed));
unsigned long f(const struct ethhdr *p) { return p->b; }
EOF
» sparc64-linux-gcc -c x.cpp -O2
» objdump -d x.o
_Z1fPK6ethhdr:
        save %sp, -144, %sp
        stx %i0, [%fp+2039]
        ldx [%fp+2039], %i0
        ldub [%i0+15], %i2
        ldub [%i0+14], %i1
        sllx %i1, 8, %i1
        or %i1, %i2, %i2
        ldub [%i0+13], %i3
...

Re: [ulogd2 PATCH 15/26] input: UNIXSOCK: prevent unaligned pointer access.

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

On 2021-10-30, at 19:42:25 +0200, Jan Engelhardt wrote:
> On Saturday 2021-10-30 18:44, Jeremy Sowden wrote:
> >`struct ulogd_unixsock_packet_t` is packed, so taking the address of its
> >`struct iphdr payload` member may yield an unaligned pointer value.
>
> That may not be a problem. Dereferencing through a pointer to
> a packed struct generates very pessimistic code even when there is no
> padding internally:

Having taken another look at this, I've adopted different approach to
pacify the compiler: the pointer is only actually dereferenced to read
one member (`ip->version`), so I shall replace the local pointer
variable with an `ip_version` variable instead.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[ulogd2 PATCH 16/26] output: DBI: fix deprecation warnings.

[Jeremy Sowden]

Switch to re-entrant libdbi functions.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/dbi/ulogd_output_DBI.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index 23cc9c8fb492..461aed4bddb6 100644
--- a/output/dbi/ulogd_output_DBI.c
+++ b/output/dbi/ulogd_output_DBI.c
@@ -29,6 +29,8 @@
 #define DEBUGP(x, args...)
 #endif
 
+static dbi_inst libdbi_instance;
+
 struct dbi_instance {
 	struct db_instance db_inst;
 
@@ -195,14 +197,14 @@ static int open_db_dbi(struct ulogd_pluginstance *upi)
 
 	ulogd_log(ULOGD_ERROR, "Opening connection for db type %s\n",
 		  dbtype);
-	driver = dbi_driver_open(dbtype);
+	driver = dbi_driver_open_r(dbtype, libdbi_instance);
 	if (driver == NULL) {
 		ulogd_log(ULOGD_ERROR, "unable to load driver for db type %s\n",
 			  dbtype);
 		close_db_dbi(upi);
 		return -1;
 	}
-	pi->dbh = dbi_conn_new(dbtype);
+	pi->dbh = dbi_conn_new_r(dbtype, libdbi_instance);
 	if (pi->dbh == NULL) {
 		ulogd_log(ULOGD_ERROR, "unable to initialize db type %s\n",
 			  dbtype);
@@ -320,7 +322,7 @@ void __attribute__ ((constructor)) init(void);
 
 void init(void)
 {
-	dbi_initialize(NULL);
+	dbi_initialize_r(NULL, &libdbi_instance);
 
 	ulogd_register_plugin(&dbi_plugin);
 }
-- 
2.33.0

[ulogd2 PATCH 17/26] output: DBI: fix string truncation warnings

[Jeremy Sowden]

Replace `strncpy` with `snprintf` and `memcpy`.

Remove intermediate buffer.

Ensure that `dst` is properly initialized if `dbi_conn_quote_string_copy`
returns an error.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/dbi/ulogd_output_DBI.c | 46 +++++++++++++++--------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index 461aed4bddb6..babaf58a9a56 100644
--- a/output/dbi/ulogd_output_DBI.c
+++ b/output/dbi/ulogd_output_DBI.c
@@ -91,15 +91,6 @@ static struct config_keyset dbi_kset = {
 #define dbtype_ce(x)	(x->ces[DB_CE_NUM+6])
 
 
-/* lower-cases s in place */
-static void str_tolower(char *s)
-{
-	while(*s) {
-		*s = tolower(*s);
-		s++;
-	}
-}
-
 /* find out which columns the table has */
 static int get_columns_dbi(struct ulogd_pluginstance *upi)
 {
@@ -139,25 +130,26 @@ static int get_columns_dbi(struct ulogd_pluginstance *upi)
 		return -ENOMEM;
 	}
 
-	for (ui=1; ui<=upi->input.num_keys; ui++) {
-		char buf[ULOGD_MAX_KEYLEN+1];
-		char *underscore;
-		const char* field_name = dbi_result_get_field_name(pi->result, ui);
+	for (ui = 1; ui <= upi->input.num_keys; ui++) {
+		const char *field_name = dbi_result_get_field_name(pi->result, ui);
+		char *cp;
 
 		if (!field_name)
 			break;
 
-		/* replace all underscores with dots */
-		strncpy(buf, field_name, ULOGD_MAX_KEYLEN);
-		while ((underscore = strchr(buf, '_')))
-			*underscore = '.';
+		snprintf(upi->input.keys[ui - 1].name,
+			 sizeof(upi->input.keys[ui - 1].name),
+			 "%s", field_name);
 
-		str_tolower(buf);
+		/* down-case and replace all underscores with dots */
+		for (cp = upi->input.keys[ui - 1].name; *cp; cp++) {
+			if (*cp == '_')
+				*cp = '.';
+			else
+				*cp = tolower(*cp);
+		}
 
-		DEBUGP("field '%s' found: ", buf);
-
-		/* add it to list of input keys */
-		strncpy(upi->input.keys[ui-1].name, buf, ULOGD_MAX_KEYLEN);
+		DEBUGP("field '%s' found: ", upi->input.keys[ui - 1].name);
 	}
 
 	/* ID is a sequence */
@@ -245,18 +237,20 @@ static int escape_string_dbi(struct ulogd_pluginstance *upi,
 	}
 
 	ret = dbi_conn_quote_string_copy(pi->dbh, src, &newstr);
-	if (ret <= 2)
+	if (ret == 0) {
+		*dst = '\0';
 		return 0;
+	}
 
 	/* dbi_conn_quote_string_copy returns a quoted string,
 	 * but __interp_db already quotes the string
 	 * So we return a string without the quotes
 	 */
-	strncpy(dst,newstr+1,ret-2);
-	dst[ret-2] = '\0';
+	memcpy(dst, newstr + 1, ret - 2);
+	dst[ret - 2] = '\0';
 	free(newstr);
 
-	return (ret-2);
+	return ret - 2;
 }
 
 static int execute_dbi(struct ulogd_pluginstance *upi,
-- 
2.33.0

[ulogd2 PATCH 18/26] output: MYSQL: Fix string truncation warning

[Jeremy Sowden]

Replace `strncpy` with `snprintf`.

Remove superfluous intermediate buffer.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/mysql/ulogd_output_MYSQL.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/output/mysql/ulogd_output_MYSQL.c b/output/mysql/ulogd_output_MYSQL.c
index 66151feb4939..946498d4d2fe 100644
--- a/output/mysql/ulogd_output_MYSQL.c
+++ b/output/mysql/ulogd_output_MYSQL.c
@@ -135,18 +135,17 @@ static int get_columns_mysql(struct ulogd_pluginstance *upi)
 	}
 
 	for (i = 0; (field = mysql_fetch_field(result)); i++) {
-		char buf[ULOGD_MAX_KEYLEN+1];
 		char *underscore;
 
+		snprintf(upi->input.keys[i].name,
+			 sizeof(upi->input.keys[i].name),
+			 "%s", field->name);
 		/* replace all underscores with dots */
-		strncpy(buf, field->name, ULOGD_MAX_KEYLEN);
-		while ((underscore = strchr(buf, '_')))
+		for (underscore = upi->input.keys[i].name;
+		     (underscore = strchr(underscore, '_')); )
 			*underscore = '.';
 
-		DEBUGP("field '%s' found\n", buf);
-
-		/* add it to list of input keys */
-		strncpy(upi->input.keys[i].name, buf, ULOGD_MAX_KEYLEN);
+		DEBUGP("field '%s' found\n", upi->input.keys[i].name);
 	}
 	/* MySQL Auto increment ... ID :) */
 	upi->input.keys[0].flags |= ULOGD_KEYF_INACTIVE;
-- 
2.33.0

[ulogd2 PATCH 19/26] output: PGSQL: Fix string truncation warning

[Jeremy Sowden]

Replace `strncpy` with `snprintf`.

Remove superfluous intermediate buffer.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/pgsql/ulogd_output_PGSQL.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/output/pgsql/ulogd_output_PGSQL.c b/output/pgsql/ulogd_output_PGSQL.c
index f5a2823a7e1d..c20ca605fb52 100644
--- a/output/pgsql/ulogd_output_PGSQL.c
+++ b/output/pgsql/ulogd_output_PGSQL.c
@@ -190,18 +190,17 @@ static int get_columns_pgsql(struct ulogd_pluginstance *upi)
 	}
 
 	for (i = 0; i < PQntuples(pi->pgres); i++) {
-		char buf[ULOGD_MAX_KEYLEN+1];
 		char *underscore;
 
+		snprintf(upi->input.keys[i].name,
+			 sizeof(upi->input.keys[i].name),
+			 "%s", PQgetvalue(pi->pgres, i, 0));
 		/* replace all underscores with dots */
-		strncpy(buf, PQgetvalue(pi->pgres, i, 0), ULOGD_MAX_KEYLEN);
-		while ((underscore = strchr(buf, '_')))
+		for (underscore = upi->input.keys[i].name;
+		     (underscore = strchr(underscore, '_')); )
 			*underscore = '.';
 
-		DEBUGP("field '%s' found: ", buf);
-
-		/* add it to list of input keys */
-		strncpy(upi->input.keys[i].name, buf, ULOGD_MAX_KEYLEN);
+		DEBUGP("field '%s' found\n", upi->input.keys[i].name);
 	}
 
 	/* ID (starting by '.') is a sequence */
-- 
2.33.0

[ulogd2 PATCH 20/26] output: SQLITE3: Fix string truncation warnings and possible buffer overruns.

[Jeremy Sowden]

Extend name length to match input key.

Replace strncpy with snprintf.

Remove intermediate buffers.

Leave `field->name` with underscores: we can get the key-name from
`field->key->name`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/sqlite3/ulogd_output_SQLITE3.c | 38 +++++++++++----------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 20ceb3b5d6e2..053d7a3b0238 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -48,7 +48,7 @@
 
 struct field {
 	TAILQ_ENTRY(field) link;
-	char name[ULOGD_MAX_KEYLEN];
+	char name[ULOGD_MAX_KEYLEN + 1];
 	struct ulogd_key *key;
 };
 
@@ -214,8 +214,6 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 {
 	struct sqlite3_priv *priv = (void *)pi->private;
 	struct field *f;
-	char buf[ULOGD_MAX_KEYLEN];
-	char *underscore;
 	char *stmt_pos;
 	int i, cols = 0;
 
@@ -231,12 +229,7 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 	stmt_pos = priv->stmt + strlen(priv->stmt);
 
 	tailq_for_each(f, priv->fields, link) {
-		strncpy(buf, f->name, ULOGD_MAX_KEYLEN);
-
-		while ((underscore = strchr(buf, '.')))
-			*underscore = '_';
-
-		sprintf(stmt_pos, "%s,", buf);
+		sprintf(stmt_pos, "%s,", f->name);
 		stmt_pos = priv->stmt + strlen(priv->stmt);
 
 		cols++;
@@ -273,10 +266,15 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 static struct ulogd_key *
 ulogd_find_key(struct ulogd_pluginstance *pi, const char *name)
 {
+	char key_name[ULOGD_MAX_KEYLEN + 1] = "";
 	unsigned int i;
 
+	/* replace all underscores with dots */
+	for (i = 0; i < sizeof(key_name) && name[i]; ++i)
+		key_name[i] = name[i] != '_' ? name[i] : '.';
+
 	for (i = 0; i < pi->input.num_keys; i++) {
-		if (strcmp(pi->input.keys[i].name, name) == 0)
+		if (strcmp(pi->input.keys[i].name, key_name) == 0)
 			return &pi->input.keys[i];
 	}
 
@@ -305,9 +303,6 @@ static int
 sqlite3_init_db(struct ulogd_pluginstance *pi)
 {
 	struct sqlite3_priv *priv = (void *)pi->private;
-	char buf[ULOGD_MAX_KEYLEN];
-	char *underscore;
-	struct field *f;
 	sqlite3_stmt *schema_stmt;
 	int col, num_cols;
 
@@ -327,23 +322,22 @@ sqlite3_init_db(struct ulogd_pluginstance *pi)
 	}
 
 	for (col = 0; col < num_cols; col++) {
-		strncpy(buf, sqlite3_column_name(schema_stmt, col), ULOGD_MAX_KEYLEN);
-
-		/* replace all underscores with dots */
-		while ((underscore = strchr(buf, '_')) != NULL)
-			*underscore = '.';
-
-		DEBUGP("field '%s' found\n", buf);
+		struct field *f;
 
 		/* prepend it to the linked list */
 		if ((f = calloc(1, sizeof(struct field))) == NULL) {
 			ulogd_log(ULOGD_ERROR, "SQLITE3: out of memory\n");
 			return -1;
 		}
-		strncpy(f->name, buf, ULOGD_MAX_KEYLEN);
+		snprintf(f->name, sizeof(f->name),
+			 "%s", sqlite3_column_name(schema_stmt, col));
 
-		if ((f->key = ulogd_find_key(pi, buf)) == NULL)
+		DEBUGP("field '%s' found\n", f->name);
+
+		if ((f->key = ulogd_find_key(pi, f->name)) == NULL) {
+			free(f);
 			return -1;
+		}
 
 		TAILQ_INSERT_TAIL(&priv->fields, f, link);
 	}
-- 
2.33.0

[ulogd2 PATCH 21/26] output: SQLITE3: catch errors creating SQL statement

[Jeremy Sowden]

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/sqlite3/ulogd_output_SQLITE3.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 053d7a3b0238..f2ee03b8c446 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -104,11 +104,14 @@ add_row(struct ulogd_pluginstance *pi)
 		ret = sqlite3_finalize(priv->p_stmt);
 		priv->p_stmt = NULL;
 
-		if (ret == SQLITE_SCHEMA)
-			sqlite3_createstmt(pi);
-		else {
+		if (ret != SQLITE_SCHEMA) {
 			ulogd_log(ULOGD_ERROR, "SQLITE3: step: %s\n",
-					  sqlite3_errmsg(priv->dbh));
+				  sqlite3_errmsg(priv->dbh));
+			goto err_reset;
+		}
+		if (sqlite3_createstmt(pi) < 0) {
+			ulogd_log(ULOGD_ERROR,
+				  "SQLITE3: Could not create statement.\n");
 			goto err_reset;
 		}
 	}
@@ -253,8 +256,8 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 	sqlite3_prepare(priv->dbh, priv->stmt, -1, &priv->p_stmt, 0);
 	if (priv->p_stmt == NULL) {
 		ulogd_log(ULOGD_ERROR, "SQLITE3: prepare: %s\n",
-				  sqlite3_errmsg(priv->dbh));
-		return 1;
+			  sqlite3_errmsg(priv->dbh));
+		return -1;
 	}
 
 	DEBUGP("statement prepared.\n");
@@ -388,7 +391,10 @@ sqlite3_start(struct ulogd_pluginstance *pi)
 	}
 
 	/* create and prepare the actual insert statement */
-	sqlite3_createstmt(pi);
+	if (sqlite3_createstmt(pi) < 0) {
+		ulogd_log(ULOGD_ERROR, "SQLITE3: Could not create statement.\n");
+		return -1;
+	}
 
 	return 0;
 }
-- 
2.33.0

[ulogd2 PATCH 22/26] util: db: fix possible string truncation.

[Jeremy Sowden]

Correct buffer size to match that of key-name.

We can now replace strncpy with strcpy.

Don't start strchr from the beginning every time.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 util/db.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/util/db.c b/util/db.c
index f0711146867f..0f8eb7057436 100644
--- a/util/db.c
+++ b/util/db.c
@@ -10,7 +10,7 @@
  *           (C) 2008,2013 Eric Leblond <eric@regit.org>
  *
  *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License version 2 
+ *  it under the terms of the GNU General Public License version 2
  *  as published by the Free Software Foundation
  *
  *  This program is distributed in the hope that it will be useful,
@@ -21,7 +21,7 @@
  *  You should have received a copy of the GNU General Public License
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- * 
+ *
  */
 
 #include <unistd.h>
@@ -96,8 +96,6 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 	if (strncasecmp(procedure,"INSERT", strlen("INSERT")) == 0 &&
 	    (procedure[strlen("INSERT")] == '\0' ||
 			procedure[strlen("INSERT")] == ' ')) {
-		char buf[ULOGD_MAX_KEYLEN];
-		char *underscore;
 
 		if(procedure[6] == '\0') {
 			/* procedure == "INSERT" */
@@ -112,11 +110,13 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 		stmt_val = mi->stmt + strlen(mi->stmt);
 
 		for (i = 0; i < upi->input.num_keys; i++) {
+			char buf[sizeof(upi->input.keys[0].name)], *underscore = buf;
+
 			if (upi->input.keys[i].flags & ULOGD_KEYF_INACTIVE)
 				continue;
 
-			strncpy(buf, upi->input.keys[i].name, ULOGD_MAX_KEYLEN);	
-			while ((underscore = strchr(buf, '.')))
+			strcpy(buf, upi->input.keys[i].name);
+			while ((underscore = strchr(underscore, '.')))
 				*underscore = '_';
 			sprintf(stmt_val, "%s,", buf);
 			stmt_val = mi->stmt + strlen(mi->stmt);
@@ -168,7 +168,7 @@ int ulogd_db_configure(struct ulogd_pluginstance *upi,
 	ret = di->driver->get_columns(upi);
 	if (ret < 0)
 		ulogd_log(ULOGD_ERROR, "error in get_columns\n");
-	
+
 	/* Close database, since ulogd core could just call configure
 	 * but abort during input key resolving routines.  configure
 	 * doesn't have a destructor... */
@@ -215,7 +215,7 @@ int ulogd_db_start(struct ulogd_pluginstance *upi)
 
 	if (di->ring.size > 0) {
 		/* allocate */
-		di->ring.ring = calloc(di->ring.size, sizeof(char) * di->ring.length);
+		di->ring.ring = calloc(di->ring.size, di->ring.length);
 		if (di->ring.ring == NULL) {
 			ret = -1;
 			goto db_error;
@@ -226,9 +226,8 @@ int ulogd_db_start(struct ulogd_pluginstance *upi)
 			  di->ring.size, di->ring.length);
 		/* init start of query for each element */
 		for(i = 0; i < di->ring.size; i++) {
-			strncpy(di->ring.ring + di->ring.length * i + 1,
-				di->stmt,
-				strlen(di->stmt));
+			strcpy(di->ring.ring + di->ring.length * i + 1,
+			       di->stmt);
 		}
 		/* init cond & mutex */
 		ret = pthread_cond_init(&di->ring.cond, NULL);
@@ -314,7 +313,7 @@ static int _init_reconnect(struct ulogd_pluginstance *upi)
 	/* Disable plugin permanently */
 	ulogd_log(ULOGD_ERROR, "permanently disabling plugin\n");
 	di->interp = &disabled_interp_db;
-	
+
 	return 0;
 }
 
-- 
2.33.0

[ulogd2 PATCH 23/26] output: JSON: fix output of GMT offset.

[Jeremy Sowden]

`gmt_offset` is a `long int`.  Use `labs` and update the format-specifier
to match.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/ulogd_output_JSON.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index 6edfa902efaf..621333261733 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -303,10 +303,10 @@ static int json_interp(struct ulogd_pluginstance *upi)
 		t = localtime_r(&now, &result);
 		if (unlikely(*opi->cached_tz = '\0' || t->tm_gmtoff != opi->cached_gmtoff)) {
 			snprintf(opi->cached_tz, sizeof(opi->cached_tz),
-				 "%c%02d%02d",
+				 "%c%02ld%02ld",
 				 t->tm_gmtoff > 0 ? '+' : '-',
-				 abs(t->tm_gmtoff) / 60 / 60,
-				 abs(t->tm_gmtoff) / 60 % 60);
+				 labs(t->tm_gmtoff) / 60 / 60,
+				 labs(t->tm_gmtoff) / 60 % 60);
 		}
 
 		if (pp_is_valid(inp, opi->usec_idx)) {
-- 
2.33.0

[ulogd2 PATCH 24/26] output: JSON: fix printf truncation warnings.

[Jeremy Sowden]

Mod gmt offset by 86400 to give the compiler a hint.

Make date-time buffer big enough to fit all integer values.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/ulogd_output_JSON.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index 621333261733..6c61eb144135 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -269,7 +269,7 @@ static int json_interp_file(struct ulogd_pluginstance *upi, char *buf)
 	return ULOGD_IRET_OK;
 }
 
-#define MAX_LOCAL_TIME_STRING 38
+#define MAX_LOCAL_TIME_STRING 80
 
 static int json_interp(struct ulogd_pluginstance *upi)
 {
@@ -305,8 +305,8 @@ static int json_interp(struct ulogd_pluginstance *upi)
 			snprintf(opi->cached_tz, sizeof(opi->cached_tz),
 				 "%c%02ld%02ld",
 				 t->tm_gmtoff > 0 ? '+' : '-',
-				 labs(t->tm_gmtoff) / 60 / 60,
-				 labs(t->tm_gmtoff) / 60 % 60);
+				 labs(t->tm_gmtoff) % 86400 / 60 / 60,
+				 labs(t->tm_gmtoff) % 86400 / 60 % 60);
 		}
 
 		if (pp_is_valid(inp, opi->usec_idx)) {
-- 
2.33.0

[ulogd2 PATCH 25/26] output: JSON: optimize appending of newline to output.

[Jeremy Sowden]

We have `buflen` available.  We can remove `strncat` and assign the characters
directly, without traversing the whole buffer.

Correct `buflen` type and fix leak if `realloc` fails.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/ulogd_output_JSON.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index 6c61eb144135..c15c9f239441 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -275,8 +275,8 @@ static int json_interp(struct ulogd_pluginstance *upi)
 {
 	struct json_priv *opi = (struct json_priv *) &upi->private;
 	unsigned int i;
-	char *buf;
-	int buflen;
+	char *buf, *tmp;
+	size_t buflen;
 	json_t *msg;
 
 	msg = json_object();
@@ -337,8 +337,6 @@ static int json_interp(struct ulogd_pluginstance *upi)
 		json_object_set_new(msg, "dvc", json_string(dvc));
 	}
 
-
-
 	for (i = 0; i < upi->input.num_keys; i++) {
 		struct ulogd_key *key = upi->input.keys[i].u.source;
 		char *field_name;
@@ -391,7 +389,6 @@ static int json_interp(struct ulogd_pluginstance *upi)
 		}
 	}
 
-
 	buf = json_dumps(msg, 0);
 	json_decref(msg);
 	if (buf == NULL) {
@@ -399,13 +396,15 @@ static int json_interp(struct ulogd_pluginstance *upi)
 		return ULOGD_IRET_ERR;
 	}
 	buflen = strlen(buf);
-	buf = realloc(buf, sizeof(char)*(buflen+2));
-	if (buf == NULL) {
+	tmp = realloc(buf, buflen + sizeof("\n"));
+	if (tmp == NULL) {
+		free(buf);
 		ulogd_log(ULOGD_ERROR, "Could not create message\n");
 		return ULOGD_IRET_ERR;
 	}
-	strncat(buf, "\n", 1);
-	buflen++;
+	buf = tmp;
+	buf[buflen++] = '\n';
+	buf[buflen]   = '\0';
 
 	if (opi->mode == JSON_MODE_FILE)
 		return json_interp_file(upi, buf);
-- 
2.33.0

[ulogd2 PATCH 26/26] output: JSON: fix possible truncation of socket path

[Jeremy Sowden]

Verify that the path is short enough, and replace `strncpy` with `strcpy`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/ulogd_output_JSON.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index c15c9f239441..3b0338991548 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -147,7 +147,8 @@ static void close_socket(struct json_priv *op) {
 static int _connect_socket_unix(struct ulogd_pluginstance *pi)
 {
 	struct json_priv *op = (struct json_priv *) &pi->private;
-	struct sockaddr_un u_addr;
+	struct sockaddr_un u_addr = { .sun_family = AF_UNIX };
+	const char *socket_path = file_ce(pi->config_kset).u.string;
 	int sfd;
 
 	close_socket(op);
@@ -156,13 +157,15 @@ static int _connect_socket_unix(struct ulogd_pluginstance *pi)
 		  file_ce(pi->config_kset).u.string);
 
 	sfd = socket(AF_UNIX, SOCK_STREAM, 0);
-	if (sfd == -1) {
+	if (sfd == -1)
 		return -1;
-	}
-	u_addr.sun_family = AF_UNIX;
-	strncpy(u_addr.sun_path, file_ce(pi->config_kset).u.string,
-		sizeof(u_addr.sun_path) - 1);
-	if (connect(sfd, (struct sockaddr *) &u_addr, sizeof(struct sockaddr_un)) == -1) {
+
+	if (sizeof(u_addr.sun_path) <= strlen(socket_path))
+		return -1;
+
+	strcpy(u_addr.sun_path, socket_path);
+
+	if (connect(sfd, (struct sockaddr *) &u_addr, sizeof(u_addr)) == -1) {
 		close(sfd);
 		return -1;
 	}
-- 
2.33.0