Attachment 908110 Details for Bug 1108708 – coroparse: More strict numbers parsing

[patch] coroparse: More strict numbers parsing

tmp.ojZ6m1LMQU (text/plain), 9.22 KB, created by Jan Friesse on 2014-06-12 13:00:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jan Friesse

Created: 2014-06-12 13:00:48 UTC

Size: 9.22 KB

patch

obsolete

>From 4e9716ed30ffe6a5750f5c6c2565815e88413c23 Mon Sep 17 00:00:00 2001
>From: Jan Friesse <jfriesse@redhat.com>
>Date: Tue, 10 Jun 2014 16:50:36 +0200
>Subject: [PATCH] coroparse: More strict numbers parsing
>
>Previous safe_atoi didn't check range of input values so if for example
>user used -1 s token timeout, it was converted to UINT32_MAX without
>letting user know.
>
>Another safe_atoi problem was using strtol. This works pretty well on
>64-bit systems, where long integer is usually 64-bits long, sadly on
>32-bit systems, it is usually 32-bit long. And because strtol returns
>signed integer, it was not possible to enter 32-bit value with highest
>bit set.
>
>Solution is to use strtoll which is guaranteed to be at least 64-bits
>long and check value range.
>
>Also error message now contains also information about expected value
>range.
>
>Signed-off-by: Jan Friesse <jfriesse@redhat.com>
>Reviewed-by: Christine Caulfield <ccaulfie@redhat.com>
>---
> exec/coroparse.c |  117 ++++++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 79 insertions(+), 38 deletions(-)
>
>diff --git a/exec/coroparse.c b/exec/coroparse.c
>index 6b883b5..bf46539 100644
>--- a/exec/coroparse.c
>+++ b/exec/coroparse.c
>@@ -405,14 +405,36 @@ static int parse_section(FILE *fp,
> 	return 0;
> }
> 
>-static int safe_atoi(const char *str, int *res)
>+static int safe_atoq_range(icmap_value_types_t value_type, long long int *min_val, long long int *max_val)
> {
>-	int val;
>+	switch (value_type) {
>+	case ICMAP_VALUETYPE_INT8: *min_val = INT8_MIN; *max_val = INT8_MAX; break;
>+	case ICMAP_VALUETYPE_UINT8: *min_val = 0; *max_val = UINT8_MAX; break;
>+	case ICMAP_VALUETYPE_INT16: *min_val = INT16_MIN; *max_val = INT16_MAX; break;
>+	case ICMAP_VALUETYPE_UINT16: *min_val = 0; *max_val = UINT16_MAX; break;
>+	case ICMAP_VALUETYPE_INT32: *min_val = INT32_MIN; *max_val = INT32_MAX; break;
>+	case ICMAP_VALUETYPE_UINT32: *min_val = 0; *max_val = UINT32_MAX; break;
>+	default:
>+		return (-1);
>+	}
>+
>+	return (0);
>+}
>+
>+/*
>+ * Convert string str to long long int res. Type of result is target_type and currently only
>+ * ICMAP_VALUETYPE_[U]INT[8|16|32] is supported.
>+ * Return 0 on success, -1 on failure.
>+ */
>+static int safe_atoq(const char *str, long long int *res, icmap_value_types_t target_type)
>+{
>+	long long int val;
>+	long long int min_val, max_val;
> 	char *endptr;
> 
> 	errno = 0;
> 
>-	val = strtol(str, &endptr, 10);
>+	val = strtoll(str, &endptr, 10);
> 	if (errno == ERANGE) {
> 		return (-1);
> 	}
>@@ -425,6 +447,14 @@ static int safe_atoi(const char *str, int *res)
> 		return (-1);
> 	}
> 
>+	if (safe_atoq_range(target_type, &min_val, &max_val) != 0) {
>+		return (-1);
>+	}
>+
>+	if (val < min_val || val > max_val) {
>+		return (-1);
>+	}
>+
> 	*res = val;
> 	return (0);
> }
>@@ -461,7 +491,10 @@ static int main_config_parser_cb(const char *path,
> 			icmap_map_t config_map,
> 			void *user_data)
> {
>-	int i;
>+	int ii;
>+	long long int val;
>+	long long int min_val, max_val;
>+	icmap_value_types_t val_type = ICMAP_VALUETYPE_BINARY;
> 	unsigned long long int ull;
> 	int add_as_string;
> 	char key_name[ICMAP_KEYNAME_MAXLEN];
>@@ -487,10 +520,11 @@ static int main_config_parser_cb(const char *path,
> 		case MAIN_CP_CB_DATA_STATE_PLOAD:
> 			if ((strcmp(path, "pload.count") == 0) ||
> 			    (strcmp(path, "pload.size") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT32;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				icmap_set_uint32_r(config_map, path, i);
>+				icmap_set_uint32_r(config_map, path, val);
> 				add_as_string = 0;
> 			}
> 			break;
>@@ -499,10 +533,11 @@ static int main_config_parser_cb(const char *path,
> 			    (strcmp(path, "quorum.votes") == 0) ||
> 			    (strcmp(path, "quorum.last_man_standing_window") == 0) ||
> 			    (strcmp(path, "quorum.leaving_timeout") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT32;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				icmap_set_uint32_r(config_map, path, i);
>+				icmap_set_uint32_r(config_map, path, val);
> 				add_as_string = 0;
> 			}
> 
>@@ -512,27 +547,30 @@ static int main_config_parser_cb(const char *path,
> 			    (strcmp(path, "quorum.wait_for_all") == 0) ||
> 			    (strcmp(path, "quorum.auto_tie_breaker") == 0) ||
> 			    (strcmp(path, "quorum.last_man_standing") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT8;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				icmap_set_uint8_r(config_map, path, i);
>+				icmap_set_uint8_r(config_map, path, val);
> 				add_as_string = 0;
> 			}
> 			break;
> 		case MAIN_CP_CB_DATA_STATE_QDEVICE:
> 			if ((strcmp(path, "quorum.device.timeout") == 0) ||
> 			    (strcmp(path, "quorum.device.votes") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT32;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				icmap_set_uint32_r(config_map, path, i);
>+				icmap_set_uint32_r(config_map, path, val);
> 				add_as_string = 0;
> 			}
> 			if ((strcmp(path, "quorum.device.master_wins") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT8;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				icmap_set_uint8_r(config_map, path, i);
>+				icmap_set_uint8_r(config_map, path, val);
> 				add_as_string = 0;
> 			}
> 			break;
>@@ -563,10 +601,11 @@ static int main_config_parser_cb(const char *path,
> 			    (strcmp(path, "totem.max_messages") == 0) ||
> 			    (strcmp(path, "totem.miss_count_const") == 0) ||
> 			    (strcmp(path, "totem.netmtu") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT32;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				icmap_set_uint32_r(config_map,path, i);
>+				icmap_set_uint32_r(config_map,path, val);
> 				add_as_string = 0;
> 			}
> 			if (strcmp(path, "totem.config_version") == 0) {
>@@ -634,11 +673,12 @@ static int main_config_parser_cb(const char *path,
> 
> 		case MAIN_CP_CB_DATA_STATE_INTERFACE:
> 			if (strcmp(path, "totem.interface.ringnumber") == 0) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT8;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
> 
>-				data->ringnumber = i;
>+				data->ringnumber = val;
> 				add_as_string = 0;
> 			}
> 			if (strcmp(path, "totem.interface.bindnetaddr") == 0) {
>@@ -654,27 +694,19 @@ static int main_config_parser_cb(const char *path,
> 				add_as_string = 0;
> 			}
> 			if (strcmp(path, "totem.interface.mcastport") == 0) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT16;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				data->mcastport = i;
>-				if (data->mcastport < 0 || data->mcastport > 65535) {
>-					*error_string = "Invalid multicast port (should be 0..65535)";
>-
>-					return (0);
>-				};
>+				data->mcastport = val;
> 				add_as_string = 0;
> 			}
> 			if (strcmp(path, "totem.interface.ttl") == 0) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT8;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
>-				data->ttl = i;
>-				if (data->ttl < 0 || data->ttl > 255) {
>-					*error_string = "Invalid TTL (should be 0..255)";
>-
>-					return (0);
>-				};
>+				data->ttl = val;
> 				add_as_string = 0;
> 			}
> 			break;
>@@ -804,11 +836,12 @@ static int main_config_parser_cb(const char *path,
> 			snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "nodelist.node.%u.%s", data->node_number, key);
> 			if ((strcmp(key, "nodeid") == 0) ||
> 			    (strcmp(key, "quorum_votes") == 0)) {
>-				if (safe_atoi(value, &i) != 0) {
>+				val_type = ICMAP_VALUETYPE_UINT32;
>+				if (safe_atoq(value, &val, val_type) != 0) {
> 					goto atoi_error;
> 				}
> 
>-				icmap_set_uint32_r(config_map, key_name, i);
>+				icmap_set_uint32_r(config_map, key_name, val);
> 				add_as_string = 0;
> 			}
> 
>@@ -923,13 +956,13 @@ static int main_config_parser_cb(const char *path,
> 				icmap_set_uint8_r(config_map, key_name, data->ttl);
> 			}
> 
>-			i = 0;
>+			ii = 0;
> 			for (iter = data->member_items_head.next;
> 			     iter != &data->member_items_head; iter = iter_next) {
> 				kv_item = list_entry(iter, struct key_value_list_item, list);
> 
> 				snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "totem.interface.%u.member.%u",
>-						data->ringnumber, i);
>+						data->ringnumber, ii);
> 				icmap_set_string_r(config_map, key_name, kv_item->value);
> 
> 				iter_next = iter->next;
>@@ -937,7 +970,7 @@ static int main_config_parser_cb(const char *path,
> 				free(kv_item->value);
> 				free(kv_item->key);
> 				free(kv_item);
>-				i++;
>+				ii++;
> 			}
> 
> 			data->state = MAIN_CP_CB_DATA_STATE_TOTEM;
>@@ -1079,8 +1112,16 @@ static int main_config_parser_cb(const char *path,
> 	return (1);
> 
> atoi_error:
>+	min_val = max_val = 0;
>+	/*
>+	 * This is really assert, because developer ether doesn't set val_type correctly or
>+	 * we've got here after some nasty memory overwrite
>+	 */
>+	assert(safe_atoq_range(val_type, &min_val, &max_val) == 0);
>+
> 	snprintf(formated_err, sizeof(formated_err),
>-	    "Value of key \"%s\" must be integer, but \"%s\" was given", key, value);
>+	    "Value of key \"%s\" is expected to be integer in range (%lld..%lld), but \"%s\" was given",
>+	    key, min_val, max_val, value);
> 	*error_string = formated_err;
> 
> 	return (0);
>-- 
>1.7.1
>

Actions: View | Diff

Attachments on bug 1108708: 908110