[ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[Jeremy Sowden]

This patch-set fixes all the warnings reported by gcc 11.

Most of the warnings concern fall-throughs in switches, possibly
problematic uses of functions like `strncpy` and `strncat` and possible
truncation of output by `sprintf` and its siblings.

Some of the patches fix bugs revealed by warnings, some tweak code to
avoid warnings, others fix or improve things I noticed while looking at
the warnings.

Changes since v3:

  * When publishing v3 I accidentally sent out two different versions of the
    patch-set under one cover-letter.  There are no code-changes in v4: it just
    omits the earlier superseded patches.
  
Changes since v2:

  * the first four patches of v2 have been merged;
  * some of the v2 patches have been broken up into more, smaller parts;
  * more detailed commit messages;
  * patches 14 and 17 are new.

Changes since v1:

  * patch 13: stat of socket removed;
  * patch 15: `struct iphdr` pointer removed;
  * patch 27 is new.

Jeremy Sowden (32):
  jhash: add "fall through" comments to switch cases
  db: add missing `break` to switch case
  filter: HWHDR: simplify flow-control
  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: remove stat of socket-path
  input: UNIXSOCK: fix possible truncation of socket path
  input: UNIXSOCK: prevent unaligned pointer access
  output: DBI: fix deprecation warnings
  output: DBI: improve mapping of DB columns to input-keys
  output: DBI: fix NUL-termination of escaped SQL string
  output: DBI: fix configuration of DB connection
  output: MYSQL: improve mapping of DB columns to input-keys
  output: PGSQL: improve mapping of DB columns to input-keys
  output: PGSQL: fix non-`connstring` configuration of DB connection
  output: SQLITE3: fix possible buffer overruns
  output: SQLITE3: fix memory-leak in error-handling
  output: SQLITE3: improve formatting of insert statement
  output: SQLITE3: improve mapping of DB columns to fields
  output: SQLITE3: improve mapping of fields to DB columns
  output: SQLITE3: catch errors creating SQL statement
  db: improve formatting of insert statement
  db: improve mapping of input-keys to DB columns
  db: simplify initialization of ring-buffer
  output: JSON: fix output of GMT offset
  output: JSON: increase time-stamp buffer size
  output: JSON: fix possible leak in error-handling.
  output: JSON: optimize appending of newline to output
  output: JSON: fix possible truncation of socket path
  output: IPFIX: remove compiler attribute macros

 filter/ulogd_filter_HWHDR.c           | 54 ++++++++---------
 filter/ulogd_filter_PWSNIFF.c         | 18 +++---
 include/ulogd/jhash.h                 | 24 ++++----
 include/ulogd/ulogd.h                 |  5 --
 input/packet/ulogd_inppkt_UNIXSOCK.c  | 46 +++++++--------
 output/dbi/ulogd_output_DBI.c         | 84 +++++++++++++--------------
 output/ipfix/ipfix.c                  |  6 +-
 output/ipfix/ipfix.h                  |  8 +--
 output/mysql/ulogd_output_MYSQL.c     | 20 +++----
 output/pgsql/ulogd_output_PGSQL.c     | 64 ++++++++------------
 output/sqlite3/ulogd_output_SQLITE3.c | 71 +++++++++++-----------
 output/ulogd_output_JSON.c            | 45 +++++++-------
 src/ulogd.c                           |  3 +-
 util/db.c                             | 36 ++++++------
 14 files changed, 223 insertions(+), 261 deletions(-)

-- 
2.33.0

[ulogd2 PATCH v4 01/32] jhash: add "fall through" comments to switch cases

[Jeremy Sowden]

gcc warns about undocumented fall-throughs in switches.  In this case,
the fall-throughs are intended, so add commnts to indicate this to the
compiler.

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 v4 02/32] db: add missing `break` to switch case

[Jeremy Sowden]

When formatting DB queries, if we get a input key of type `RAW`, we log
a message indicating that `RAW` is unsupported, then fall through to the
default case, which logs another message that the key type is unknown.
Add the missing `break` statement to prevent the fall-through.

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 v4 03/32] filter: HWHDR: simplify flow-control

[Jeremy Sowden]

The `interp_mac2str` function concludes with a `switch` followed by a
`return` statement.

The `switch` has one case falling through to a default:

  switch (expr) {
  case X:
    // ... X code ...
  default:
    // ... default code ...
  }

This is equivalent to the simpler and more readily comprehensible:

  if (expr == X) {
    // ... X code ...
  }
  // ... default code ...

Replace the former with the latter.

Doing so makes it obvious that the following `return` statement is never
reached.  Remove it.

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 v4 04/32] filter: HWHDR: re-order KEY_RAW_MAC checks

[Jeremy Sowden]

Currently, in `interp_mac2str` 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 v4 05/32] 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 v4 06/32] Replace malloc+memset with calloc

[Jeremy Sowden]

There are a number of places where we `malloc` some memory and then
`memset` it to zero.  Use `calloc` instead.

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 v4 07/32] filter: PWSNIFF: replace malloc+strncpy with strndup

[Jeremy Sowden]

There are a couple of instances of allocating memory with `malloc`,
followed by copying a string to it with `strncpy` and adding an explicit
assignment of `\0` to terminate the string.  Replace them with
`strndup`.

Add an enum to name indices of output keys.

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

diff --git a/filter/ulogd_filter_PWSNIFF.c b/filter/ulogd_filter_PWSNIFF.c
index 934ff0e09c4f..ef9e02115d84 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,
@@ -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 v4 08/32] input: UNIXSOCK: remove stat of socket-path

[Jeremy Sowden]

When creating the UNIX socket, there is a TOCTOU race between the
stat(2) and bind(2) calls, and if the path is already bound, the bind(2)
call will fail in any case.  Remove the stat(2) call.

Tidy up a couple of error message.

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

diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index 86ab590073d8..3f3abc3a0b77 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -477,12 +477,11 @@ static int _create_unix_socket(const char *unix_path)
 	int ret = -1;
 	struct sockaddr_un server_sock;
 	int s;
-	struct stat st_dummy;
 
 	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 +489,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 +501,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

[ulogd2 PATCH v4 09/32] input: UNIXSOCK: fix possible truncation of socket path

[Jeremy Sowden]

Verify that the socket 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 3f3abc3a0b77..66735e3ab4fe 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -475,9 +475,18 @@ 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;
 
+	if (strlen(unix_path) >= sizeof(server_sock.sun_path)) {
+		ulogd_log(ULOGD_ERROR,
+			  "ulogd2: unix socket path '%s' too long\n",
+			  unix_path);
+		return -1;
+	}
+
+	strcpy(server_sock.sun_path, unix_path);
+
 	s = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (s < 0) {
 		ulogd_log(ULOGD_ERROR,
@@ -485,10 +494,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 v4 10/32] 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.  We
only actually dereference the pointer to get the IP version, so replace
the pointer with a version variable and elsewhere use `pkt.payload`
directly.

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 66735e3ab4fe..5cf7cfb57703 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;
+	unsigned int ip_version = pkt->payload.version;
 	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], &pkt->payload);
 	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 = (void*)&pkt->payload + 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;
-			};
+			}
 		}
 	}
 
@@ -666,7 +666,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

[ulogd2 PATCH v4 11/32] output: DBI: fix deprecation warnings

[Jeremy Sowden]

The DBI output plugin uses some libdbi functions which have been
deprecated in favour of re-entrant equivalents.  Switch to the
re-entrant functions.

Remove superfluous `init` declaration.

Add destructor to clean up DBI instance on exit.

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

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index 23cc9c8fb492..b4a5bacd156f 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;
 
@@ -173,7 +175,6 @@ static int close_db_dbi(struct ulogd_pluginstance *upi)
 	ulogd_log(ULOGD_DEBUG, "dbi: closing connection\n");
 	dbi_conn_close(pi->dbh);
 	pi->dbh = NULL;
-	//dbi_shutdown();
 
 	return 0;
 }
@@ -195,14 +196,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);
@@ -316,11 +317,14 @@ static struct ulogd_plugin dbi_plugin = {
 	.version	= VERSION,
 };
 
-void __attribute__ ((constructor)) init(void);
-
-void init(void)
+void __attribute__ ((constructor)) init(void)
 {
-	dbi_initialize(NULL);
+	dbi_initialize_r(NULL, &libdbi_instance);
 
 	ulogd_register_plugin(&dbi_plugin);
 }
+
+void __attribute__ ((destructor)) fini(void)
+{
+	dbi_shutdown_r(libdbi_instance);
+}
-- 
2.33.0

[ulogd2 PATCH v4 12/32] output: DBI: improve mapping of DB columns to input-keys

[Jeremy Sowden]

Currently, we copy the column-name to a buffer, iterate over it to
replace the underscores with full-stops, using `strchr` from the start
of the buffer on each iteration, iterate over it a second time to
lower-case all letters, and finally copy the buffer to the input-key's
`name` member.

In addition to being inefficient, `strncpy` is used to do the copies,
which leads gcc to complain:

  ulogd_output_DBI.c:160:17: warning: `strncpy` output may be truncated copying 31 bytes from a string of length 31

Furthermore, the buffer is not initialized, which means that there is
also a possible buffer overrun if the column-name is too long, since
`strncpy` will not append a NUL.

Instead, copy the column-name directly to the input-key using
`snprintf`, and then iterate over it once to replace underscores and
lower-case letters.

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

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index b4a5bacd156f..fff9abc57ff6 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)
 {
@@ -140,24 +131,25 @@ static int get_columns_dbi(struct ulogd_pluginstance *upi)
 	}
 
 	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);
+		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 = '.';
-
-		str_tolower(buf);
+		snprintf(upi->input.keys[ui - 1].name,
+			 sizeof(upi->input.keys[ui - 1].name),
+			 "%s", field_name);
 
-		DEBUGP("field '%s' found: ", 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);
+		}
 
-		/* 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 */
-- 
2.33.0

[ulogd2 PATCH v4 13/32] output: DBI: fix NUL-termination of escaped SQL string

[Jeremy Sowden]

On error, `dbi_conn_quote_string_copy` returns zero.  In this case, we
need to set `*dst` to NUL.  Handle a return-value of `2` as normal
below.  `1` is never returned.

Replace `strncpy` with `memcpy`: using `strncpy` is nearly always a
mistake, and we don't need its special behaviour here.

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

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index fff9abc57ff6..57e3058036d9 100644
--- a/output/dbi/ulogd_output_DBI.c
+++ b/output/dbi/ulogd_output_DBI.c
@@ -236,18 +236,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 v4 14/32] output: DBI: fix configuration of DB connection

[Jeremy Sowden]

In `open_db_dbi`, we test whether various config-settings are defined
by comparing their string values to `NULL`.  However, the `u.string`
member of `struct config_entry` is an array, not a pointer, so it is
never `NULL`.  Instead, check whether the string is empty.

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

diff --git a/output/dbi/ulogd_output_DBI.c b/output/dbi/ulogd_output_DBI.c
index 57e3058036d9..5eee6b593b5b 100644
--- a/output/dbi/ulogd_output_DBI.c
+++ b/output/dbi/ulogd_output_DBI.c
@@ -175,10 +175,10 @@ static int close_db_dbi(struct ulogd_pluginstance *upi)
 static int open_db_dbi(struct ulogd_pluginstance *upi)
 {
 	struct dbi_instance *pi = (struct dbi_instance *) upi->private;
-	char *server = host_ce(upi->config_kset).u.string;
-	char *user = user_ce(upi->config_kset).u.string;
-	char *pass = pass_ce(upi->config_kset).u.string;
-	char *db = db_ce(upi->config_kset).u.string;
+	char *server = host_ce  (upi->config_kset).u.string;
+	char *user   = user_ce  (upi->config_kset).u.string;
+	char *pass   = pass_ce  (upi->config_kset).u.string;
+	char *db     = db_ce    (upi->config_kset).u.string;
 	char *dbtype = dbtype_ce(upi->config_kset).u.string;
 	dbi_driver driver;
 	int ret;
@@ -203,13 +203,13 @@ static int open_db_dbi(struct ulogd_pluginstance *upi)
 		return -1;
 	}
 
-	if (server)
+	if (server[0])
 		dbi_conn_set_option(pi->dbh, "host", server);
-	if (user)
+	if (user[0])
 		dbi_conn_set_option(pi->dbh, "username", user);
-	if (pass)
+	if (pass[0])
 		dbi_conn_set_option(pi->dbh, "password", pass);
-	if (db)
+	if (db[0])
 		dbi_conn_set_option(pi->dbh, "dbname", db);
 
 	ret = dbi_conn_connect(pi->dbh);
-- 
2.33.0

[ulogd2 PATCH v4 15/32] output: MYSQL: improve mapping of DB columns to input-keys

[Jeremy Sowden]

Currently, we copy the column-name to a buffer, iterate over it to
replace the underscores with full-stops, using `strchr` from the start
of the buffer on each iteration, then copy the buffer to the input-key's
`name` member.

Apart from the inefficiency, `strncpy` is used to do the copies, which
leads gcc to complain:

  ulogd_output_MYSQL.c:149:17: warning: `strncpy` output may be truncated copying 31 bytes from a string of length 31

Furthermore, the buffer is not initialized, which means that there is
also a possible buffer overrun if the column-name is too long, since
`strncpy` will not append a NUL.

Instead, copy the column-name directly to the input-key using
`snprintf`, and run `strchr` from the last underscore on each iteration.

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

diff --git a/output/mysql/ulogd_output_MYSQL.c b/output/mysql/ulogd_output_MYSQL.c
index 66151feb4939..9727e303f2c5 100644
--- a/output/mysql/ulogd_output_MYSQL.c
+++ b/output/mysql/ulogd_output_MYSQL.c
@@ -135,18 +135,18 @@ 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 v4 16/32] output: PGSQL: improve mapping of DB columns to input-keys

[Jeremy Sowden]

Currently, we copy the column-name to a buffer, iterate over it to
replace the underscores with full-stops, using `strchr` from the start
of the buffer on each iteration, then copy the buffer to the input-key's
`name` member.

Apart from the inefficiency, `strncpy` is used to do the copies, which
leads gcc to complain:

  ulogd_output_PGSQL.c:204:17: warning: `strncpy` output may be truncated copying 31 bytes from a string of length 31

Furthermore, the buffer is not initialized, which means that there is
also a possible buffer overrun if the column-name is too long, since
`strncpy` will not append a NUL.

Instead, copy the column-name directly to the input-key using
`snprintf`, and run `strchr` from the last underscore on each iteration.

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

diff --git a/output/pgsql/ulogd_output_PGSQL.c b/output/pgsql/ulogd_output_PGSQL.c
index f5a2823a7e1d..71d94031ac4e 100644
--- a/output/pgsql/ulogd_output_PGSQL.c
+++ b/output/pgsql/ulogd_output_PGSQL.c
@@ -190,18 +190,18 @@ 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 v4 17/32] output: PGSQL: fix non-`connstring` configuration of DB connection

[Jeremy Sowden]

In `open_db_pgsql`, we test whether various config-settings are defined
by comparing their string values to `NULL`.  However, the `u.string`
member of `struct config_entry` is an array, not a pointer, so it is
never `NULL`.  Instead, check whether the string is empty.

Use a pointer to the end of the `connstr` buffer and `sprintf`, rather
than repeated `strcat`s.

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

diff --git a/output/pgsql/ulogd_output_PGSQL.c b/output/pgsql/ulogd_output_PGSQL.c
index 71d94031ac4e..b52023273bc6 100644
--- a/output/pgsql/ulogd_output_PGSQL.c
+++ b/output/pgsql/ulogd_output_PGSQL.c
@@ -232,48 +232,38 @@ static int open_db_pgsql(struct ulogd_pluginstance *upi)
 	char *schema = NULL;
 	char pgbuf[128];
 
-	if (!connstr) {
-		char *server = host_ce(upi->config_kset).u.string;
-		unsigned int port = port_ce(upi->config_kset).u.value;
-		char *user = user_ce(upi->config_kset).u.string;
-		char *pass = pass_ce(upi->config_kset).u.string;
-		char *db = db_ce(upi->config_kset).u.string;
+	if (!connstr[0]) {
+		char         *server = host_ce(upi->config_kset).u.string;
+		unsigned int  port   = port_ce(upi->config_kset).u.value;
+		char         *user   = user_ce(upi->config_kset).u.string;
+		char         *pass   = pass_ce(upi->config_kset).u.string;
+		char         *db     = db_ce  (upi->config_kset).u.string;
+		char         *cp;
 		/* 80 is more than what we need for the fixed parts below */
 		len = 80 + strlen(user) + strlen(db);
 
 		/* hostname and  and password are the only optionals */
-		if (server)
+		if (server[0])
 			len += strlen(server);
-		if (pass)
+		if (pass[0])
 			len += strlen(pass);
 		if (port)
 			len += 20;
 
-		connstr = (char *) malloc(len);
+		cp = connstr = malloc(len);
 		if (!connstr)
 			return -ENOMEM;
-		connstr[0] = '\0';
 
-		if (server && strlen(server) > 0) {
-			strcpy(connstr, " host=");
-			strcat(connstr, server);
-		}
+		if (server[0])
+			cp += sprintf(cp, "host=%s ", server);
 
-		if (port) {
-			char portbuf[20];
-			snprintf(portbuf, sizeof(portbuf), " port=%u", port);
-			strcat(connstr, portbuf);
-		}
+		if (port)
+			cp += sprintf(cp, "port=%u ", port);
 
-		strcat(connstr, " dbname=");
-		strcat(connstr, db);
-		strcat(connstr, " user=");
-		strcat(connstr, user);
+		cp += sprintf(cp, "dbname=%s user=%s", db, user);
 
-		if (pass) {
-			strcat(connstr, " password=");
-			strcat(connstr, pass);
-		}
+		if (pass[0])
+			cp += sprintf(cp, " password=%s", pass);
 	}
 	pi->dbh = PQconnectdb(connstr);
 	if (PQstatus(pi->dbh) != CONNECTION_OK) {
-- 
2.33.0

[ulogd2 PATCH v4 18/32] output: SQLITE3: fix possible buffer overruns

[Jeremy Sowden]

There is a an off-by-one error in the size of some of the buffers used
to hold key-names.  The maximum length of a name is `ULOGD_MAX_KEYLEN`,
and so declare the buffers with size `ULOGD_MAX_KEYLEN + 1`.

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

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 20ceb3b5d6e2..554b1b34488c 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,7 +214,7 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 {
 	struct sqlite3_priv *priv = (void *)pi->private;
 	struct field *f;
-	char buf[ULOGD_MAX_KEYLEN];
+	char buf[ULOGD_MAX_KEYLEN + 1];
 	char *underscore;
 	char *stmt_pos;
 	int i, cols = 0;
@@ -305,7 +305,7 @@ static int
 sqlite3_init_db(struct ulogd_pluginstance *pi)
 {
 	struct sqlite3_priv *priv = (void *)pi->private;
-	char buf[ULOGD_MAX_KEYLEN];
+	char buf[ULOGD_MAX_KEYLEN + 1];
 	char *underscore;
 	struct field *f;
 	sqlite3_stmt *schema_stmt;
-- 
2.33.0

[ulogd2 PATCH v4 19/32] output: SQLITE3: fix memory-leak in error-handling

[Jeremy Sowden]

When mapping DB column names to input-keys, if we cannot find a key to
match a column, the newly allocated `struct field` is leaked.  Free it,
and log an error message.

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

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 554b1b34488c..41aeeec27854 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -342,8 +342,12 @@ sqlite3_init_db(struct ulogd_pluginstance *pi)
 		}
 		strncpy(f->name, buf, ULOGD_MAX_KEYLEN);
 
-		if ((f->key = ulogd_find_key(pi, buf)) == NULL)
+		if ((f->key = ulogd_find_key(pi, buf)) == NULL) {
+			ulogd_log(ULOGD_ERROR,
+				  "SQLITE3: unknown input key: %s\n", buf);
+			free(f);
 			return -1;
+		}
 
 		TAILQ_INSERT_TAIL(&priv->fields, f, link);
 	}
-- 
2.33.0

[ulogd2 PATCH v4 20/32] output: SQLITE3: improve formatting of insert statement

[Jeremy Sowden]

`sqlite3_createstmt` contains a variable `stmt_pos` which points to the
end of the SQL already written, where the next chunk should be appended.
Currently, this is assigned after every write:

  sprintf(stmt_pos, ...);
  stmt_pos = priv->stmt + strlen(priv->stmt);

However, since `sprintf` returns the number of bytes written, increment
`stmt_pos` by the return-value of `sprintf` in order to avoid the
repeated `strlen` calls.

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

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 41aeeec27854..da1c09f08047 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -226,9 +226,9 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 		ulogd_log(ULOGD_ERROR, "SQLITE3: out of memory\n");
 		return -1;
 	}
+	stmt_pos = priv->stmt;
 
-	sprintf(priv->stmt, "insert into %s (", table_ce(pi));
-	stmt_pos = priv->stmt + strlen(priv->stmt);
+	stmt_pos += sprintf(stmt_pos, "insert into %s (", table_ce(pi));
 
 	tailq_for_each(f, priv->fields, link) {
 		strncpy(buf, f->name, ULOGD_MAX_KEYLEN);
@@ -236,19 +236,17 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 		while ((underscore = strchr(buf, '.')))
 			*underscore = '_';
 
-		sprintf(stmt_pos, "%s,", buf);
-		stmt_pos = priv->stmt + strlen(priv->stmt);
+		stmt_pos += sprintf(stmt_pos, "%s,", buf);
 
 		cols++;
 	}
 
 	*(stmt_pos - 1) = ')';
 
-	sprintf(stmt_pos, " values (");
-	stmt_pos = priv->stmt + strlen(priv->stmt);
+	stmt_pos += sprintf(stmt_pos, " values (");
 
 	for (i = 0; i < cols - 1; i++) {
-		sprintf(stmt_pos,"?,");
+		strcpy(stmt_pos, "?,");
 		stmt_pos += 2;
 	}
 
-- 
2.33.0

[ulogd2 PATCH v4 21/32] output: SQLITE3: improve mapping of DB columns to fields

[Jeremy Sowden]

Currently, we copy the column-name to a buffer, iterate over it to
replace the underscores with full-stops, using `strchr` from the start
of the buffer on each iteration, then copy the buffer to the field's
`name` member.

Apart from the inefficiency, `strncpy` is used to do the copies, which
leads gcc to complain:

  ulogd_output_SQLITE3.c:341:17: warning: `strncpy` output may be truncated copying 31 bytes from a string of length 31

Furthermore, the buffer is not initialized, which means that there is
also a possible buffer overrun if the column-name is too long, since
`strncpy` will not append a NUL.

Instead, copy the column-name directly to the field using `snprintf`,
and run `strchr` from the last underscore on each iteration.

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

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index da1c09f08047..e3040a8a2fac 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -303,9 +303,6 @@ static int
 sqlite3_init_db(struct ulogd_pluginstance *pi)
 {
 	struct sqlite3_priv *priv = (void *)pi->private;
-	char buf[ULOGD_MAX_KEYLEN + 1];
-	char *underscore;
-	struct field *f;
 	sqlite3_stmt *schema_stmt;
 	int col, num_cols;
 
@@ -325,24 +322,27 @@ 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;
+		char *underscore;
 
 		/* 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));
+
+		/* replace all underscores with dots */
+		for (underscore = f->name;
+		     (underscore = strchr(underscore, '_')) != NULL; )
+			*underscore = '.';
+
+		DEBUGP("field '%s' found\n", f->name);
 
-		if ((f->key = ulogd_find_key(pi, buf)) == NULL) {
+		if ((f->key = ulogd_find_key(pi, f->name)) == NULL) {
 			ulogd_log(ULOGD_ERROR,
-				  "SQLITE3: unknown input key: %s\n", buf);
+				  "SQLITE3: unknown input key: %s\n", f->name);
 			free(f);
 			return -1;
 		}
-- 
2.33.0

[ulogd2 PATCH v4 22/32] output: SQLITE3: improve mapping of fields to DB columns

[Jeremy Sowden]

Currently, we derive a field-name by replacing all the underscores in a
DB column-name with full-stops and use the field-name to find the
matching input-key.  However, every time we create a new insert SQL
statement, we derive the column-names by copying the field-names to a
buffer, replacing all the full-stops with underscores, and then
appending the buffer containing the column-name to the one containing
the statments.

Apart from the inefficiency, `strncpy` is used to do the copies, which
leads gcc to complain:

  ulogd_output_SQLITE3.c:234:17: warning: `strncpy` output may be truncated copying 31 bytes from a string of length 31

Instead, leave the underscores in the field-name, but copy it once to a
buffer in which the underscores are replaced and use this to find the
input-key.

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

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index e3040a8a2fac..c61694a51d47 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -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 + 1];
-	char *underscore;
 	char *stmt_pos;
 	int i, cols = 0;
 
@@ -231,13 +229,7 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 	stmt_pos += sprintf(stmt_pos, "insert into %s (", table_ce(pi));
 
 	tailq_for_each(f, priv->fields, link) {
-		strncpy(buf, f->name, ULOGD_MAX_KEYLEN);
-
-		while ((underscore = strchr(buf, '.')))
-			*underscore = '_';
-
-		stmt_pos += sprintf(stmt_pos, "%s,", buf);
-
+		stmt_pos += sprintf(stmt_pos, "%s,", f->name);
 		cols++;
 	}
 
@@ -271,10 +263,15 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
 static struct ulogd_key *
 ulogd_find_key(struct ulogd_pluginstance *pi, const char *name)
 {
+	char buf[ULOGD_MAX_KEYLEN + 1] = "";
 	unsigned int i;
 
+	/* replace all underscores with dots */
+	for (i = 0; i < sizeof(buf) - 1 && name[i]; ++i)
+		buf[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, buf) == 0)
 			return &pi->input.keys[i];
 	}
 
@@ -323,7 +320,6 @@ sqlite3_init_db(struct ulogd_pluginstance *pi)
 
 	for (col = 0; col < num_cols; col++) {
 		struct field *f;
-		char *underscore;
 
 		/* prepend it to the linked list */
 		if ((f = calloc(1, sizeof(struct field))) == NULL) {
@@ -333,11 +329,6 @@ sqlite3_init_db(struct ulogd_pluginstance *pi)
 		snprintf(f->name, sizeof(f->name),
 			 "%s", sqlite3_column_name(schema_stmt, col));
 
-		/* replace all underscores with dots */
-		for (underscore = f->name;
-		     (underscore = strchr(underscore, '_')) != NULL; )
-			*underscore = '.';
-
 		DEBUGP("field '%s' found\n", f->name);
 
 		if ((f->key = ulogd_find_key(pi, f->name)) == NULL) {
-- 
2.33.0

[ulogd2 PATCH v4 23/32] output: SQLITE3: catch errors creating SQL statement

[Jeremy Sowden]

`sqlite3_createstmt` returns non-zero on error, but the return-value was
being ignored.  Change the calling code to check the return-value, log
an error message and propagate the error.

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 c61694a51d47..c03018155a6b 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;
 		}
 	}
@@ -250,8 +253,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");
@@ -387,7 +390,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 v4 24/32] db: improve formatting of insert statement

[Jeremy Sowden]

`sql_createstmt` contains a variable `stmt_val` which points to the end
of the SQL already written, where the next chunk should be appended.
Currently, this is assigned after every write:

  sprintf(stmt_val, ...);
  stmt_val = mi->stmt + strlen(mi->stmt);

However, since `sprintf` returns the number of bytes written, increment
`stmt_val` by the return-value of `sprintf` in order to avoid the
repeated `strlen` calls.

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

diff --git a/util/db.c b/util/db.c
index f0711146867f..2dbe0db2fbfe 100644
--- a/util/db.c
+++ b/util/db.c
@@ -67,7 +67,6 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 	unsigned int i;
 	char *table = table_ce(upi->config_kset).u.string;
 	char *procedure = procedure_ce(upi->config_kset).u.string;
-	char *stmt_val = NULL;
 
 	if (mi->stmt)
 		free(mi->stmt);
@@ -96,20 +95,21 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 	if (strncasecmp(procedure,"INSERT", strlen("INSERT")) == 0 &&
 	    (procedure[strlen("INSERT")] == '\0' ||
 			procedure[strlen("INSERT")] == ' ')) {
+		char *stmt_val = mi->stmt;
 		char buf[ULOGD_MAX_KEYLEN];
 		char *underscore;
 
 		if(procedure[6] == '\0') {
 			/* procedure == "INSERT" */
 			if (mi->schema)
-				sprintf(mi->stmt, "insert into %s.%s (", mi->schema, table);
+				stmt_val += sprintf(stmt_val,
+						    "insert into %s.%s (",
+						    mi->schema, table);
 			else
-				sprintf(mi->stmt, "insert into %s (", table);
-		}
-		else
-			sprintf(mi->stmt, "%s (", procedure);
-
-		stmt_val = mi->stmt + strlen(mi->stmt);
+				stmt_val += sprintf(stmt_val,
+						    "insert into %s (", table);
+		} else
+			stmt_val += sprintf(stmt_val, "%s (", procedure);
 
 		for (i = 0; i < upi->input.num_keys; i++) {
 			if (upi->input.keys[i].flags & ULOGD_KEYF_INACTIVE)
@@ -118,8 +118,7 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 			strncpy(buf, upi->input.keys[i].name, ULOGD_MAX_KEYLEN);	
 			while ((underscore = strchr(buf, '.')))
 				*underscore = '_';
-			sprintf(stmt_val, "%s,", buf);
-			stmt_val = mi->stmt + strlen(mi->stmt);
+			stmt_val += sprintf(stmt_val, "%s,", buf);
 		}
 		*(stmt_val - 1) = ')';
 
-- 
2.33.0

[ulogd2 PATCH v4 25/32] db: improve mapping of input-keys to DB columns

[Jeremy Sowden]

Currently, we copy the key-name to a buffer, iterate over it to replace
the full-stops with underscores, using `strchr` from the start of the
buffer on each iteration, then append the buffer to the SQL statement.

Apart from the inefficiency, `strncpy` is used to do the copies, which
leads gcc to complain:

  ../../util/db.c:118:25: warning: `strncpy` output may be truncated copying 31 bytes from a string of length 31

Furthermore, the buffer is one character too short and so there is the
possibility of overruns.

Instead, append the key-name directly to the statement using `sprintf`,
and run `strchr` from the last underscore on each iteration.

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

diff --git a/util/db.c b/util/db.c
index 2dbe0db2fbfe..339e39ef4797 100644
--- a/util/db.c
+++ b/util/db.c
@@ -96,8 +96,6 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 	    (procedure[strlen("INSERT")] == '\0' ||
 			procedure[strlen("INSERT")] == ' ')) {
 		char *stmt_val = mi->stmt;
-		char buf[ULOGD_MAX_KEYLEN];
-		char *underscore;
 
 		if(procedure[6] == '\0') {
 			/* procedure == "INSERT" */
@@ -112,13 +110,18 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
 			stmt_val += sprintf(stmt_val, "%s (", procedure);
 
 		for (i = 0; i < upi->input.num_keys; i++) {
+			char *underscore;
+
 			if (upi->input.keys[i].flags & ULOGD_KEYF_INACTIVE)
 				continue;
 
-			strncpy(buf, upi->input.keys[i].name, ULOGD_MAX_KEYLEN);	
-			while ((underscore = strchr(buf, '.')))
+			underscore = stmt_val;
+
+			stmt_val += sprintf(stmt_val, "%s,",
+					    upi->input.keys[i].name);
+
+			while ((underscore = strchr(underscore, '.')))
 				*underscore = '_';
-			stmt_val += sprintf(stmt_val, "%s,", buf);
 		}
 		*(stmt_val - 1) = ')';
 
-- 
2.33.0

[ulogd2 PATCH v4 26/32] db: simplify initialization of ring-buffer

[Jeremy Sowden]

Currently, `strncpy` is used to copy the SQL statement to the ring
buffer, passing the length of the source string, which leads gcc to
complain:

  ../../util/db.c:231:25: warning: `strncpy` specified bound depends on the length of the source argument

In fact, the ring buffer is sized to be a multiple of the size of the
SQL buffer, and the SQL is simply copied multiple times at increasing
offsets, so use `strcpy` instead.

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

diff --git a/util/db.c b/util/db.c
index 339e39ef4797..c1d24365239f 100644
--- a/util/db.c
+++ b/util/db.c
@@ -228,9 +228,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);
-- 
2.33.0

[ulogd2 PATCH v4 27/32] output: JSON: fix output of GMT offset

[Jeremy Sowden]

The compiler has two sets of complaints.  Firstly, `t->tm_gmtoffset` is
a `long int`, but it is being passed to `abs`, which leads to warnings
such as:

  ulogd_output_JSON.c:308:34: warning: absolute value function `abs` given an argument of type `long int` but has parameter of type `int` which may cause truncation of value

Secondly, it can't verify that the hour value derived from the offset
will in fact fit into `%02d`, thus:

  ulogd_output_JSON.c:306:37: warning: `%02d` directive output may be truncated writing between 2 and 6 bytes into a region of size 5

To remedy these, we now mod the offset by 86,400 and assign it to an `int`
before deriving the hour and minute values.

We also change the format-specifier for the hour value to `%+03d` which
causes a sign to be printed even if the value is positive, thus allowing
us not to specify the sign explicitly and to drop the `abs` call for the
hour value.

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

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index 6edfa902efaf..f5c065dd062a 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -302,11 +302,12 @@ static int json_interp(struct ulogd_pluginstance *upi)
 			now = time(NULL);
 		t = localtime_r(&now, &result);
 		if (unlikely(*opi->cached_tz = '\0' || t->tm_gmtoff != opi->cached_gmtoff)) {
+			int gmtoff   = t->tm_gmtoff % 86400;
+			int gmtoff_h = gmtoff / 3600;
+			int gmtoff_m = abs(gmtoff) / 60 % 60;
+
 			snprintf(opi->cached_tz, sizeof(opi->cached_tz),
-				 "%c%02d%02d",
-				 t->tm_gmtoff > 0 ? '+' : '-',
-				 abs(t->tm_gmtoff) / 60 / 60,
-				 abs(t->tm_gmtoff) / 60 % 60);
+				 "%+03d%02d", gmtoff_h, gmtoff_m);
 		}
 
 		if (pp_is_valid(inp, opi->usec_idx)) {
-- 
2.33.0

[ulogd2 PATCH v4 28/32] output: JSON: increase time-stamp buffer size

[Jeremy Sowden]

The output buffer for date-times is of sufficient size provided that we
don't get oversized integer values for any of the fields, which is a
reasonable assumption.  However, the compiler complains about possible
truncation, e.g.:

  ulogd_output_JSON.c:314:65: warning: `%06u` directive output may be truncated writing between 6 and 10 bytes into a region of size between 0 and 18
  ulogd_output_JSON.c:313:25: note: `snprintf` output between 27 and 88 bytes into a destination of size 38

Fix the warnings by increasing the buffer size.

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

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index f5c065dd062a..d949df6bb530 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)
 {
-- 
2.33.0

[ulogd2 PATCH v4 29/32] output: JSON: fix possible leak in error-handling.

[Jeremy Sowden]

The `realloc` extending the buffer containing the JSON to allow us to
insert a final new-line may fail.  Therefore, we need to assign the
return-value to a temporary variable or we will not able to free the
existing buffer on error.

Use the correct type for `buflen`.

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

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index d949df6bb530..6af872c64391 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();
@@ -338,8 +338,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;
@@ -392,7 +390,6 @@ static int json_interp(struct ulogd_pluginstance *upi)
 		}
 	}
 
-
 	buf = json_dumps(msg, 0);
 	json_decref(msg);
 	if (buf == NULL) {
@@ -400,11 +397,13 @@ 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;
 	}
+	buf = tmp;
 	strncat(buf, "\n", 1);
 	buflen++;
 
-- 
2.33.0

[ulogd2 PATCH v4 30/32] 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.

Fixes a compiler warning:

  logd_output_JSON.c:407:9: warning: `strncat` specified bound 1 equals source length

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

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index 6af872c64391..f60bd6ea51da 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -404,8 +404,8 @@ static int json_interp(struct ulogd_pluginstance *upi)
 		return ULOGD_IRET_ERR;
 	}
 	buf = tmp;
-	strncat(buf, "\n", 1);
-	buflen++;
+	buf[buflen++] = '\n';
+	buf[buflen]   = '\0';
 
 	if (opi->mode == JSON_MODE_FILE)
 		return json_interp_file(upi, buf);
-- 
2.33.0

[ulogd2 PATCH v4 31/32] 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 f60bd6ea51da..33428c96b84b 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);
@@ -155,14 +156,16 @@ static int _connect_socket_unix(struct ulogd_pluginstance *pi)
 	ulogd_log(ULOGD_DEBUG, "connecting to unix:%s\n",
 		  file_ce(pi->config_kset).u.string);
 
+	if (strlen(socket_path) >= sizeof(u_addr.sun_path))
+		return -1;
+
+	strcpy(u_addr.sun_path, socket_path);
+
 	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 (connect(sfd, (struct sockaddr *) &u_addr, sizeof(u_addr)) == -1) {
 		close(sfd);
 		return -1;
 	}
-- 
2.33.0

[ulogd2 PATCH v4 32/32] output: IPFIX: remove compiler attribute macros

[Jeremy Sowden]

The ipfix.h header includes three macros which expand to compiler attributes.
Presumably, at some point the definitions were one branch of an if-else
preprocessor conditional where the definitions in the other branch expanded to
nothing.  This is no longer the case.  Only one of the macros (`__packed`) is
used and the raw attribute is used elsewhere in the code-base.  Remove the
macros.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/ulogd/ulogd.h | 5 -----
 output/ipfix/ipfix.c  | 2 --
 output/ipfix/ipfix.h  | 8 ++++----
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/include/ulogd/ulogd.h b/include/ulogd/ulogd.h
index a487c8e70e37..092d9f521a70 100644
--- a/include/ulogd/ulogd.h
+++ b/include/ulogd/ulogd.h
@@ -28,11 +28,6 @@
 
 /* types without length */
 #define ULOGD_RET_NONE		0x0000
-#define __packed		__attribute__((packed))
-#define __noreturn		__attribute__((noreturn))
-#define __cold			__attribute__((cold))
-
-#define __packed		__attribute__((packed))
 
 #define ULOGD_RET_INT8		0x0001
 #define ULOGD_RET_INT16		0x0002
diff --git a/output/ipfix/ipfix.c b/output/ipfix/ipfix.c
index b2719fd1d8a3..e0b3440e1d1a 100644
--- a/output/ipfix/ipfix.c
+++ b/output/ipfix/ipfix.c
@@ -8,8 +8,6 @@
 /* These forward declarations are needed since ulogd.h doesn't like to be the first */
 #include <ulogd/linuxlist.h>
 
-#define __packed		__attribute__((packed))
-
 #include "ipfix.h"
 
 #include <ulogd/ulogd.h>
diff --git a/output/ipfix/ipfix.h b/output/ipfix/ipfix.h
index 93945fbd562b..b0f3ae64740f 100644
--- a/output/ipfix/ipfix.h
+++ b/output/ipfix/ipfix.h
@@ -19,7 +19,7 @@ struct ipfix_hdr {
 	uint32_t seqno;
 	uint32_t oid;				/* Observation Domain ID */
 	uint8_t data[];
-} __packed;
+} __attribute__((packed));
 
 #define IPFIX_HDRLEN		sizeof(struct ipfix_hdr)
 
@@ -32,7 +32,7 @@ struct ipfix_templ_hdr {
 	uint16_t tid;
 	uint16_t cnt;
 	uint8_t data[];
-} __packed;
+} __attribute__((packed));
 
 #define IPFIX_TEMPL_HDRLEN(nfields)	sizeof(struct ipfix_templ_hdr) + (sizeof(uint16_t) * 2 * nfields)
 
@@ -42,7 +42,7 @@ struct ipfix_set_hdr {
 	uint16_t id;
 	uint16_t len;
 	uint8_t data[];
-} __packed;
+} __attribute__((packed));
 
 #define IPFIX_SET_HDRLEN		sizeof(struct ipfix_set_hdr)
 
@@ -67,7 +67,7 @@ struct vy_ipfix_data {
 	uint16_t dport;
 	uint8_t l4_proto;
 	uint32_t aid;				/* Application ID */
-} __packed;
+} __attribute__((packed));
 
 #define VY_IPFIX_SID		256
 
-- 
2.33.0

Re: [ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[Pablo Neira Ayuso]

On Tue, Nov 30, 2021 at 10:55:28AM +0000, Jeremy Sowden wrote:
> This patch-set fixes all the warnings reported by gcc 11.
> 
> Most of the warnings concern fall-throughs in switches, possibly
> problematic uses of functions like `strncpy` and `strncat` and possible
> truncation of output by `sprintf` and its siblings.
> 
> Some of the patches fix bugs revealed by warnings, some tweak code to
> avoid warnings, others fix or improve things I noticed while looking at
> the warnings.
> 
> Changes since v3:
> 
>   * When publishing v3 I accidentally sent out two different versions of the
>     patch-set under one cover-letter.  There are no code-changes in v4: it just
>     omits the earlier superseded patches.

Applied from 1 to 19 (all inclusive)

Thanks.

Re: [ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[Pablo Neira Ayuso]

On Mon, Dec 06, 2021 at 11:21:01PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Nov 30, 2021 at 10:55:28AM +0000, Jeremy Sowden wrote:
> > This patch-set fixes all the warnings reported by gcc 11.
> > 
> > Most of the warnings concern fall-throughs in switches, possibly
> > problematic uses of functions like `strncpy` and `strncat` and possible
> > truncation of output by `sprintf` and its siblings.
> > 
> > Some of the patches fix bugs revealed by warnings, some tweak code to
> > avoid warnings, others fix or improve things I noticed while looking at
> > the warnings.
> > 
> > Changes since v3:
> > 
> >   * When publishing v3 I accidentally sent out two different versions of the
> >     patch-set under one cover-letter.  There are no code-changes in v4: it just
> >     omits the earlier superseded patches.
> 
> Applied from 1 to 19 (all inclusive)

Applied remaining patches with comments.

- Patch #20, #24 maybe consider conversion to snprintf at some point, not
  your fault, this code is using sprintf in many spots. I think the
  only problematic scenario which might trigger problems is the
  configuration path using too long object names.

- Patch #21, #22 and #25, maybe consolidate this database field from
  _ to . in a common function.

- Patch #27, tm_gmtoff mod 86400 is really required? tm_gmtoff can be
  either -12/+12 * 60 * 60, simple assignment to integer should calm
  down the compiler?

- Patch #80, I guess you picked 80 just to provide a sufficiently
  large buffer to calm down compiler.

- Patch #31: I have replaced this patch with a check from .start and
  .signal paths to validate the unix socket path. The signal path of
  ulogd2 is problematic since configuration file errors should
  likely stop the daemon. I'll post it after this email.

- Patch #32: this IPFIX plugin was tested with wireshark according to
  4f639231c83b ("IPFIX: Add IPFIX output plugin"), I wonder if this
  attribute((packed)) is breaking anything, or maybe this was all
  tested on 32-bit?

Anyway, after this update it's probably better to look at using
pkg-config in the build system.

Thanks for fixing up these compiler warnings.

Re: [ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[Jeremy Sowden]

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

On 2022-01-03, at 19:10:13 +0100, Pablo Neira Ayuso wrote:
> On Mon, Dec 06, 2021 at 11:21:01PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Nov 30, 2021 at 10:55:28AM +0000, Jeremy Sowden wrote:
> > > This patch-set fixes all the warnings reported by gcc 11.
> > >
> > > Most of the warnings concern fall-throughs in switches, possibly
> > > problematic uses of functions like `strncpy` and `strncat` and
> > > possible truncation of output by `sprintf` and its siblings.
> > >
> > > Some of the patches fix bugs revealed by warnings, some tweak code
> > > to avoid warnings, others fix or improve things I noticed while
> > > looking at the warnings.
> > >
> > > Changes since v3:
> > >
> > >   * When publishing v3 I accidentally sent out two different
> > >     versions of the patch-set under one cover-letter.  There are
> > >     no code-changes in v4: it just omits the earlier superseded
> > >     patches.
> >
> > Applied from 1 to 19 (all inclusive)
>
> Applied remaining patches with comments.
>
> - Patch #20, #24 maybe consider conversion to snprintf at some point,
>   not your fault, this code is using sprintf in many spots. I think
>   the only problematic scenario which might trigger problems is the
>   configuration path using too long object names.

Yeah, there definitely were other places where I started to make
changes, but held off to stop the patch-set becoming even bigger than it
already was and focus on fixing the warnings.

> - Patch #21, #22 and #25, maybe consolidate this database field from
>   _ to . in a common function.

I've got the beginnings of a patch-set to do some tidying of the DB API.

There's an unused local variable left in the SQLITE3 plug-in.  I've
attached a patch to remove it.

> - Patch #27, tm_gmtoff mod 86400 is really required? tm_gmtoff can be
>   either -12/+12 * 60 * 60, simple assignment to integer should calm
>   down the compiler?

The compiler wasn't smart enough to know that the range of tm_gmtoff is
±43200, so it couldn't work out that the hours would fit in `%+03d`
without the mod.

> - Patch #80, I guess you picked 80 just to provide a sufficiently
>   large buffer to calm down compiler.

Correct.

> - Patch #31: I have replaced this patch with a check from .start and
>   .signal paths to validate the unix socket path. The signal path of
>   ulogd2 is problematic since configuration file errors should
>   likely stop the daemon. I'll post it after this email.

Looks good.

> - Patch #32: this IPFIX plugin was tested with wireshark according to
>   4f639231c83b ("IPFIX: Add IPFIX output plugin"), I wonder if this
>   attribute((packed)) is breaking anything, or maybe this was all
>   tested on 32-bit?

I'll take a look.

> Anyway, after this update it's probably better to look at using
> pkg-config in the build system.

J.

[-- Attachment #1.2: 0001-output-SQLITE3-remove-unused-variable.patch --]
[-- Type: text/x-diff, Size: 943 bytes --]

From e45879a7ea5529c26f369c297295332143ee8420 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Wed, 5 Jan 2022 22:37:21 +0000
Subject: [PATCH] output: SQLITE3: remove unused variable

There's local variable left over from a previous tidy-up.  Remove it.

Fixes: 67b0be90f16f ("output: SQLITE3: improve mapping of fields to DB columns")
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/sqlite3/ulogd_output_SQLITE3.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 51eab782cc9d..0a9ad67edcff 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -320,7 +320,6 @@ sqlite3_init_db(struct ulogd_pluginstance *pi)
 	}
 
 	for (col = 0; col < num_cols; col++) {
-		char *underscore;
 		struct field *f;
 
 		/* prepend it to the linked list */
-- 
2.34.1


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

Re: [ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[Pablo Neira Ayuso]

On Wed, Jan 05, 2022 at 10:48:30PM +0000, Jeremy Sowden wrote:
> From e45879a7ea5529c26f369c297295332143ee8420 Mon Sep 17 00:00:00 2001
> From: Jeremy Sowden <jeremy@azazel.net>
> Date: Wed, 5 Jan 2022 22:37:21 +0000
> Subject: [PATCH] output: SQLITE3: remove unused variable
> 
> There's local variable left over from a previous tidy-up.  Remove it.

Applied, thanks

> Fixes: 67b0be90f16f ("output: SQLITE3: improve mapping of fields to DB columns")
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  output/sqlite3/ulogd_output_SQLITE3.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
> index 51eab782cc9d..0a9ad67edcff 100644
> --- a/output/sqlite3/ulogd_output_SQLITE3.c
> +++ b/output/sqlite3/ulogd_output_SQLITE3.c
> @@ -320,7 +320,6 @@ sqlite3_init_db(struct ulogd_pluginstance *pi)
>  	}
>  
>  	for (col = 0; col < num_cols; col++) {
> -		char *underscore;
>  		struct field *f;
>  
>  		/* prepend it to the linked list */
> -- 
> 2.34.1
> 

Re: [ulogd2 PATCH v4 00/32] Fixes for compiler warnings

[Jeremy Sowden]

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

On 2022-01-03, at 19:10:13 +0100, Pablo Neira Ayuso wrote:
> Anyway, after this update it's probably better to look at using
> pkg-config in the build system.

Thought I'd already done this.  Hadn't posted the patches.  Will do
that shortly.

J.

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