ctdb-protocol: Fix marshalling for ctdb_var_list

Signed-off-by: Amitay Isaacs <amitay@gmail.com>
Reviewed-by: Martin Schwenke <martin@meltin.net>

5 files changed, 170 insertions, 61 deletions

diff --git a/ctdb/protocol/protocol_control.c b/ctdb/protocol/protocol_control.c
index c8edaf740f0..9122fcbf117 100644
--- a/ctdb/protocol/protocol_control.c
+++ b/ctdb/protocol/protocol_control.c
@@ -1472,7 +1472,7 @@ static void ctdb_reply_control_data_push(struct ctdb_reply_control_data *cd,
 		break;
 
 	case CTDB_CONTROL_LIST_TUNABLES:
-		ctdb_var_list_push(cd->data.tun_var_list, buf);
+		ctdb_var_list_push(cd->data.tun_var_list, buf, &np);
 		break;
 
 	case CTDB_CONTROL_GET_ALL_TUNABLES:
@@ -1644,7 +1644,7 @@ static int ctdb_reply_control_data_pull(uint8_t *buf, size_t buflen,
 
 	case CTDB_CONTROL_LIST_TUNABLES:
 		ret = ctdb_var_list_pull(buf, buflen, mem_ctx,
-					 &cd->data.tun_var_list);
+					 &cd->data.tun_var_list, &np);
 		break;
 
 	case CTDB_CONTROL_GET_ALL_TUNABLES:
diff --git a/ctdb/protocol/protocol_private.h b/ctdb/protocol/protocol_private.h
index 1afe5668bb7..1a01322ac30 100644
--- a/ctdb/protocol/protocol_private.h
+++ b/ctdb/protocol/protocol_private.h
@@ -198,10 +198,10 @@ int ctdb_node_flag_change_pull(uint8_t *buf, size_t buflen, TALLOC_CTX *mem_ctx,
 			       struct ctdb_node_flag_change **out,
 			       size_t *npull);
 
-size_t ctdb_var_list_len(struct ctdb_var_list *var_list);
-void ctdb_var_list_push(struct ctdb_var_list *var_list, uint8_t *buf);
+size_t ctdb_var_list_len(struct ctdb_var_list *in);
+void ctdb_var_list_push(struct ctdb_var_list *in, uint8_t *buf, size_t *npush);
 int ctdb_var_list_pull(uint8_t *buf, size_t buflen, TALLOC_CTX *mem_ctx,
-		       struct ctdb_var_list **out);
+		       struct ctdb_var_list **out, size_t *npull);
 
 size_t ctdb_tunable_list_len(struct ctdb_tunable_list *tun_list);
 void ctdb_tunable_list_push(struct ctdb_tunable_list *tun_list, uint8_t *buf);
diff --git a/ctdb/protocol/protocol_types.c b/ctdb/protocol/protocol_types.c
index 537ea62084c..c33c9495c90 100644
--- a/ctdb/protocol/protocol_types.c
+++ b/ctdb/protocol/protocol_types.c
@@ -2270,95 +2270,110 @@ fail:
 	return ret;
 }
 
-struct ctdb_var_list_wire {
-	uint32_t length;
-	char list_str[1];
-};
-
-size_t ctdb_var_list_len(struct ctdb_var_list *var_list)
+size_t ctdb_var_list_len(struct ctdb_var_list *in)
 {
+	uint32_t u32 = 0;
 	int i;
-	size_t len = sizeof(uint32_t);
 
-	for (i=0; i<var_list->count; i++) {
-		len += strlen(var_list->var[i]) + 1;
+	for (i=0; i<in->count; i++) {
+		u32 += ctdb_string_len(&in->var[i]);
 	}
-	return len;
+
+	return ctdb_uint32_len(&u32) + u32;
 }
 
-void ctdb_var_list_push(struct ctdb_var_list *var_list, uint8_t *buf)
+void ctdb_var_list_push(struct ctdb_var_list *in, uint8_t *buf, size_t *npush)
 {
-	struct ctdb_var_list_wire *wire = (struct ctdb_var_list_wire *)buf;
-	int i, n;
-	size_t offset = 0;
+	size_t offset = 0, np;
+	uint32_t u32;
+	int i;
+	uint8_t sep = ':';
 
-	if (var_list->count > 0) {
-		n = sprintf(wire->list_str, "%s", var_list->var[0]);
-		offset += n;
-	}
-	for (i=1; i<var_list->count; i++) {
-		n = sprintf(&wire->list_str[offset], ":%s", var_list->var[i]);
-		offset += n;
+	/* The length only corresponds to the payload size */
+	u32 = ctdb_var_list_len(in);
+	u32 -= ctdb_uint32_len(&u32);
+
+	ctdb_uint32_push(&u32, buf+offset, &np);
+	offset += np;
+
+	/* The variables are separated by ':' and the complete string is null
+	 * terminated.
+	 */
+	for (i=0; i<in->count; i++) {
+		ctdb_string_push(&in->var[i], buf+offset, &np);
+		offset += np;
+
+		if (i < in->count - 1) {
+			/* Replace '\0' with ':' */
+			ctdb_uint8_push(&sep, buf+offset-1, &np);
+		}
 	}
-	wire->length = offset + 1;
+
+	*npush = offset;
 }
 
 int ctdb_var_list_pull(uint8_t *buf, size_t buflen, TALLOC_CTX *mem_ctx,
-		       struct ctdb_var_list **out)
+		       struct ctdb_var_list **out, size_t *npull)
 {
-	struct ctdb_var_list *var_list = NULL;
-	struct ctdb_var_list_wire *wire = (struct ctdb_var_list_wire *)buf;
-	char *str, *s, *tok, *ptr;
-	const char **list;
+	struct ctdb_var_list *val;
+	const char *str, **list;
+	char *s, *tok, *ptr = NULL;
+	size_t offset = 0, np;
+	uint32_t u32;
+	int ret;
 
-	if (buflen < sizeof(uint32_t)) {
-		return EMSGSIZE;
-	}
-	if (wire->length > buflen) {
-		return EMSGSIZE;
-	}
-	if (sizeof(uint32_t) + wire->length < sizeof(uint32_t)) {
-		return EMSGSIZE;
+	val = talloc_zero(mem_ctx, struct ctdb_var_list);
+	if (val == NULL) {
+		return ENOMEM;
 	}
-	if (buflen < sizeof(uint32_t) + wire->length) {
-		return EMSGSIZE;
+
+	ret = ctdb_uint32_pull(buf+offset, buflen-offset, &u32, &np);
+	if (ret != 0) {
+		goto fail;
 	}
+	offset += np;
 
-	str = talloc_strndup(mem_ctx, (char *)wire->list_str, wire->length);
-	if (str == NULL) {
-		return ENOMEM;
+	if (buflen-offset < u32) {
+		ret = EMSGSIZE;
+		goto fail;
 	}
 
-	var_list = talloc_zero(mem_ctx, struct ctdb_var_list);
-	if (var_list == NULL) {
+	ret = ctdb_string_pull(buf+offset, u32, val, &str, &np);
+	if (ret != 0) {
 		goto fail;
 	}
+	offset += np;
 
-	s = str;
+	s = discard_const(str);
 	while ((tok = strtok_r(s, ":", &ptr)) != NULL) {
-		s = NULL;
-		list = talloc_realloc(var_list, var_list->var, const char *,
-				      var_list->count+1);
+		list = talloc_realloc(val, val->var, const char *,
+				      val->count+1);
 		if (list == NULL) {
+			ret = ENOMEM;
 			goto fail;
 		}
 
-		var_list->var = list;
-		var_list->var[var_list->count] = talloc_strdup(var_list, tok);
-		if (var_list->var[var_list->count] == NULL) {
+		val->var = list;
+
+		s = talloc_strdup(val, tok);
+		if (s == NULL) {
+			ret = ENOMEM;
 			goto fail;
 		}
-		var_list->count++;
+
+		val->var[val->count] = s;
+		val->count += 1;
+		s = NULL;
 	}
 
-	talloc_free(str);
-	*out = var_list;
+	talloc_free(discard_const(str));
+	*out = val;
+	*npull = offset;
 	return 0;
 
 fail:
-	talloc_free(str);
-	talloc_free(var_list);
-	return ENOMEM;
+	talloc_free(val);
+	return ret;
 }
 
 size_t ctdb_tunable_list_len(struct ctdb_tunable_list *tun_list)
diff --git a/ctdb/tests/src/protocol_types_compat_test.c b/ctdb/tests/src/protocol_types_compat_test.c
index e2e6d39fb06..e58c9306709 100644
--- a/ctdb/tests/src/protocol_types_compat_test.c
+++ b/ctdb/tests/src/protocol_types_compat_test.c
@@ -825,6 +825,98 @@ static int ctdb_node_flag_change_pull_old(uint8_t *buf, size_t buflen,
 	return 0;
 }
 
+struct ctdb_var_list_wire {
+	uint32_t length;
+	char list_str[1];
+};
+
+static size_t ctdb_var_list_len_old(struct ctdb_var_list *in)
+{
+	int i;
+	size_t len = sizeof(uint32_t);
+
+	for (i=0; i<in->count; i++) {
+		len += strlen(in->var[i]) + 1;
+	}
+	return len;
+}
+
+static void ctdb_var_list_push_old(struct ctdb_var_list *in, uint8_t *buf)
+{
+	struct ctdb_var_list_wire *wire = (struct ctdb_var_list_wire *)buf;
+	int i, n;
+	size_t offset = 0;
+
+	if (in->count > 0) {
+		n = sprintf(wire->list_str, "%s", in->var[0]);
+		offset += n;
+	}
+	for (i=1; i<in->count; i++) {
+		n = sprintf(&wire->list_str[offset], ":%s", in->var[i]);
+		offset += n;
+	}
+	wire->length = offset + 1;
+}
+
+static int ctdb_var_list_pull_old(uint8_t *buf, size_t buflen,
+				  TALLOC_CTX *mem_ctx,
+				  struct ctdb_var_list **out)
+{
+	struct ctdb_var_list *val = NULL;
+	struct ctdb_var_list_wire *wire = (struct ctdb_var_list_wire *)buf;
+	char *str, *s, *tok, *ptr;
+	const char **list;
+
+	if (buflen < sizeof(uint32_t)) {
+		return EMSGSIZE;
+	}
+	if (wire->length > buflen) {
+		return EMSGSIZE;
+	}
+	if (sizeof(uint32_t) + wire->length < sizeof(uint32_t)) {
+		return EMSGSIZE;
+	}
+	if (buflen < sizeof(uint32_t) + wire->length) {
+		return EMSGSIZE;
+	}
+
+	str = talloc_strndup(mem_ctx, (char *)wire->list_str, wire->length);
+	if (str == NULL) {
+		return ENOMEM;
+	}
+
+	val = talloc_zero(mem_ctx, struct ctdb_var_list);
+	if (val == NULL) {
+		goto fail;
+	}
+
+	s = str;
+	while ((tok = strtok_r(s, ":", &ptr)) != NULL) {
+		s = NULL;
+		list = talloc_realloc(val, val->var, const char *,
+				      val->count+1);
+		if (list == NULL) {
+			goto fail;
+		}
+
+		val->var = list;
+		val->var[val->count] = talloc_strdup(val, tok);
+		if (val->var[val->count] == NULL) {
+			goto fail;
+		}
+		val->count++;
+	}
+
+	talloc_free(str);
+	*out = val;
+	return 0;
+
+fail:
+	talloc_free(str);
+	talloc_free(val);
+	return ENOMEM;
+}
+
 
 COMPAT_TYPE3_TEST(struct ctdb_statistics, ctdb_statistics);
 COMPAT_TYPE3_TEST(struct ctdb_vnn_map, ctdb_vnn_map);
@@ -844,6 +936,7 @@ COMPAT_TYPE3_TEST(ctdb_sock_addr, ctdb_sock_addr);
 COMPAT_TYPE3_TEST(struct ctdb_connection, ctdb_connection);
 COMPAT_TYPE3_TEST(struct ctdb_tunable, ctdb_tunable);
 COMPAT_TYPE3_TEST(struct ctdb_node_flag_change, ctdb_node_flag_change);
+COMPAT_TYPE3_TEST(struct ctdb_var_list, ctdb_var_list);
 
 int main(int argc, char *argv[])
 {
@@ -868,6 +961,7 @@ int main(int argc, char *argv[])
 	COMPAT_TEST_FUNC(ctdb_connection)();
 	COMPAT_TEST_FUNC(ctdb_tunable)();
 	COMPAT_TEST_FUNC(ctdb_node_flag_change)();
+	COMPAT_TEST_FUNC(ctdb_var_list)();
 
 	return 0;
 }
diff --git a/ctdb/tests/src/protocol_types_test.c b/ctdb/tests/src/protocol_types_test.c
index 4e2f7d347e7..6961c570487 100644
--- a/ctdb/tests/src/protocol_types_test.c
+++ b/ctdb/tests/src/protocol_types_test.c
@@ -64,7 +64,7 @@ PROTOCOL_TYPE3_TEST(ctdb_sock_addr, ctdb_sock_addr);
 PROTOCOL_TYPE3_TEST(struct ctdb_connection, ctdb_connection);
 PROTOCOL_TYPE3_TEST(struct ctdb_tunable, ctdb_tunable);
 PROTOCOL_TYPE3_TEST(struct ctdb_node_flag_change, ctdb_node_flag_change);
-DEFINE_TEST(struct ctdb_var_list, ctdb_var_list);
+PROTOCOL_TYPE3_TEST(struct ctdb_var_list, ctdb_var_list);
 DEFINE_TEST(struct ctdb_tunable_list, ctdb_tunable_list);
 DEFINE_TEST(struct ctdb_tickle_list, ctdb_tickle_list);
 DEFINE_TEST(struct ctdb_addr_info, ctdb_addr_info);