Add module options checker

Improve time limits handling
Fix smtp proxy options parsing

9 files changed, 228 insertions, 39 deletions

diff --git a/src/cfg_file.h b/src/cfg_file.h
index 9045d3a7a..2595cc779 100644
--- a/src/cfg_file.h
+++ b/src/cfg_file.h
@@ -43,6 +43,15 @@ struct classifier;
 enum { VAL_UNDEF=0, VAL_TRUE, VAL_FALSE };
 
 /**
+ * Type of time configuration parameter
+ */
+enum time_type {
+	TIME_SECONDS = 0,
+	TIME_MILLISECONDS,
+	TIME_MINUTES,
+	TIME_HOURS
+};
+/**
  * Types of rspamd bind lines
  */
 enum rspamd_cred_type {
@@ -357,11 +366,12 @@ gchar* get_module_opt (struct config_file *cfg, gchar *module_name, gchar *opt_n
 gsize parse_limit (const gchar *limit);
 
 /**
- * Parse seconds
+ * Parse time
  * @param t string representation of seconds (eg. 1D)
- * @return numeric value of string
+ * @param default_type dimension of time if no suffix is specified
+ * @return value of time in milliseconds
  */
-guint parse_seconds (const gchar *t);
+guint parse_time (const gchar *t, enum time_type default_type);
 
 /**
  * Parse flag
diff --git a/src/cfg_utils.c b/src/cfg_utils.c
index 6432b2474..2a0f548b8 100644
--- a/src/cfg_utils.c
+++ b/src/cfg_utils.c
@@ -266,6 +266,7 @@ parse_limit (const gchar *limit)
 	if (!limit || *limit == '\0')
 		return 0;
 
+	errno = 0;
 	result = strtoul (limit, &err_str, 10);
 
 	if (*err_str != '\0') {
@@ -281,45 +282,124 @@ parse_limit (const gchar *limit)
 		else if (*err_str == 'g' || *err_str == 'G') {
 			result *= 1073741824L;
 		}
+		else {
+			msg_warn ("invalid limit value '%s' at position '%s'", limit, err_str);
+			result = 0;
+		}
 	}
 
 	return result;
 }
 
 guint
-parse_seconds (const gchar *t)
+parse_time (const gchar *t, enum time_type default_type)
 {
-	guint                           result = 0;
+	union {
+		guint                       i;
+		double                      d;
+	}                               result;
+	gboolean                        use_double = FALSE;
 	gchar                           *err_str;
 
 	if (!t || *t == '\0')
 		return 0;
 
-	result = strtoul (t, &err_str, 10);
+	errno = 0;
+	result.i = strtoul (t, &err_str, 10);
 
 	if (*err_str != '\0') {
+		if (*err_str == '.') {
+			/* Try to handle decimal point */
+			errno = 0;
+			result.d = strtod (t, &err_str);
+			use_double = TRUE;
+		}
 		/* Seconds */
 		if (*err_str == 's' || *err_str == 'S') {
-			result *= 1000;
+			if (use_double) {
+				result.d *= 1000.;
+			}
+			else {
+				result.i *= 1000;
+			}
 		}
 		/* Minutes */
 		else if (*err_str == 'm' || *err_str == 'M') {
 			/* Handle ms correctly */
-			if (*(err_str + 1) == 's' || *(err_str + 1) == 'S') {
-				result *= 60 * 1000;
+			if (*(err_str + 1) != 's' && *(err_str + 1) != 'S') {
+				if (use_double) {
+					result.d *= 60. * 1000.;
+				}
+				else {
+					result.i *= 60 * 1000;
+				}
 			}
 		}
 		/* Hours */
 		else if (*err_str == 'h' || *err_str == 'H') {
-			result *= 60 * 60 * 1000;
+			if (use_double) {
+				result.d *= 60. * 60. * 1000.;
+			}
+			else {
+				result.i *= 60 * 60 * 1000;
+			}
 		}
 		/* Days */
 		else if (*err_str == 'd' || *err_str == 'D') {
-			result *= 24 * 60 * 60 * 1000;
+			if (use_double) {
+				result.d *= 24. * 60. * 60. * 1000.;
+			}
+			else {
+				result.i *= 24 * 60 * 60 * 1000;
+			}
+		}
+		else {
+			msg_warn ("invalid time value '%s' at position '%s'", t, err_str);
+			if (use_double) {
+				result.d = 0.;
+			}
+			else {
+				result.i = 0;
+			}
 		}
 	}
-
-	return result;
+	else {
+		/* Switch to default time multiplier */
+		switch (default_type) {
+		case TIME_HOURS:
+			if (use_double) {
+				result.d *= 60. * 60. * 1000.;
+			}
+			else {
+				result.i *= 60 * 60 * 1000;
+			}
+			break;
+		case TIME_MINUTES:
+			if (use_double) {
+				result.d *= 60. * 1000.;
+			}
+			else {
+				result.i *= 60 * 1000;
+			}
+			break;
+		case TIME_SECONDS:
+			if (use_double) {
+				result.d *= 1000.;
+			}
+			else {
+				result.i *= 1000;
+			}
+			break;
+		case TIME_MILLISECONDS:
+			break;
+		}
+	}
+	if (use_double) {
+		return rint (result.d);
+	}
+	else {
+		return result.i;
+	}
 }
 
 gchar
diff --git a/src/cfg_xml.c b/src/cfg_xml.c
index ddf995481..5ed8229bb 100644
--- a/src/cfg_xml.c
+++ b/src/cfg_xml.c
@@ -35,6 +35,7 @@
 #include "tokenizers/tokenizers.h"
 #include "lua/lua_common.h"
 #include "view.h"
+#include "map.h"
 #include "expressions.h"
 #include "settings.h"
 
@@ -815,7 +816,8 @@ handle_module_opt (struct config_file *cfg, struct rspamd_xml_userdata *ctx, con
 			is_lua = TRUE;
 		}
 	}
-	/* XXX: in fact we cannot check for lua modules and need to do it in post-config procedure
+	/*
+	 * XXX: in fact we cannot check for lua modules and need to do it in post-config procedure
 	 * so just insert any options provided and try to handle them in further process
 	 */
 
@@ -1151,7 +1153,7 @@ handle_statfile_binlog_rotate (struct config_file *cfg, struct rspamd_xml_userda
 	if (st->binlog == NULL) {
 		st->binlog = memory_pool_alloc0 (cfg->cfg_pool, sizeof (struct statfile_binlog_params));
 	}
-	st->binlog->rotate_time = parse_seconds (data);
+	st->binlog->rotate_time = parse_time (data, TIME_SECONDS);
 	
 	return TRUE;
 }
@@ -1226,7 +1228,7 @@ xml_handle_seconds (struct config_file *cfg, struct rspamd_xml_userdata *ctx, GH
 	guint32                      *dest;
 
 	dest = (guint32 *)G_STRUCT_MEMBER_P (dest_struct, offset);
-	*dest = parse_seconds (data);
+	*dest = parse_time (data, TIME_SECONDS);
 	
 	return TRUE;
 }
@@ -1686,9 +1688,79 @@ rspamd_xml_error (GMarkupParseContext *context, GError *error, gpointer user_dat
 }
 
 /* Register handlers for specific parts of config */
+
+/* Checker for module options */
+static gboolean
+check_module_option (struct config_file *cfg, const gchar *mname, const gchar *optname, const gchar *data)
+{
+	struct xml_config_param          *param;
+	enum module_opt_type              type;
+	GHashTable                       *module;
+	gchar                            *err_str;
+
+	if (module_options == NULL) {
+		msg_warn ("no module options registered while checking option %s for module %s", mname, optname);
+		return FALSE;
+	}
+	if ((module = g_hash_table_lookup (module_options, mname)) == NULL) {
+		msg_warn ("module %s has not registered any options while checking for option %s", mname, optname);
+		return FALSE;
+	}
+
+	if ((param = g_hash_table_lookup (module, optname)) == NULL) {
+		msg_warn ("module %s has not registered option %s", mname, optname);
+		return FALSE;
+	}
+
+	type = param->offset;
+
+	/* Now handle option of each type */
+	switch (type) {
+	case MODULE_OPT_TYPE_STRING:
+	case MODULE_OPT_TYPE_ANY:
+		/* Allways OK */
+		return TRUE;
+	case MODULE_OPT_TYPE_INT:
+		(void)strtol (data, &err_str, 10);
+		if (*err_str != '\0') {
+			msg_warn ("non-numeric data for option: '%s' for module: '%s' at position: '%s'", optname, mname, err_str);
+			return FALSE;
+		}
+		break;
+	case MODULE_OPT_TYPE_UINT:
+		(void)strtoul (data, &err_str, 10);
+		if (*err_str != '\0') {
+			msg_warn ("non-numeric data for option: '%s' for module: '%s' at position: '%s'", optname, mname, err_str);
+			return FALSE;
+		}
+		break;
+	case MODULE_OPT_TYPE_TIME:
+		(void)parse_time (data, TIME_SECONDS);
+		if (errno != 0) {
+			msg_warn ("non-numeric data for option: '%s' for module: '%s': %s", optname, mname, strerror (errno));
+			return FALSE;
+		}
+		break;
+	case MODULE_OPT_TYPE_SIZE:
+		(void)parse_limit (data);
+		if (errno != 0) {
+			msg_warn ("non-numeric data for option: '%s' for module: '%s': %s", optname, mname, strerror (errno));
+			return FALSE;
+		}
+		break;
+	case MODULE_OPT_TYPE_MAP:
+		if (!check_map_proto (data, NULL, NULL)) {
+			return FALSE;
+		}
+		break;
+	}
+
+	return TRUE;
+}
+
 /* Register new module option */
 void
-register_module_opt (const gchar *mname, const gchar *optname, element_handler_func func, gpointer dest_struct, gint offset)
+register_module_opt (const gchar *mname, const gchar *optname, enum module_opt_type type)
 {
 	struct xml_config_param          *param;
 	GHashTable                       *module;
@@ -1703,9 +1775,8 @@ register_module_opt (const gchar *mname, const gchar *optname, element_handler_f
 	if ((param = g_hash_table_lookup (module, optname)) == NULL) {
 		/* Register new param */
 		param = g_malloc (sizeof (struct xml_config_param));
-		param->handler = func;
-		param->user_data = dest_struct;
-		param->offset = offset;
+		param->handler = NULL;
+		param->offset = type;
 		param->name = optname;
 		g_hash_table_insert (module, (char *)optname, param);
 	}
@@ -1714,9 +1785,8 @@ register_module_opt (const gchar *mname, const gchar *optname, element_handler_f
 		msg_warn ("replace old handler for param '%s'", optname);
 		g_free (param);
 		param = g_malloc (sizeof (struct xml_config_param));
-		param->handler = func;
-		param->user_data = dest_struct;
-		param->offset = offset;
+		param->handler = NULL;
+		param->offset = type;
 		param->name = optname;
 		g_hash_table_insert (module, (char *)optname, param);
 	}
diff --git a/src/cfg_xml.h b/src/cfg_xml.h
index dc55d2c86..7c976e78e 100644
--- a/src/cfg_xml.h
+++ b/src/cfg_xml.h
@@ -28,6 +28,16 @@ enum xml_read_state {
 	XML_END
 };
 
+enum module_opt_type {
+	MODULE_OPT_TYPE_STRING = 0,
+	MODULE_OPT_TYPE_INT,
+	MODULE_OPT_TYPE_UINT,
+	MODULE_OPT_TYPE_TIME,
+	MODULE_OPT_TYPE_MAP,
+	MODULE_OPT_TYPE_SIZE,
+	MODULE_OPT_TYPE_ANY
+};
+
 struct rspamd_xml_userdata {
 	enum xml_read_state state;
 	struct config_file *cfg;
@@ -137,7 +147,7 @@ gboolean handle_statfile_binlog_rotate (struct config_file *cfg, struct rspamd_x
 gboolean handle_statfile_binlog_master (struct config_file *cfg, struct rspamd_xml_userdata *ctx, GHashTable *attrs, gchar *data, gpointer user_data, gpointer dest_struct, gint offset);
 
 /* Register new module option */
-void register_module_opt (const gchar *mname, const gchar *optname, element_handler_func func, gpointer dest_struct, gint offset);
+void register_module_opt (const gchar *mname, const gchar *optname, enum module_opt_type type);
 
 /* Register new worker's options */
 void register_worker_opt (gint wtype, const gchar *optname, element_handler_func func, gpointer dest_struct, gint offset);
diff --git a/src/greylist_storage.c b/src/greylist_storage.c
index 2217c4d53..293cc1f7f 100644
--- a/src/greylist_storage.c
+++ b/src/greylist_storage.c
@@ -301,10 +301,10 @@ config_greylist_worker (struct rspamd_worker *worker)
 	ctx->expire_time = DEFAULT_EXPIRE_TIME;
 
 	if ((value = g_hash_table_lookup (worker->cf->params, "greylist_time")) != NULL) {
-		ctx->greylist_time = parse_seconds (value) / 1000;
+		ctx->greylist_time = parse_time (value, TIME_SECONDS) / 1000;
 	}
 	if ((value = g_hash_table_lookup (worker->cf->params, "expire_time")) != NULL) {
-		ctx->expire_time = parse_seconds (value) / 1000;
+		ctx->expire_time = parse_time (value, TIME_SECONDS) / 1000;
 	}
 	worker->ctx = ctx;
 
diff --git a/src/map.c b/src/map.c
index a5ad317e4..298c0c2e5 100644
--- a/src/map.c
+++ b/src/map.c
@@ -431,6 +431,29 @@ read_map_file (struct rspamd_map *map, struct file_map_data *data)
 }
 
 gboolean
+check_map_proto (const gchar *map_line, gint *res, const gchar **pos)
+{
+	if (g_ascii_strncasecmp (map_line, "http://", sizeof ("http://") - 1) == 0) {
+		if (res && pos) {
+			*res = PROTO_HTTP;
+			*pos = map_line + sizeof ("http://") - 1;
+		}
+	}
+	else if (g_ascii_strncasecmp (map_line, "file://", sizeof ("file://") - 1) == 0) {
+		if (res && pos) {
+			*res = PROTO_FILE;
+			*pos = map_line + sizeof ("file://") - 1;
+		}
+	}
+	else {
+		msg_warn ("invalid map fetching protocol: %s", map_line);
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+gboolean
 add_map (const gchar *map_line, map_cb_t read_callback, map_fin_cb_t fin_callback, void **user_data)
 {
 	struct rspamd_map              *new_map;
@@ -443,16 +466,7 @@ add_map (const gchar *map_line, map_cb_t read_callback, map_fin_cb_t fin_callbac
 	struct hostent                 *hent;
 
 	/* First of all detect protocol line */
-	if (strncmp (map_line, "http://", sizeof ("http://") - 1) == 0) {
-		proto = PROTO_HTTP;
-		def = map_line + sizeof ("http://") - 1;
-	}
-	else if (strncmp (map_line, "file://", sizeof ("file://") - 1) == 0) {
-		proto = PROTO_FILE;
-		def = map_line + sizeof ("file://") - 1;
-	}
-	else {
-		msg_warn ("invalid map fetching protocol: %s", map_line);
+	if (!check_map_proto (map_line, (int *)&proto, &def)) {
 		return FALSE;
 	}
 	/* Constant pool */
diff --git a/src/map.h b/src/map.h
index eb427b600..de95c1153 100644
--- a/src/map.h
+++ b/src/map.h
@@ -70,6 +70,10 @@ struct rspamd_map {
 };
 
 /**
+ * Check map protocol
+ */
+gboolean check_map_proto (const gchar *map_line, gint *res, const gchar **pos);
+/**
  * Add map from line
  */
 gboolean add_map (const gchar *map_line, map_cb_t read_callback, map_fin_cb_t fin_callback, void **user_data);
diff --git a/src/plugins/surbl.c b/src/plugins/surbl.c
index bcaa30021..85f457a7e 100644
--- a/src/plugins/surbl.c
+++ b/src/plugins/surbl.c
@@ -224,6 +224,7 @@ surbl_module_init (struct config_file *cfg, struct module_ctx **ctx)
 	*ctx = (struct module_ctx *)surbl_module_ctx;
 
 	register_protocol_command ("urls", urls_command_handler);
+	/* Register module options */
 
 	return 0;
 }
@@ -280,13 +281,13 @@ surbl_module_config (struct config_file *cfg)
 		surbl_module_ctx->url_expire = DEFAULT_SURBL_URL_EXPIRE;
 	}
 	if ((value = get_module_opt (cfg, "surbl", "redirector_connect_timeout")) != NULL) {
-		surbl_module_ctx->connect_timeout = parse_seconds (value);
+		surbl_module_ctx->connect_timeout = parse_time (value, TIME_SECONDS);
 	}
 	else {
 		surbl_module_ctx->connect_timeout = DEFAULT_REDIRECTOR_CONNECT_TIMEOUT;
 	}
 	if ((value = get_module_opt (cfg, "surbl", "redirector_read_timeout")) != NULL) {
-		surbl_module_ctx->read_timeout = parse_seconds (value);
+		surbl_module_ctx->read_timeout = parse_time (value, TIME_SECONDS);
 	}
 	else {
 		surbl_module_ctx->read_timeout = DEFAULT_REDIRECTOR_READ_TIMEOUT;
diff --git a/src/smtp.c b/src/smtp.c
index d1bef023a..793f33e3a 100644
--- a/src/smtp.c
+++ b/src/smtp.c
@@ -958,7 +958,7 @@ config_smtp_worker (struct rspamd_worker *worker)
 		return FALSE;
 	}
 	/* Create smtp banner */
-	if ((ctx->smtp_banner_str) != NULL) {
+	if ((value = ctx->smtp_banner_str) != NULL) {
 		parse_smtp_banner (ctx, value);
 	}