[PATCH ulogd2 0/4] Some bug-fixes

[PATCH ulogd2 0/4] Some bug-fixes

[Jeremy Sowden]

As requested by Pablo, I've broken up the 34-part "Refactor of the DB
output plug-ins" patch-series I sent out last month into smaller chunks.
This first set contains four unrelated bug-fixes.

Jeremy Sowden (4):
  ulogd: fix parse-error check
  filter: fix buffer sizes in filter plug-ins
  JSON: remove incorrect config value check
  db: fix back-log capacity checks

 filter/ulogd_filter_HWHDR.c   |  4 ++--
 filter/ulogd_filter_IP2BIN.c  | 14 +++++++-------
 filter/ulogd_filter_IP2HBIN.c |  2 +-
 filter/ulogd_filter_IP2STR.c  |  6 +++---
 output/ulogd_output_JSON.c    |  8 +++-----
 src/ulogd.c                   |  2 +-
 util/db.c                     | 11 +++++++----
 7 files changed, 24 insertions(+), 23 deletions(-)

-- 
2.35.1

[PATCH ulogd2 1/4] ulogd: fix parse-error check

[Jeremy Sowden]

If `config_parse_file` returns `-ERRTOOLONG`, `config_errce` may be
`NULL`.  However, the calling function checks whether
`config_errce->key` is `NULL` instead.

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 b02f2602a895..8ea9793ec0fb 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1134,7 +1134,7 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
 				"section \"%s\" not found\n", section);
 			break;
 		case -ERRTOOLONG:
-			if (config_errce->key)
+			if (config_errce)
 				ulogd_log(ULOGD_ERROR,
 					  "string value too long for key \"%s\"\n",
 					  config_errce->key);
-- 
2.35.1

[PATCH ulogd2 2/4] filter: fix buffer sizes in filter plug-ins

[Jeremy Sowden]

Three of the filter plug-ins define arrays to hold output key values.
The arrays are sized based on the values of enums.  For example:

  enum output_keys {
    KEY_MAC_TYPE,
    KEY_MAC_PROTOCOL,
    KEY_MAC_SADDR,
    START_KEY = KEY_MAC_SADDR,
    KEY_MAC_DADDR,
    KEY_MAC_ADDR,
    MAX_KEY = KEY_MAC_ADDR,
  };

  static char hwmac_str[MAX_KEY - START_KEY][HWADDR_LENGTH];

The arrays are indexed by subtracting `START_KEY` from the enum value of
the key currently being processed: `hwmac_str[okey - START_KEY]`.
However, this means that the last key (`KEY_MAC_ADDR` in this example)
will run off the end of the array.  Increase the size of the arrays.

In the case of `IP2BIN` and `IP2HBIN`, there is no overrun, but only
because they use the wrong upper bound when looping over the keys, and
thus don't assign a value to the last key.  Correct the bound.

Also some small white-space tweaks.

Link: https://bugzilla.netfilter.org/show_bug.cgi?id=890
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 filter/ulogd_filter_HWHDR.c   |  4 ++--
 filter/ulogd_filter_IP2BIN.c  | 14 +++++++-------
 filter/ulogd_filter_IP2HBIN.c |  2 +-
 filter/ulogd_filter_IP2STR.c  |  6 +++---
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/filter/ulogd_filter_HWHDR.c b/filter/ulogd_filter_HWHDR.c
index bbca5e9b92f2..a5ee60dea44b 100644
--- a/filter/ulogd_filter_HWHDR.c
+++ b/filter/ulogd_filter_HWHDR.c
@@ -109,7 +109,7 @@ static struct ulogd_key mac2str_keys[] = {
 	},
 };
 
-static char hwmac_str[MAX_KEY - START_KEY][HWADDR_LENGTH];
+static char hwmac_str[MAX_KEY - START_KEY + 1][HWADDR_LENGTH];
 
 static int parse_mac2str(struct ulogd_key *ret, unsigned char *mac,
 			 int okey, int len)
@@ -126,7 +126,7 @@ static int parse_mac2str(struct ulogd_key *ret, unsigned char *mac,
 	buf_cur = hwmac_str[okey - START_KEY];
 	for (i = 0; i < len; i++)
 		buf_cur += sprintf(buf_cur, "%02x%c", mac[i],
-				i == len - 1 ? 0 : ':');
+				   i == len - 1 ? 0 : ':');
 
 	okey_set_ptr(&ret[okey], hwmac_str[okey - START_KEY]);
 
diff --git a/filter/ulogd_filter_IP2BIN.c b/filter/ulogd_filter_IP2BIN.c
index 2172d93506d5..42bcd7c15f1b 100644
--- a/filter/ulogd_filter_IP2BIN.c
+++ b/filter/ulogd_filter_IP2BIN.c
@@ -114,7 +114,7 @@ static struct ulogd_key ip2bin_keys[] = {
 
 };
 
-static char ipbin_array[MAX_KEY-START_KEY][IPADDR_LENGTH];
+static char ipbin_array[MAX_KEY - START_KEY + 1][IPADDR_LENGTH];
 
 /**
  * Convert IPv4 address (as 32-bit unsigned integer) to IPv6 address:
@@ -128,7 +128,7 @@ static inline void uint32_to_ipv6(const uint32_t ipv4, struct in6_addr *ipv6)
 	ipv6->s6_addr32[3] = ipv4;
 }
 
-static int ip2bin(struct ulogd_key* inp, int index, int oindex)
+static int ip2bin(struct ulogd_key *inp, int index, int oindex)
 {
 	char family = ikey_get_u8(&inp[KEY_OOB_FAMILY]);
 	char convfamily = family;
@@ -184,7 +184,7 @@ static int ip2bin(struct ulogd_key* inp, int index, int oindex)
 	addr8 = &addr->s6_addr[0];
 	for (i = 0; i < 4; i++) {
 		written = sprintf(buffer, "%02x%02x%02x%02x",
-				addr8[0], addr8[1], addr8[2], addr8[3]);
+				  addr8[0], addr8[1], addr8[2], addr8[3]);
 		if (written != 2 * 4) {
 			buffer[0] = 0;
 			return ULOGD_IRET_ERR;
@@ -205,13 +205,13 @@ static int interp_ip2bin(struct ulogd_pluginstance *pi)
 	int fret;
 
 	/* Iter on all addr fields */
-	for(i = START_KEY; i < MAX_KEY; i++) {
+	for(i = START_KEY; i <= MAX_KEY; i++) {
 		if (pp_is_valid(inp, i)) {
-			fret = ip2bin(inp, i, i-START_KEY);
+			fret = ip2bin(inp, i, i - START_KEY);
 			if (fret != ULOGD_IRET_OK)
 				return fret;
-			okey_set_ptr(&ret[i-START_KEY],
-				     ipbin_array[i-START_KEY]);
+			okey_set_ptr(&ret[i - START_KEY],
+				     ipbin_array[i - START_KEY]);
 		}
 	}
 
diff --git a/filter/ulogd_filter_IP2HBIN.c b/filter/ulogd_filter_IP2HBIN.c
index 087e824ae94b..2711f9c3e12a 100644
--- a/filter/ulogd_filter_IP2HBIN.c
+++ b/filter/ulogd_filter_IP2HBIN.c
@@ -153,7 +153,7 @@ static int interp_ip2hbin(struct ulogd_pluginstance *pi)
 	}
 
 	/* Iter on all addr fields */
-	for(i = START_KEY; i < MAX_KEY; i++) {
+	for(i = START_KEY; i <= MAX_KEY; i++) {
 		if (pp_is_valid(inp, i)) {
 			switch (convfamily) {
 			case AF_INET:
diff --git a/filter/ulogd_filter_IP2STR.c b/filter/ulogd_filter_IP2STR.c
index 66324b0b3b22..4d0536817b6c 100644
--- a/filter/ulogd_filter_IP2STR.c
+++ b/filter/ulogd_filter_IP2STR.c
@@ -137,7 +137,7 @@ static struct ulogd_key ip2str_keys[] = {
 	},
 };
 
-static char ipstr_array[MAX_KEY-START_KEY][IPADDR_LENGTH];
+static char ipstr_array[MAX_KEY - START_KEY + 1][IPADDR_LENGTH];
 
 static int ip2str(struct ulogd_key *inp, int index, int oindex)
 {
@@ -197,10 +197,10 @@ static int interp_ip2str(struct ulogd_pluginstance *pi)
 	/* Iter on all addr fields */
 	for (i = START_KEY; i <= MAX_KEY; i++) {
 		if (pp_is_valid(inp, i)) {
-			fret = ip2str(inp, i, i-START_KEY);
+			fret = ip2str(inp, i, i - START_KEY);
 			if (fret != ULOGD_IRET_OK)
 				return fret;
-			okey_set_ptr(&ret[i-START_KEY],
+			okey_set_ptr(&ret[i - START_KEY],
 				     ipstr_array[i-START_KEY]);
 		}
 	}
-- 
2.35.1

[PATCH ulogd2 3/4] JSON: remove incorrect config value check

[Jeremy Sowden]

The `u.string` member of a config entry is an array, and so never `NULL`.
Output the device string unconditionally.

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

diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index bbc3dba5d41a..700abc25e5ea 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -277,7 +277,7 @@ static int json_interp(struct ulogd_pluginstance *upi)
 {
 	struct json_priv *opi = (struct json_priv *) &upi->private;
 	unsigned int i;
-	char *buf, *tmp;
+	char *dvc, *buf, *tmp;
 	size_t buflen;
 	json_t *msg;
 
@@ -335,10 +335,8 @@ static int json_interp(struct ulogd_pluginstance *upi)
 			json_object_set_new(msg, "timestamp", json_string(timestr));
 	}
 
-	if (upi->config_kset->ces[JSON_CONF_DEVICE].u.string) {
-		char *dvc = upi->config_kset->ces[JSON_CONF_DEVICE].u.string;
-		json_object_set_new(msg, "dvc", json_string(dvc));
-	}
+	dvc = upi->config_kset->ces[JSON_CONF_DEVICE].u.string;
+	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;
-- 
2.35.1

[PATCH ulogd2 4/4] db: fix back-log capacity checks

[Jeremy Sowden]

Hitherto, when adding queries to the back-log, the memory usage has been
incremented and decremented by the size of the query structure and the
length of the SQL statement, `sizeof(struct db_stmt) + len`.  However,
when checking whether there is available capacity to add a new query,
the struct size has been ignored.  Amend the check to include the struct
size, and also account for the NUL that terminates the SQL.

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

diff --git a/util/db.c b/util/db.c
index c1d24365239f..ebd9f152ed83 100644
--- a/util/db.c
+++ b/util/db.c
@@ -404,14 +404,17 @@ static void __format_query_db(struct ulogd_pluginstance *upi, char *start)
 static int __add_to_backlog(struct ulogd_pluginstance *upi, const char *stmt, unsigned int len)
 {
 	struct db_instance *di = (struct db_instance *) &upi->private;
+	unsigned int query_size;
 	struct db_stmt *query;
 
 	/* check if we are using backlog */
 	if (di->backlog_memcap == 0)
 		return 0;
 
+	query_size = sizeof(*query) + len + 1;
+
 	/* check len against backlog */
-	if (len + di->backlog_memusage > di->backlog_memcap) {
+	if (query_size + di->backlog_memcap - di->backlog_memusage) {
 		if (di->backlog_full == 0)
 			ulogd_log(ULOGD_ERROR,
 				  "Backlog is full starting to reject events.\n");
@@ -419,7 +422,7 @@ static int __add_to_backlog(struct ulogd_pluginstance *upi, const char *stmt, un
 		return -1;
 	}
 
-	query = malloc(sizeof(struct db_stmt));
+	query = malloc(sizeof(*query));
 	if (query == NULL)
 		return -1;
 
@@ -431,7 +434,7 @@ static int __add_to_backlog(struct ulogd_pluginstance *upi, const char *stmt, un
 		return -1;
 	}
 
-	di->backlog_memusage += len + sizeof(struct db_stmt);
+	di->backlog_memusage += query_size;
 	di->backlog_full = 0;
 
 	llist_add_tail(&query->list, &di->backlog);
@@ -489,7 +492,7 @@ static int __treat_backlog(struct ulogd_pluginstance *upi)
 			di->driver->close_db(upi);
 			return _init_reconnect(upi);
 		} else {
-			di->backlog_memusage -= query->len + sizeof(struct db_stmt);
+			di->backlog_memusage -= sizeof(*query) + query->len + 1;
 			llist_del(&query->list);
 			free(query->stmt);
 			free(query);
-- 
2.35.1

Re: [PATCH ulogd2 0/4] Some bug-fixes

[Pablo Neira Ayuso]

On Sat, Dec 03, 2022 at 07:02:08PM +0000, Jeremy Sowden wrote:
> As requested by Pablo, I've broken up the 34-part "Refactor of the DB
> output plug-ins" patch-series I sent out last month into smaller chunks.
> This first set contains four unrelated bug-fixes.

Applied to ulogd2, thanks

Re: [PATCH ulogd2 2/4] filter: fix buffer sizes in filter plug-ins

[Pablo Neira Ayuso]

On Sat, Dec 03, 2022 at 07:02:10PM +0000, Jeremy Sowden wrote:
[...]
> The arrays are indexed by subtracting `START_KEY` from the enum value of
> the key currently being processed: `hwmac_str[okey - START_KEY]`.
> However, this means that the last key (`KEY_MAC_ADDR` in this example)
> will run off the end of the array.  Increase the size of the arrays.

BTW, did you detect this via valgrind or such? If so, posting an
extract of the splat in the commit message is good to have.

Just a side note for your follow up patches, this batch is already on
git.netfilter.org

Thanks.

Re: [PATCH ulogd2 2/4] filter: fix buffer sizes in filter plug-ins

[Jeremy Sowden]

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

On 2022-12-08, at 22:07:33 +0100, Pablo Neira Ayuso wrote:
> On Sat, Dec 03, 2022 at 07:02:10PM +0000, Jeremy Sowden wrote:
> [...]
> > The arrays are indexed by subtracting `START_KEY` from the enum
> > value of the key currently being processed: `hwmac_str[okey -
> > START_KEY]`.  However, this means that the last key (`KEY_MAC_ADDR`
> > in this example) will run off the end of the array.  Increase the
> > size of the arrays.
>
> BTW, did you detect this via valgrind or such? If so, posting an
> extract of the splat in the commit message is good to have.

One of the GCC sanitizers, IIRC.  I'll make sure to include a trace in
future.

J.

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