[PATCH ulogd2 1/8] ulogd: fix config file fd leak

[PATCH ulogd2 1/8] ulogd: fix config file fd leak

[Corubba Smith]

Consistently use the return jump to close the config file descriptor if
opened, to prevent it from leaking.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 src/conffile.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/conffile.c b/src/conffile.c
index 66769de..5b7f834 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -143,7 +143,8 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 		/* if line was fetch completely, string ends with '\n' */
 		if (! strchr(line, '\n')) {
 			ulogd_log(ULOGD_ERROR, "line %d too long.\n", linenum);
-			return -ERRTOOLONG;
+			err = -ERRTOOLONG;
+			goto cpf_error;
 		}

 		if (!(wordend = get_word(line, " \t\n\r[]", (char *) wordbuf)))
@@ -156,8 +157,8 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 	}

 	if (!found) {
-		fclose(cfile);
-		return -ERRSECTION;
+		err = -ERRSECTION;
+		goto cpf_error;
 	}

 	/* Parse this section until next section */
@@ -175,7 +176,8 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 		/* if line was fetch completely, string ends with '\n' */
 		if (! strchr(line, '\n')) {
 			ulogd_log(ULOGD_ERROR, "line %d too long.\n", linenum);
-			return -ERRTOOLONG;
+			err = -ERRTOOLONG;
+			goto cpf_error;
 		}

 		if (!(wordend = get_word(line, " =\t\n\r", (char *) &wordbuf)))
--
2.48.1

[PATCH ulogd2 2/8] ulogd: add ulogd_parse_configfile public function

[Corubba Smith]

Provide a new function `ulogd_parse_configfile()` in the public
interface, which wraps `parse_config_file()` to parse a section of the
config file and communicates found errors to the user. It can be used
as a drop-in replacement because arguments and return value are
compatible.

This relieves plugins of the need to translate the individual error
codes to human readable messages, and plugins are mostly interested if
there is any error, not what specific error.

This reuses the existing `parse_conffile()` function with slight
adjustments.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 include/ulogd/ulogd.h |   1 +
 src/ulogd.c           | 104 ++++++++++++++++++++++--------------------
 2 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/include/ulogd/ulogd.h b/include/ulogd/ulogd.h
index c7cf402..088d85d 100644
--- a/include/ulogd/ulogd.h
+++ b/include/ulogd/ulogd.h
@@ -362,6 +362,7 @@ void __ulogd_log(int level, char *file, int line, const char *message, ...)

 int ulogd_key_size(struct ulogd_key *key);
 int ulogd_wildcard_inputkeys(struct ulogd_pluginstance *upi);
+int ulogd_parse_configfile(const char *section, struct config_keyset *ce);

 /***********************************************************************
  * file descriptor handling
diff --git a/src/ulogd.c b/src/ulogd.c
index 6c5ff9a..80e1ac0 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -251,6 +251,60 @@ int ulogd_wildcard_inputkeys(struct ulogd_pluginstance *upi)
 	return 0;
 }

+/**
+ * Parse the given section in the config file into the given keyset.
+ * Returns ULOGD_IRET_OK on success, ULOGD_IRET_ERR on error.
+ * If an error occurs, writes a descriptive message to the log.
+ */
+int ulogd_parse_configfile(const char *section, struct config_keyset *ce)
+{
+	int err;
+
+	err = config_parse_file(section, ce);
+
+	switch(err) {
+		case 0:
+			return ULOGD_IRET_OK;
+			break;
+		case -ERROPEN:
+			ulogd_log(ULOGD_ERROR,
+			          "unable to open configfile: %s\n",
+			          ulogd_configfile);
+			break;
+		case -ERRMAND:
+			ulogd_log(ULOGD_ERROR,
+			          "mandatory option \"%s\" not found\n",
+			          config_errce->key);
+			break;
+		case -ERRMULT:
+			ulogd_log(ULOGD_ERROR,
+			          "option \"%s\" occurred more than once\n",
+			          config_errce->key);
+			break;
+		case -ERRUNKN:
+			ulogd_log(ULOGD_ERROR,
+			          "unknown config key \"%s\"\n",
+			          config_errce->key);
+			break;
+		case -ERRSECTION:
+			ulogd_log(ULOGD_ERROR,
+			          "section \"%s\" not found\n",
+			          section);
+			break;
+		case -ERRTOOLONG:
+			if (config_errce != NULL)
+				ulogd_log(ULOGD_ERROR,
+				          "string value too long for key \"%s\"\n",
+				          config_errce->key);
+			else
+				ulogd_log(ULOGD_ERROR,
+				          "string value is too long\n");
+			break;
+	}
+
+	return ULOGD_IRET_ERR;
+}
+

 /***********************************************************************
  * PLUGIN MANAGEMENT
@@ -1098,54 +1152,6 @@ static int logfile_open(const char *name)
 	return 0;
 }

-/* wrapper to handle conffile error codes */
-static int parse_conffile(const char *section, struct config_keyset *ce)
-{
-	int err;
-
-	err = config_parse_file(section, ce);
-
-	switch(err) {
-		case 0:
-			return 0;
-			break;
-		case -ERROPEN:
-			ulogd_log(ULOGD_ERROR,
-				"unable to open configfile: %s\n",
-				ulogd_configfile);
-			break;
-		case -ERRMAND:
-			ulogd_log(ULOGD_ERROR,
-				"mandatory option \"%s\" not found\n",
-				config_errce->key);
-			break;
-		case -ERRMULT:
-			ulogd_log(ULOGD_ERROR,
-				"option \"%s\" occurred more than once\n",
-				config_errce->key);
-			break;
-		case -ERRUNKN:
-			ulogd_log(ULOGD_ERROR,
-				"unknown config key \"%s\"\n",
-				config_errce->key);
-			break;
-		case -ERRSECTION:
-			ulogd_log(ULOGD_ERROR,
-				"section \"%s\" not found\n", section);
-			break;
-		case -ERRTOOLONG:
-			if (config_errce)
-				ulogd_log(ULOGD_ERROR,
-					  "string value too long for key \"%s\"\n",
-					  config_errce->key);
-			else
-				ulogd_log(ULOGD_ERROR,
-					  "string value is too long\n");
-			break;
-	}
-	return 1;
-}
-
 /*
  * Apply F_WRLCK to fd using fcntl().
  *
@@ -1594,7 +1600,7 @@ int main(int argc, char* argv[])
 	}

 	/* parse config file */
-	if (parse_conffile("global", &ulogd_kset)) {
+	if (ulogd_parse_configfile("global", &ulogd_kset)) {
 		ulogd_log(ULOGD_FATAL, "unable to parse config file\n");
 		warn_and_exit(daemonize);
 	}
--
2.48.1

[PATCH ulogd2 3/8] all: use config_parse_file function in all plugins

[Corubba Smith]

Replace all usages of `config_parse_file()` in plugins with the new
`ulogd_parse_configfile()` function, adding error handling where it was
missing. I used the same codestyle as the surrounding code, which varies
between plugins.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 filter/ulogd_filter_MARK.c            | 3 +--
 input/flow/ulogd_inpflow_NFCT.c       | 2 +-
 input/packet/ulogd_inppkt_NFLOG.c     | 3 +--
 input/packet/ulogd_inppkt_ULOG.c      | 2 +-
 input/packet/ulogd_inppkt_UNIXSOCK.c  | 3 +--
 input/sum/ulogd_inpflow_NFACCT.c      | 2 +-
 output/ipfix/ulogd_output_IPFIX.c     | 2 +-
 output/pcap/ulogd_output_PCAP.c       | 2 +-
 output/sqlite3/ulogd_output_SQLITE3.c | 3 ++-
 output/ulogd_output_GPRINT.c          | 2 +-
 output/ulogd_output_GRAPHITE.c        | 2 +-
 output/ulogd_output_JSON.c            | 2 +-
 output/ulogd_output_LOGEMU.c          | 2 +-
 output/ulogd_output_NACCT.c           | 2 +-
 output/ulogd_output_OPRINT.c          | 2 +-
 output/ulogd_output_SYSLOG.c          | 7 ++++---
 output/ulogd_output_XML.c             | 2 +-
 util/db.c                             | 2 +-
 18 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/filter/ulogd_filter_MARK.c b/filter/ulogd_filter_MARK.c
index 149725d..d5a8181 100644
--- a/filter/ulogd_filter_MARK.c
+++ b/filter/ulogd_filter_MARK.c
@@ -94,8 +94,7 @@ static int configure(struct ulogd_pluginstance *upi,
 	ulogd_log(ULOGD_DEBUG, "parsing config file section `%s', "
 		  "plugin `%s'\n", upi->id, upi->plugin->name);

-	config_parse_file(upi->id, upi->config_kset);
-	return 0;
+	return ulogd_parse_configfile(upi->id, upi->config_kset);
 }

 static struct ulogd_plugin mark_pluging = {
diff --git a/input/flow/ulogd_inpflow_NFCT.c b/input/flow/ulogd_inpflow_NFCT.c
index 899b7e3..ec3dff6 100644
--- a/input/flow/ulogd_inpflow_NFCT.c
+++ b/input/flow/ulogd_inpflow_NFCT.c
@@ -1028,7 +1028,7 @@ static int configure_nfct(struct ulogd_pluginstance *upi,
 {
 	int ret;

-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/input/packet/ulogd_inppkt_NFLOG.c b/input/packet/ulogd_inppkt_NFLOG.c
index 4fdeb12..f716136 100644
--- a/input/packet/ulogd_inppkt_NFLOG.c
+++ b/input/packet/ulogd_inppkt_NFLOG.c
@@ -557,8 +557,7 @@ static int configure(struct ulogd_pluginstance *upi,
 	ulogd_log(ULOGD_DEBUG, "parsing config file section `%s', "
 		  "plugin `%s'\n", upi->id, upi->plugin->name);

-	config_parse_file(upi->id, upi->config_kset);
-	return 0;
+	return ulogd_parse_configfile(upi->id, upi->config_kset);
 }

 static int become_system_logging(struct ulogd_pluginstance *upi, uint8_t pf)
diff --git a/input/packet/ulogd_inppkt_ULOG.c b/input/packet/ulogd_inppkt_ULOG.c
index 45ffc8b..44bc71d 100644
--- a/input/packet/ulogd_inppkt_ULOG.c
+++ b/input/packet/ulogd_inppkt_ULOG.c
@@ -269,7 +269,7 @@ static int ulog_read_cb(int fd, unsigned int what, void *param)
 static int configure(struct ulogd_pluginstance *upi,
 		     struct ulogd_pluginstance_stack *stack)
 {
-	return config_parse_file(upi->id, upi->config_kset);
+	return ulogd_parse_configfile(upi->id, upi->config_kset);
 }
 static int init(struct ulogd_pluginstance *upi)
 {
diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index f1d1534..b328500 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -719,8 +719,7 @@ static int configure(struct ulogd_pluginstance *upi,
 	ulogd_log(ULOGD_DEBUG, "parsing config file section `%s', "
 		  "plugin `%s'\n", upi->id, upi->plugin->name);

-	config_parse_file(upi->id, upi->config_kset);
-	return 0;
+	return ulogd_parse_configfile(upi->id, upi->config_kset);
 }

 static int start(struct ulogd_pluginstance *upi)
diff --git a/input/sum/ulogd_inpflow_NFACCT.c b/input/sum/ulogd_inpflow_NFACCT.c
index b022e63..bd45df4 100644
--- a/input/sum/ulogd_inpflow_NFACCT.c
+++ b/input/sum/ulogd_inpflow_NFACCT.c
@@ -221,7 +221,7 @@ static int configure_nfacct(struct ulogd_pluginstance *upi,
 {
 	int ret;

-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/output/ipfix/ulogd_output_IPFIX.c b/output/ipfix/ulogd_output_IPFIX.c
index 1c0f730..e96c7f8 100644
--- a/output/ipfix/ulogd_output_IPFIX.c
+++ b/output/ipfix/ulogd_output_IPFIX.c
@@ -268,7 +268,7 @@ static int ipfix_configure(struct ulogd_pluginstance *pi, struct ulogd_pluginsta
 	int oid, port, mtu, ret;
 	char addr[16];

-	ret = config_parse_file(pi->id, pi->config_kset);
+	ret = ulogd_parse_configfile(pi->id, pi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
index 19ce47f..474992e 100644
--- a/output/pcap/ulogd_output_PCAP.c
+++ b/output/pcap/ulogd_output_PCAP.c
@@ -260,7 +260,7 @@ static void signal_pcap(struct ulogd_pluginstance *upi, int signal)
 static int configure_pcap(struct ulogd_pluginstance *upi,
 			  struct ulogd_pluginstance_stack *stack)
 {
-	return config_parse_file(upi->id, upi->config_kset);
+	return ulogd_parse_configfile(upi->id, upi->config_kset);
 }

 static int start_pcap(struct ulogd_pluginstance *upi)
diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 6aeb7a3..51c0fc8 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -363,7 +363,8 @@ sqlite3_configure(struct ulogd_pluginstance *pi,
 {
 	/* struct sqlite_priv *priv = (void *)pi->private; */

-	config_parse_file(pi->id, pi->config_kset);
+	if (ulogd_parse_configfile(pi->id, pi->config_kset) < 0)
+		return -1;

 	if (ulogd_wildcard_inputkeys(pi) < 0)
 		return -1;
diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
index 37829fa..8f881a3 100644
--- a/output/ulogd_output_GPRINT.c
+++ b/output/ulogd_output_GPRINT.c
@@ -230,7 +230,7 @@ static int gprint_configure(struct ulogd_pluginstance *upi,
 	if (ret < 0)
 		return ret;

-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/output/ulogd_output_GRAPHITE.c b/output/ulogd_output_GRAPHITE.c
index 5328f8e..e54b24d 100644
--- a/output/ulogd_output_GRAPHITE.c
+++ b/output/ulogd_output_GRAPHITE.c
@@ -214,7 +214,7 @@ static int configure_graphite(struct ulogd_pluginstance *pi,
 			    struct ulogd_pluginstance_stack *stack)
 {
 	ulogd_log(ULOGD_DEBUG, "parsing config file section %s\n", pi->id);
-	return config_parse_file(pi->id, pi->config_kset);
+	return ulogd_parse_configfile(pi->id, pi->config_kset);
 }

 static struct ulogd_plugin graphite_plugin = {
diff --git a/output/ulogd_output_JSON.c b/output/ulogd_output_JSON.c
index f80d0e2..2e7211a 100644
--- a/output/ulogd_output_JSON.c
+++ b/output/ulogd_output_JSON.c
@@ -494,7 +494,7 @@ static int json_configure(struct ulogd_pluginstance *upi,
 	if (ret < 0)
 		return ret;

-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/output/ulogd_output_LOGEMU.c b/output/ulogd_output_LOGEMU.c
index cfcfab7..f5d1def 100644
--- a/output/ulogd_output_LOGEMU.c
+++ b/output/ulogd_output_LOGEMU.c
@@ -178,7 +178,7 @@ static int configure_logemu(struct ulogd_pluginstance *pi,
 			    struct ulogd_pluginstance_stack *stack)
 {
 	ulogd_log(ULOGD_DEBUG, "parsing config file section %s\n", pi->id);
-	return config_parse_file(pi->id, pi->config_kset);
+	return ulogd_parse_configfile(pi->id, pi->config_kset);
 }

 static struct ulogd_plugin logemu_plugin = {
diff --git a/output/ulogd_output_NACCT.c b/output/ulogd_output_NACCT.c
index d369c7a..080a576 100644
--- a/output/ulogd_output_NACCT.c
+++ b/output/ulogd_output_NACCT.c
@@ -203,7 +203,7 @@ nacct_conf(struct ulogd_pluginstance *pi,
 {
 	int ret;

-	if ((ret = config_parse_file(pi->id, pi->config_kset)) < 0)
+	if ((ret = ulogd_parse_configfile(pi->id, pi->config_kset)) < 0)
 		return ret;

 	return 0;
diff --git a/output/ulogd_output_OPRINT.c b/output/ulogd_output_OPRINT.c
index 13934ff..1137be1 100644
--- a/output/ulogd_output_OPRINT.c
+++ b/output/ulogd_output_OPRINT.c
@@ -161,7 +161,7 @@ static int oprint_configure(struct ulogd_pluginstance *upi,
 	if (ret < 0)
 		return ret;

-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/output/ulogd_output_SYSLOG.c b/output/ulogd_output_SYSLOG.c
index 9777f0f..9ee6a61 100644
--- a/output/ulogd_output_SYSLOG.c
+++ b/output/ulogd_output_SYSLOG.c
@@ -83,12 +83,13 @@ static int _output_syslog(struct ulogd_pluginstance *upi)
 static int syslog_configure(struct ulogd_pluginstance *pi,
 			    struct ulogd_pluginstance_stack *stack)
 {
-	int syslog_facility, syslog_level;
+	int syslog_facility, syslog_level, ret;
 	char *facility, *level;
 	struct syslog_instance *li = (struct syslog_instance *) &pi->private;

-	/* FIXME: error handling */
-	config_parse_file(pi->id, pi->config_kset);
+	ret = ulogd_parse_configfile(pi->id, pi->config_kset);
+	if (ret < 0)
+		return ret;

 	facility = pi->config_kset->ces[0].u.string;
 	level = pi->config_kset->ces[1].u.string;
diff --git a/output/ulogd_output_XML.c b/output/ulogd_output_XML.c
index 44af596..55c7a7c 100644
--- a/output/ulogd_output_XML.c
+++ b/output/ulogd_output_XML.c
@@ -190,7 +190,7 @@ static int xml_configure(struct ulogd_pluginstance *upi,
 {
 	int ret;

-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0)
 		return ret;

diff --git a/util/db.c b/util/db.c
index 749a45f..11c3e6a 100644
--- a/util/db.c
+++ b/util/db.c
@@ -153,7 +153,7 @@ int ulogd_db_configure(struct ulogd_pluginstance *upi,
 	ulogd_log(ULOGD_NOTICE, "(re)configuring\n");

 	/* First: Parse configuration file section for this instance */
-	ret = config_parse_file(upi->id, upi->config_kset);
+	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
 	if (ret < 0) {
 		ulogd_log(ULOGD_ERROR, "error parsing config file\n");
 		return ret;
--
2.48.1

[PATCH ulogd2 4/8] ulogd: raise error on unknown config key

[Corubba Smith]

Until a6fbeb96e889 ("new configuration file syntax (Magnus Boden)")
this was already caught, and the enum member is still present.

Check if the for loop worked throught the whole array without hitting a
matching config option, and return with the unknown key error code.
Because there is no existing config_entry struct with that unknwon key
to use with the established config_errce pointer, allocate a new struct.
This potentially creates a memory leak if that config_entry is never
freed again, but for me that is acceptable is this rare case.

Since the memory allocation for the struct can fail, also reuse the old
out-of-memory error to indicate that.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 src/conffile.c | 11 +++++++++++
 src/ulogd.c    |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/src/conffile.c b/src/conffile.c
index 5b7f834..96eff69 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -230,6 +230,17 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 			break;
 		}
 		pr_debug("parse_file: exiting main loop\n");
+
+		if (i == kset->num_ces) {
+			config_errce = calloc(1, sizeof(struct config_entry));
+			if (config_errce == NULL) {
+				err = -ERROOM;
+				goto cpf_error;
+			}
+			strncpy(&config_errce->key[0], wordbuf, CONFIG_KEY_LEN - 1);
+			err = -ERRUNKN;
+			goto cpf_error;
+		}
 	}


diff --git a/src/ulogd.c b/src/ulogd.c
index 80e1ac0..96f88db 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -285,6 +285,8 @@ int ulogd_parse_configfile(const char *section, struct config_keyset *ce)
 			ulogd_log(ULOGD_ERROR,
 			          "unknown config key \"%s\"\n",
 			          config_errce->key);
+			free(config_errce);
+			config_errce = NULL;
 			break;
 		case -ERRSECTION:
 			ulogd_log(ULOGD_ERROR,
--
2.48.1

[PATCH ulogd2 5/8] ulogd: ignore malformed config directives

[Corubba Smith]

When a config directive is provided with a malformed argument (e.g.
`loglevel="1`), then the call to get_word() returns NULL and `wordbuf`
is left unchanged aka still contains the directive name. Unlike the
previous calls to get_word(), the return value is not checked here, and
processing continues with `args` pointing to the still unchanged
`wordbuf`. So `loglevel="1` is effectively parsed as
`loglevel=loglevel`.

Instead if no valid argument is found, ignore the directive and log a
warning.

Due to the way get_word() is implemented, this unfortunately will report
an empty argument (e.g. `loglevel=`) as malformed as well. Ideally that
should behave the same as `loglevel=""`, but I found no nice way to
achieve that. An empty argument is only useful in rare cases, so
treating it as malformed should be fine for now. That's still way better
than the previous broken "name as value" behaviour.

Fixes: e88384d9d5a1 ("added new generic get_word() function to do better parsing")
Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 src/conffile.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/conffile.c b/src/conffile.c
index 96eff69..7b9fb0f 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -198,6 +198,12 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 			}

 			wordend = get_word(wordend, " =\t\n\r", (char *) &wordbuf);
+			if (wordend == NULL) {
+				ulogd_log(ULOGD_NOTICE,
+					"ignoring malformed config directive \"%s\" on line %d\n",
+					ce->key, linenum);
+				break;
+			}
 			args = (char *)&wordbuf;

 			if (ce->hit && !(ce->options & CONFIG_OPT_MULTI))
--
2.48.1

[PATCH ulogd2 6/8] ulogd: improve integer option parsing

[Corubba Smith]

The `value` union member in `struct config_entry` is declared as `int`
since basically the beginning in e07722e46001 ("config stuff added").
The parsing was switched from the original `atoi()` in 015849995f7f
("Fix hexadecimal parsing in config file") to `strtoul()`.

Switch the function for parsing to the signed `strtol()` variant since
the result will be stored in a signed int, and it makes sense to support
negative numbers. Detect when `strtol()` does not properly consume the
whole argument and return a new format error. Also check the numerical
value to make sure the signed int does not overflow, in which case
a new range error is returned.

Unfortunately there is no `strtoi()` which would do the proper range
check itself, so the intermediate `long` and range-check for `int` is
required. I also considered changing the `value` union member from
`int` to `long`, which would make it possible to use the parsed value
as-is. But since this is part of the api towards plugins (including
third party) such a potentially breaking change felt unwarranted. This
also means that still only 16bit integer values are *guaranteed* to
work, although most platforms use bigger widths for int.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 include/ulogd/conffile.h |  2 ++
 src/conffile.c           | 17 ++++++++++++++++-
 src/ulogd.c              | 10 ++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/include/ulogd/conffile.h b/include/ulogd/conffile.h
index 1f3d563..fb54dea 100644
--- a/include/ulogd/conffile.h
+++ b/include/ulogd/conffile.h
@@ -19,6 +19,8 @@ enum {
 	ERRUNKN,	/* unknown config key */
 	ERRSECTION,	/* section not found */
 	ERRTOOLONG,	/* string too long */
+	ERRINTFORMAT,	/* integer format is invalid */
+	ERRINTRANGE,	/* integer value is out of range */
 };

 /* maximum line length of config file entries */
diff --git a/src/conffile.c b/src/conffile.c
index 7b9fb0f..f412804 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -17,6 +17,7 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */

+#include <limits.h>
 #include <ulogd/ulogd.h>
 #include <ulogd/common.h>
 #include <ulogd/conffile.h>
@@ -227,7 +228,21 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 					}
 					break;
 				case CONFIG_TYPE_INT:
-					ce->u.value = strtoul(args, NULL, 0);
+					errno = 0;
+					char *endptr = NULL;
+					long parsed = strtol(args, &endptr, 0);
+					if (endptr == args || *endptr != '\0') {
+						config_errce = ce;
+						err = -ERRINTFORMAT;
+						goto cpf_error;
+					}
+					if (errno == ERANGE ||
+					    parsed < INT_MIN || parsed > INT_MAX) {
+						config_errce = ce;
+						err = -ERRINTRANGE;
+						goto cpf_error;
+					}
+					ce->u.value = (int)parsed;
 					break;
 				case CONFIG_TYPE_CALLBACK:
 					(ce->u.parser)(args);
diff --git a/src/ulogd.c b/src/ulogd.c
index 96f88db..96cea8a 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -302,6 +302,16 @@ int ulogd_parse_configfile(const char *section, struct config_keyset *ce)
 				ulogd_log(ULOGD_ERROR,
 				          "string value is too long\n");
 			break;
+		case -ERRINTFORMAT:
+			ulogd_log(ULOGD_ERROR,
+			          "integer has invalid format for key \"%s\"\n",
+			          config_errce->key);
+			break;
+		case -ERRINTRANGE:
+			ulogd_log(ULOGD_ERROR,
+			          "integer is out of range for key \"%s\"\n",
+			          config_errce->key);
+			break;
 	}

 	return ULOGD_IRET_ERR;
--
2.48.1

[PATCH ulogd2 7/8] ulogd: default configure implementation

[Corubba Smith]

Provide a default implementation for the configure hook which simply
calls ulogd_parse_configfile(), so simple plugins only need to provide
the config_keyset. This also triggers an "unknown key" error if a
plugin defines no config_keyset (aka it has no options), but the config
file contains directives for it.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 src/conffile.c |  7 ++++++-
 src/ulogd.c    | 21 ++++++++++++++-------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/conffile.c b/src/conffile.c
index f412804..b9027da 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -158,7 +158,12 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 	}

 	if (!found) {
-		err = -ERRSECTION;
+		if (kset->num_ces == 0) {
+			/* If there are no options, then no section isnt an error. */
+			err = 0;
+		} else {
+			err = -ERRSECTION;
+		}
 		goto cpf_error;
 	}

diff --git a/src/ulogd.c b/src/ulogd.c
index 96cea8a..d72ede5 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -869,15 +869,22 @@ create_stack_resolve_keys(struct ulogd_pluginstance_stack *stack)
 			  pi_cur->plugin->name);
 		/* call plugin to tell us which keys it requires in
 		 * given configuration */
+		int ret;
 		if (pi_cur->plugin->configure) {
-			int ret = pi_cur->plugin->configure(pi_cur,
-							    stack);
-			if (ret < 0) {
-				ulogd_log(ULOGD_ERROR, "error during "
-					  "configure of plugin %s\n",
-					  pi_cur->plugin->name);
-				return ret;
+			ret = pi_cur->plugin->configure(pi_cur, stack);
+		} else {
+			struct config_keyset empty_kset = {.num_ces=0};
+			struct config_keyset *kset = &empty_kset;
+			if (pi_cur->config_kset) {
+				kset = pi_cur->config_kset;
 			}
+			ret = ulogd_parse_configfile(pi_cur->id, kset);
+		}
+		if (ret < 0) {
+			ulogd_log(ULOGD_ERROR, "error during "
+				  "configure of plugin %s\n",
+				  pi_cur->plugin->name);
+			return ret;
 		}
 	}

--
2.48.1

[PATCH ulogd2 8/8] all: remove trivial configure hooks

[Corubba Smith]

These are now covered by the default implementation.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 filter/ulogd_filter_MARK.c           | 10 ----------
 input/flow/ulogd_inpflow_NFCT.c      | 13 -------------
 input/packet/ulogd_inppkt_NFLOG.c    | 10 ----------
 input/packet/ulogd_inppkt_ULOG.c     |  6 ------
 input/packet/ulogd_inppkt_UNIXSOCK.c | 10 ----------
 output/pcap/ulogd_output_PCAP.c      |  7 -------
 output/ulogd_output_GRAPHITE.c       |  8 --------
 output/ulogd_output_LOGEMU.c         |  8 --------
 output/ulogd_output_NACCT.c          | 13 -------------
 output/ulogd_output_XML.c            | 13 -------------
 10 files changed, 98 deletions(-)

diff --git a/filter/ulogd_filter_MARK.c b/filter/ulogd_filter_MARK.c
index d5a8181..b977780 100644
--- a/filter/ulogd_filter_MARK.c
+++ b/filter/ulogd_filter_MARK.c
@@ -88,15 +88,6 @@ static int interp_mark(struct ulogd_pluginstance *pi)
 	return ULOGD_IRET_OK;
 }

-static int configure(struct ulogd_pluginstance *upi,
-		     struct ulogd_pluginstance_stack *stack)
-{
-	ulogd_log(ULOGD_DEBUG, "parsing config file section `%s', "
-		  "plugin `%s'\n", upi->id, upi->plugin->name);
-
-	return ulogd_parse_configfile(upi->id, upi->config_kset);
-}
-
 static struct ulogd_plugin mark_pluging = {
 	.name = "MARK",
 	.input = {
@@ -109,7 +100,6 @@ static struct ulogd_plugin mark_pluging = {
 		},
 	.interp = &interp_mark,
 	.config_kset = &libulog_kset,
-	.configure = &configure,
 	.version = VERSION,
 };

diff --git a/input/flow/ulogd_inpflow_NFCT.c b/input/flow/ulogd_inpflow_NFCT.c
index ec3dff6..31457de 100644
--- a/input/flow/ulogd_inpflow_NFCT.c
+++ b/input/flow/ulogd_inpflow_NFCT.c
@@ -1023,18 +1023,6 @@ static void polling_timer_cb(struct ulogd_timer *t, void *data)
 	ulogd_add_timer(&cpi->timer, pollint_ce(upi->config_kset).u.value);
 }

-static int configure_nfct(struct ulogd_pluginstance *upi,
-			  struct ulogd_pluginstance_stack *stack)
-{
-	int ret;
-
-	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
 static void overrun_timeout(struct ulogd_timer *a, void *data)
 {
 	int family = AF_UNSPEC;
@@ -1551,7 +1539,6 @@ static struct ulogd_plugin nfct_plugin = {
 	},
 	.config_kset 	= &nfct_kset,
 	.interp 	= NULL,
-	.configure	= &configure_nfct,
 	.start		= &constructor_nfct,
 	.stop		= &destructor_nfct,
 	.signal		= &signal_nfct,
diff --git a/input/packet/ulogd_inppkt_NFLOG.c b/input/packet/ulogd_inppkt_NFLOG.c
index f716136..62b3963 100644
--- a/input/packet/ulogd_inppkt_NFLOG.c
+++ b/input/packet/ulogd_inppkt_NFLOG.c
@@ -551,15 +551,6 @@ release_ct:
 	return ret;
 }

-static int configure(struct ulogd_pluginstance *upi,
-		     struct ulogd_pluginstance_stack *stack)
-{
-	ulogd_log(ULOGD_DEBUG, "parsing config file section `%s', "
-		  "plugin `%s'\n", upi->id, upi->plugin->name);
-
-	return ulogd_parse_configfile(upi->id, upi->config_kset);
-}
-
 static int become_system_logging(struct ulogd_pluginstance *upi, uint8_t pf)
 {
 	struct nflog_input *ui = (struct nflog_input *) upi->private;
@@ -723,7 +714,6 @@ struct ulogd_plugin libulog_plugin = {
 		.num_keys = ARRAY_SIZE(output_keys),
 	},
 	.priv_size 	= sizeof(struct nflog_input),
-	.configure 	= &configure,
 	.start 		= &start,
 	.stop 		= &stop,
 	.config_kset 	= &libulog_kset,
diff --git a/input/packet/ulogd_inppkt_ULOG.c b/input/packet/ulogd_inppkt_ULOG.c
index 44bc71d..2eb994c 100644
--- a/input/packet/ulogd_inppkt_ULOG.c
+++ b/input/packet/ulogd_inppkt_ULOG.c
@@ -266,11 +266,6 @@ static int ulog_read_cb(int fd, unsigned int what, void *param)
 	return 0;
 }

-static int configure(struct ulogd_pluginstance *upi,
-		     struct ulogd_pluginstance_stack *stack)
-{
-	return ulogd_parse_configfile(upi->id, upi->config_kset);
-}
 static int init(struct ulogd_pluginstance *upi)
 {
 	struct ulog_input *ui = (struct ulog_input *) &upi->private;
@@ -325,7 +320,6 @@ struct ulogd_plugin libulog_plugin = {
 		.keys = output_keys,
 		.num_keys = ARRAY_SIZE(output_keys),
 	},
-	.configure = &configure,
 	.start = &init,
 	.stop = &fini,
 	.config_kset = &libulog_kset,
diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index b328500..0d9ba60 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -713,15 +713,6 @@ static int unixsock_server_read_cb(int fd, unsigned int what, void *param)
 	return 0;
 }

-static int configure(struct ulogd_pluginstance *upi,
-		     struct ulogd_pluginstance_stack *stack)
-{
-	ulogd_log(ULOGD_DEBUG, "parsing config file section `%s', "
-		  "plugin `%s'\n", upi->id, upi->plugin->name);
-
-	return ulogd_parse_configfile(upi->id, upi->config_kset);
-}
-
 static int start(struct ulogd_pluginstance *upi)
 {
 	struct unixsock_input *ui = (struct unixsock_input *) upi->private;
@@ -809,7 +800,6 @@ struct ulogd_plugin libunixsock_plugin = {
 		.num_keys = ARRAY_SIZE(output_keys),
 	},
 	.priv_size 	= sizeof(struct unixsock_input),
-	.configure 	= &configure,
 	.start 		= &start,
 	.stop 		= &stop,
 	.config_kset 	= &libunixsock_kset,
diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c
index 474992e..ec29a9e 100644
--- a/output/pcap/ulogd_output_PCAP.c
+++ b/output/pcap/ulogd_output_PCAP.c
@@ -257,12 +257,6 @@ static void signal_pcap(struct ulogd_pluginstance *upi, int signal)
 	}
 }

-static int configure_pcap(struct ulogd_pluginstance *upi,
-			  struct ulogd_pluginstance_stack *stack)
-{
-	return ulogd_parse_configfile(upi->id, upi->config_kset);
-}
-
 static int start_pcap(struct ulogd_pluginstance *upi)
 {
 	return append_create_outfile(upi);
@@ -291,7 +285,6 @@ static struct ulogd_plugin pcap_plugin = {
 	.config_kset	= &pcap_kset,
 	.priv_size	= sizeof(struct pcap_instance),

-	.configure	= &configure_pcap,
 	.start		= &start_pcap,
 	.stop		= &stop_pcap,
 	.signal		= &signal_pcap,
diff --git a/output/ulogd_output_GRAPHITE.c b/output/ulogd_output_GRAPHITE.c
index e54b24d..6942123 100644
--- a/output/ulogd_output_GRAPHITE.c
+++ b/output/ulogd_output_GRAPHITE.c
@@ -210,13 +210,6 @@ static int fini_graphite(struct ulogd_pluginstance *pi) {
 	return 0;
 }

-static int configure_graphite(struct ulogd_pluginstance *pi,
-			    struct ulogd_pluginstance_stack *stack)
-{
-	ulogd_log(ULOGD_DEBUG, "parsing config file section %s\n", pi->id);
-	return ulogd_parse_configfile(pi->id, pi->config_kset);
-}
-
 static struct ulogd_plugin graphite_plugin = {
 	.name = "GRAPHITE",
 	.input = {
@@ -230,7 +223,6 @@ static struct ulogd_plugin graphite_plugin = {
 	.config_kset 	= &graphite_kset,
 	.priv_size 	= sizeof(struct graphite_instance),

-	.configure	= &configure_graphite,
 	.start	 	= &start_graphite,
 	.stop	 	= &fini_graphite,

diff --git a/output/ulogd_output_LOGEMU.c b/output/ulogd_output_LOGEMU.c
index f5d1def..372cac3 100644
--- a/output/ulogd_output_LOGEMU.c
+++ b/output/ulogd_output_LOGEMU.c
@@ -174,13 +174,6 @@ static int fini_logemu(struct ulogd_pluginstance *pi) {
 	return 0;
 }

-static int configure_logemu(struct ulogd_pluginstance *pi,
-			    struct ulogd_pluginstance_stack *stack)
-{
-	ulogd_log(ULOGD_DEBUG, "parsing config file section %s\n", pi->id);
-	return ulogd_parse_configfile(pi->id, pi->config_kset);
-}
-
 static struct ulogd_plugin logemu_plugin = {
 	.name = "LOGEMU",
 	.input = {
@@ -194,7 +187,6 @@ static struct ulogd_plugin logemu_plugin = {
 	.config_kset 	= &logemu_kset,
 	.priv_size 	= sizeof(struct logemu_instance),

-	.configure	= &configure_logemu,
 	.start	 	= &start_logemu,
 	.stop	 	= &fini_logemu,

diff --git a/output/ulogd_output_NACCT.c b/output/ulogd_output_NACCT.c
index 080a576..fa7c501 100644
--- a/output/ulogd_output_NACCT.c
+++ b/output/ulogd_output_NACCT.c
@@ -197,18 +197,6 @@ sighup_handler_print(struct ulogd_pluginstance *pi, int signal)
 	}
 }

-static int
-nacct_conf(struct ulogd_pluginstance *pi,
-		   struct ulogd_pluginstance_stack *stack)
-{
-	int ret;
-
-	if ((ret = ulogd_parse_configfile(pi->id, pi->config_kset)) < 0)
-		return ret;
-
-	return 0;
-}
-
 static int
 nacct_init(struct ulogd_pluginstance *pi)
 {
@@ -243,7 +231,6 @@ static struct ulogd_plugin nacct_plugin = {
 	.output = {
 		.type = ULOGD_DTYPE_SINK,
 	},
-	.configure = &nacct_conf,
 	.interp	= &nacct_interp,
 	.start 	= &nacct_init,
 	.stop	= &nacct_fini,
diff --git a/output/ulogd_output_XML.c b/output/ulogd_output_XML.c
index 55c7a7c..b657436 100644
--- a/output/ulogd_output_XML.c
+++ b/output/ulogd_output_XML.c
@@ -185,18 +185,6 @@ static int xml_output(struct ulogd_pluginstance *upi)
 	return ULOGD_IRET_OK;
 }

-static int xml_configure(struct ulogd_pluginstance *upi,
-			 struct ulogd_pluginstance_stack *stack)
-{
-	int ret;
-
-	ret = ulogd_parse_configfile(upi->id, upi->config_kset);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
 static int xml_fini(struct ulogd_pluginstance *pi)
 {
 	struct xml_priv *op = (struct xml_priv *) &pi->private;
@@ -333,7 +321,6 @@ static struct ulogd_plugin xml_plugin = {
 	.config_kset	= &xml_kset,
 	.priv_size	= sizeof(struct xml_priv),

-	.configure	= &xml_configure,
 	.start		= &xml_start,
 	.stop		= &xml_fini,
 	.interp		= &xml_output,
--
2.48.1

Re: [PATCH ulogd2 2/8] ulogd: add ulogd_parse_configfile public function

[Florian Westphal]

Corubba Smith <corubba@gmx.de> wrote:
> Provide a new function `ulogd_parse_configfile()` in the public
> interface, which wraps `parse_config_file()` to parse a section of the
> config file and communicates found errors to the user. It can be used
> as a drop-in replacement because arguments and return value are
> compatible.

Most patches in this series no longer apply to ulogd2.git,
can you rebase and resend?

Thanks!

> +int ulogd_parse_configfile(const char *section, struct config_keyset *ce)
> +{
> +	int err;
> +
> +	err = config_parse_file(section, ce);
> +
> +	switch(err) {
> +		case 0:
> +			return ULOGD_IRET_OK;
> +			break;

Up to you if you want to change it or not; you can reduce one
indent level:

	switch(err) {
	case 0:
		return ULOGD_IRET_OK;
...