[ulogd patch] small code cleaning

[ulogd patch] small code cleaning

[Eric Leblond]

Hello,

Here's a small patchset that brings some code cleaning and aim
at improving usability of ulogd.

Patchset statistics:
 TODO                              |    7 -------
 include/ulogd/conffile.h          |    7 ++++---
 input/packet/ulogd_inppkt_NFLOG.c |    2 --
 output/ulogd_output_LOGEMU.c      |    5 +----
 src/conffile.c                    |   31 +++++++++++++++++++++++++++++--
 src/ulogd.c                       |   12 ++++++++++--
 6 files changed, 44 insertions(+), 20 deletions(-)

If everybody is happy with this patchset, I will pushed it to
git tree and prepare a release in the upcoming weeks.

BR,
--
Eric Leblond <eric@regit.org>

[PATCH 1/7] Update TODO.

[Eric Leblond]

Try to sync TODO with real state of the project.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 TODO |    7 -------
 1 file changed, 7 deletions(-)

diff --git a/TODO b/TODO
index 3a2fe72..d9c0fb3 100644
--- a/TODO
+++ b/TODO
@@ -1,11 +1,4 @@
-- autoconf/automake detection of libnetfilter_{log,conntrack}, mysql, pgsql, sqlite3, pcap
-- conditional compilation of NFLOG, CTNL, PGSQL, MYSQL, SQLITE3, PCAP plugins
 - add support for capabilities to run as non-root
-- IPv6 support (core and extensions)
 - support for static linking
 - issues with ulogd_BASE and partially copied packets (--ulog-cprange)
 - problem wrt. ulogd_BASE and fragments
-- cleanup keys after we've propagatet through the whole stack (RETF_VALID, FREE)
-- port SQLITE3 plugin
-- convert db layer and pgsql + mysql plugin to a 'parameter bind' scheme for efficiency
-- autoconf detection of SCTP / DCCP support
-- 
1.7.10.4

[PATCH 2/7] Suppress dead FIXME.

[Eric Leblond]

Signed-off-by: Eric Leblond <eric@regit.org>
---
 input/packet/ulogd_inppkt_NFLOG.c |    2 --
 src/ulogd.c                       |    1 -
 2 files changed, 3 deletions(-)

diff --git a/input/packet/ulogd_inppkt_NFLOG.c b/input/packet/ulogd_inppkt_NFLOG.c
index 7870c31..cc0c2d4 100644
--- a/input/packet/ulogd_inppkt_NFLOG.c
+++ b/input/packet/ulogd_inppkt_NFLOG.c
@@ -339,7 +339,6 @@ interp_packet(struct ulogd_pluginstance *upi, u_int8_t pf_family,
 		    label_ce(upi->config_kset).u.value);
 
 	if (ph) {
-		/* FIXME */
 		okey_set_u8(&ret[NFLOG_KEY_OOB_HOOK], ph->hook);
 		okey_set_u16(&ret[NFLOG_KEY_OOB_PROTOCOL],
 			     ntohs(ph->hw_protocol));
@@ -376,7 +375,6 @@ interp_packet(struct ulogd_pluginstance *upi, u_int8_t pf_family,
 	if (! (nflog_get_timestamp(ldata, &ts) == 0 && ts.tv_sec))
 		gettimeofday(&ts, NULL);
 
-	/* FIXME: convert endianness */
 	okey_set_u32(&ret[NFLOG_KEY_OOB_TIME_SEC], ts.tv_sec & 0xffffffff);
 	okey_set_u32(&ret[NFLOG_KEY_OOB_TIME_USEC], ts.tv_usec & 0xffffffff);
 
diff --git a/src/ulogd.c b/src/ulogd.c
index d0fdcc5..c7617d9 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1102,7 +1102,6 @@ static void signal_handler(int signal)
 
 static void print_usage(void)
 {
-	/* FIXME */
 	printf("ulogd Version %s\n", VERSION);
 	printf(COPYRIGHT);
 	printf("This is free software with ABSOLUTELY NO WARRANTY.\n\n");
-- 
1.7.10.4

[PATCH 3/7] Use access to ensure readability of config gile

[Eric Leblond]

This patch adds a call to access to check the readability of the
configuration file.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/conffile.c |   11 ++++++++++-
 src/ulogd.c    |    2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/conffile.c b/src/conffile.c
index dd0ed8f..616d7a9 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -16,9 +16,11 @@
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
+
 #include <ulogd/ulogd.h>
 #include <ulogd/common.h>
 #include <ulogd/conffile.h>
+#include <unistd.h>
 
 
 /* points to config entry with error */
@@ -89,10 +91,17 @@ static char *get_word(char *line, char *not, char *buf)
 /* register config file with us */
 int config_register_file(const char *file)
 {
-	/* FIXME: stat of file */
 	if (fname)
 		return 1;
 
+	if (access(file, R_OK) != 0) {
+		ulogd_log(ULOGD_ERROR,
+			 "unable to read configfile \"%s\": %s\n",
+			 file,
+			 strerror(errno));
+		return 1;
+	}
+
 	pr_debug("%s: registered config file '%s'\n", __func__, file);
 
 	fname = (char *) malloc(strlen(file)+1);
diff --git a/src/ulogd.c b/src/ulogd.c
index c7617d9..f8c8ed0 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1216,7 +1216,7 @@ int main(int argc, char* argv[])
 
 	/* parse config file */
 	if (parse_conffile("global", &ulogd_kset)) {
-		ulogd_log(ULOGD_FATAL, "parse_conffile\n");
+		ulogd_log(ULOGD_FATAL, "unable to parse config file\n");
 		warn_and_exit(daemonize);
 	}
 
-- 
1.7.10.4

[PATCH 4/7] Fix typo in comments.

[Eric Leblond]

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/ulogd/conffile.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/ulogd/conffile.h b/include/ulogd/conffile.h
index 09eb8cf..0b8ac0e 100644
--- a/include/ulogd/conffile.h
+++ b/include/ulogd/conffile.h
@@ -20,13 +20,13 @@ enum {
 	ERRSECTION,	/* section not found */
 };
 
-/* maximum line lenght of config file entries */
+/* maximum line length of config file entries */
 #define LINE_LEN 		255
 
-/* maximum lenght of config key name */
+/* maximum length of config key name */
 #define CONFIG_KEY_LEN		30
 
-/* maximum lenght of string config value */
+/* maximum length of string config value */
 #define CONFIG_VAL_STRING_LEN	225
 
 /* valid config types */
-- 
1.7.10.4

[PATCH 5/7] Add handling of too long arguments.

[Eric Leblond]

When an argument is too long, it can not be store into ulogd
configuration and this must results in a error.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/ulogd/conffile.h |    1 +
 src/conffile.c           |    5 ++++-
 src/ulogd.c              |    5 +++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/ulogd/conffile.h b/include/ulogd/conffile.h
index 0b8ac0e..69a6f70 100644
--- a/include/ulogd/conffile.h
+++ b/include/ulogd/conffile.h
@@ -18,6 +18,7 @@ enum {
 	ERRMAND,	/* mandatory option not found */
 	ERRUNKN,	/* unknown config key */
 	ERRSECTION,	/* section not found */
+	ERRTOOLONG,	/* string too long */
 };
 
 /* maximum line length of config file entries */
diff --git a/src/conffile.c b/src/conffile.c
index 616d7a9..9a73406 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -197,7 +197,10 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 					if (strlen(args) < 
 					    CONFIG_VAL_STRING_LEN ) {
 						strcpy(ce->u.string, args);
-						/* FIXME: what if not ? */
+					} else {
+						config_errce = ce;
+						err = -ERRTOOLONG;
+						goto cpf_error;
 					}
 					break;
 				case CONFIG_TYPE_INT:
diff --git a/src/ulogd.c b/src/ulogd.c
index f8c8ed0..6c0df8a 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -989,6 +989,11 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
 			ulogd_log(ULOGD_ERROR,
 				"section \"%s\" not found\n", section);
 			break;
+		case -ERRTOOLONG:
+			ulogd_log(ULOGD_ERROR,
+				"too long string value for key \"%s\"\n",
+				config_errce->key);
+			break;
 	}
 	return 1;
 }
-- 
1.7.10.4

[PATCH 6/7] logemu: return error if configuration is invalid.

[Eric Leblond]

Signed-off-by: Eric Leblond <eric@regit.org>
---
 output/ulogd_output_LOGEMU.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/output/ulogd_output_LOGEMU.c b/output/ulogd_output_LOGEMU.c
index 26f139b..bb27209 100644
--- a/output/ulogd_output_LOGEMU.c
+++ b/output/ulogd_output_LOGEMU.c
@@ -177,11 +177,8 @@ static int fini_logemu(struct ulogd_pluginstance *pi) {
 static int configure_logemu(struct ulogd_pluginstance *pi,
 			    struct ulogd_pluginstance_stack *stack)
 {
-	/* FIXME: error handling */
 	ulogd_log(ULOGD_DEBUG, "parsing config file section %s\n", pi->id);
-	config_parse_file(pi->id, pi->config_kset);
-
-	return 0;
+	return config_parse_file(pi->id, pi->config_kset);
 }
 
 static struct ulogd_plugin logemu_plugin = { 
-- 
1.7.10.4

[PATCH 7/7] conf: error handling for too long line

[Eric Leblond]

Line length in configuration file is limited to 255 due to the use
of a static buffer for line reading. Accepting too long line
without warning as it is currently done could result in some
unexplainable failure.

This patch adds error handling and reject configuration file if a
non comment line is longer than the maximum value.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/conffile.c |   15 +++++++++++++++
 src/ulogd.c    |   10 +++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/conffile.c b/src/conffile.c
index 9a73406..b8e82a8 100644
--- a/src/conffile.c
+++ b/src/conffile.c
@@ -123,6 +123,7 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 	unsigned int i;
 	char linebuf[LINE_LEN+1];
 	char *line = linebuf;
+	int linenum = 0;
 
 	pr_debug("%s: section='%s' file='%s'\n", __func__, section, fname);
 
@@ -135,9 +136,16 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 		char wordbuf[LINE_LEN];
 		char *wordend;
 
+		linenum++;
 		if (*line == '#')
 			continue;
 
+		/* 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;
+		}
+
 		if (!(wordend = get_word(line, " \t\n[]", (char *) wordbuf)))
 			continue;
 		pr_debug("word: \"%s\"\n", wordbuf);
@@ -159,10 +167,17 @@ int config_parse_file(const char *section, struct config_keyset *kset)
 		char wordbuf[LINE_LEN];
 		char *wordend;
 		
+		linenum++;
 		pr_debug("line read: %s\n", line);
 		if (*line == '#')
 			continue;
 
+		/* 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;
+		}
+
 		if (!(wordend = get_word(line, " =\t\n", (char *) &wordbuf)))
 			continue;
 
diff --git a/src/ulogd.c b/src/ulogd.c
index 6c0df8a..4af4e9a 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -990,9 +990,13 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
 				"section \"%s\" not found\n", section);
 			break;
 		case -ERRTOOLONG:
-			ulogd_log(ULOGD_ERROR,
-				"too long string value for key \"%s\"\n",
-				config_errce->key);
+			if (config_errce->key)
+				ulogd_log(ULOGD_ERROR,
+					  "too long string value for key \"%s\"\n",
+					  config_errce->key);
+			else
+				ulogd_log(ULOGD_ERROR,
+					  "too long string value\n");
 			break;
 	}
 	return 1;
-- 
1.7.10.4

Re: [PATCH 5/7] Add handling of too long arguments.

[Mr Dash Four]

> +				"too long string value for key \"%s\"\n",
>   
"string value too long for key"

Re: [PATCH 7/7] conf: error handling for too long line

[Mr Dash Four]

> +					  "too long string value for key \"%s\"\n",
>   
"string value too long for key"

Re: [PATCH 5/7] Add handling of too long arguments.

[Eric Leblond]

Hi,

On Mon, 2013-02-18 at 09:41 +0000, Mr Dash Four wrote:
> > +				"too long string value for key \"%s\"\n",
> >   
> "string value too long for key"

Thanks a lot. Fixing this and the other one!

BR,
-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/

Re: [ulogd patch] small code cleaning

[Eric Leblond]

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

Hello,

Le lundi 18 février 2013 à 09:37 +0100, Eric Leblond a écrit :
> Hello,
> 
> Here's a small patchset that brings some code cleaning and aim
> at improving usability of ulogd.

Patchset has been pushed to master with minor modifications.

BR,
--
Eric Leblond <eric@regit.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]