[PATCH v2 01/11] dbus: Validate field type in get_header_field

[PATCH v2 01/11] dbus: Validate field type in get_header_field

[Andrew Zaborowski]

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

It seems there's no place where we are validating the header
fields of incoming messages so let's do it here.
---
 ell/dbus-message.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index ece98a4..25875a9 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -554,7 +554,8 @@ static inline bool message_iter_next_entry(struct l_dbus_message_iter *iter,
 }
 
 static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
-						uint8_t type, va_list args)
+						uint8_t type, char data_type,
+						va_list args)
 {
 	struct l_dbus_message_iter header;
 	struct l_dbus_message_iter array, iter;
@@ -591,6 +592,9 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 		if (field_type != type)
 			continue;
 
+		if (iter.sig_start[iter.sig_pos] != data_type)
+			return false;
+
 		return message_iter_next_entry_valist(&iter, args);
 	}
 
@@ -598,13 +602,14 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 }
 
 static inline bool get_header_field(struct l_dbus_message *message,
-                                                uint8_t type, ...)
+					uint8_t type, char data_type, ...)
 {
 	va_list args;
 	bool result;
 
-	va_start(args, type);
-	result = get_header_field_from_iter_valist(message, type, args);
+	va_start(args, data_type);
+	result = get_header_field_from_iter_valist(message, type, data_type,
+							args);
 	va_end(args);
 
 	return result;
@@ -658,7 +663,7 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
 
 	message->sealed = true;
 
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
+	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&message->signature);
 
 	return message;
@@ -690,7 +695,7 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 
 	message->sealed = true;
 
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
+	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&message->signature);
 
 	return message;
@@ -1095,7 +1100,7 @@ static bool append_arguments(struct l_dbus_message *message,
 	build_header(message, signature);
 	message->sealed = true;
 
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
+	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&message->signature);
 
 	return true;
@@ -1130,7 +1135,7 @@ LIB_EXPORT bool l_dbus_message_get_error(struct l_dbus_message *message,
 		return false;
 
 	if (!message->error_name)
-		get_header_field(message, DBUS_MESSAGE_FIELD_ERROR_NAME,
+		get_header_field(message, DBUS_MESSAGE_FIELD_ERROR_NAME, 's',
 					&message->error_name);
 
 	if (name)
@@ -1213,7 +1218,7 @@ LIB_EXPORT const char *l_dbus_message_get_path(struct l_dbus_message *message)
 		return NULL;
 
 	if (!message->path)
-		get_header_field(message, DBUS_MESSAGE_FIELD_PATH,
+		get_header_field(message, DBUS_MESSAGE_FIELD_PATH, 'o',
 					&message->path);
 
 	return message->path;
@@ -1225,7 +1230,7 @@ LIB_EXPORT const char *l_dbus_message_get_interface(struct l_dbus_message *messa
 		return NULL;
 
 	if (!message->interface)
-		get_header_field(message, DBUS_MESSAGE_FIELD_INTERFACE,
+		get_header_field(message, DBUS_MESSAGE_FIELD_INTERFACE, 's',
 					&message->interface);
 
 	return message->interface;
@@ -1237,7 +1242,7 @@ LIB_EXPORT const char *l_dbus_message_get_member(struct l_dbus_message *message)
 		return NULL;
 
 	if (!message->member)
-		get_header_field(message, DBUS_MESSAGE_FIELD_MEMBER,
+		get_header_field(message, DBUS_MESSAGE_FIELD_MEMBER, 's',
 					&message->member);
 
 	return message->member;
@@ -1249,7 +1254,7 @@ LIB_EXPORT const char *l_dbus_message_get_destination(struct l_dbus_message *mes
 		return NULL;
 
 	if (!message->destination)
-		get_header_field(message, DBUS_MESSAGE_FIELD_DESTINATION,
+		get_header_field(message, DBUS_MESSAGE_FIELD_DESTINATION, 's',
 							&message->destination);
 
 	return message->destination;
@@ -1261,7 +1266,7 @@ LIB_EXPORT const char *l_dbus_message_get_sender(struct l_dbus_message *message)
 		return NULL;
 
 	if (!message->sender)
-		get_header_field(message, DBUS_MESSAGE_FIELD_SENDER,
+		get_header_field(message, DBUS_MESSAGE_FIELD_SENDER, 's',
 					&message->sender);
 
 	return message->sender;
@@ -1282,7 +1287,7 @@ uint32_t _dbus_message_get_reply_serial(struct l_dbus_message *message)
 		return 0;
 
 	if (message->reply_serial == 0)
-		get_header_field(message, DBUS_MESSAGE_FIELD_REPLY_SERIAL,
+		get_header_field(message, DBUS_MESSAGE_FIELD_REPLY_SERIAL, 'u',
 					&message->reply_serial);
 
 	return message->reply_serial;
@@ -1809,7 +1814,7 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_builder_finalize(
 	build_header(builder->message, generated_signature);
 	builder->message->sealed = true;
 
-	get_header_field(builder->message, DBUS_MESSAGE_FIELD_SIGNATURE,
+	get_header_field(builder->message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&builder->message->signature);
 	l_free(generated_signature);
 
-- 
2.5.0

[PATCH v2 02/11] dbus: Check some more return values when parsing messages

[Andrew Zaborowski]

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

Without this we will still segfault on some incoming messages.  Add a
comment to clarify no get_header_field value check.
---
 ell/dbus-kernel.c  |  2 ++
 ell/dbus-message.c | 27 ++++++++++++++++++---------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 2b41177..09721c2 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -561,6 +561,8 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 
 	*out_message = dbus_message_build(header, header_size, body, body_size,
 						NULL, 0);
+	if (!*out_message)
+		return -EBADMSG;
 
 	if (kmsg->src_id != KDBUS_SRC_ID_KERNEL) {
 		sprintf(unique_bus_name, ":1.%llu", kmsg->src_id);
diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 25875a9..70f98b8 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -568,16 +568,21 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 	if (_dbus_message_is_gvariant(message)) {
 		uint32_t header_length;
 
-		_gvariant_iter_init(&header, message, "yyyyuuu", NULL,
-					message->header, message->header_size);
+		if (!_gvariant_iter_init(&header, message, "yyyyuuu", NULL,
+					message->header, message->header_size))
+			return false;
+
 		if (!message_iter_next_entry(&header, &endian, &message_type,
 						&flags, &version, &body_length,
 						&serial, &header_length))
 			return false;
 
-		_gvariant_iter_init(&header, message, "a(yv)", NULL,
-					message->header + 16, header_length);
-		_gvariant_iter_enter_array(&header, &array);
+		if (!_gvariant_iter_init(&header, message, "a(yv)", NULL,
+					message->header + 16, header_length))
+			return false;
+
+		if (!_gvariant_iter_enter_array(&header, &array))
+			return false;
 	} else {
 		_dbus1_iter_init(&header, message, "yyyyuua(yv)", NULL,
 				message->header, message->header_size);
@@ -663,6 +668,7 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
 
 	message->sealed = true;
 
+	/* If the field is absent message->signature will remain NULL */
 	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&message->signature);
 
@@ -695,6 +701,7 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 
 	message->sealed = true;
 
+	/* If the field is absent message->signature will remain NULL */
 	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&message->signature);
 
@@ -1179,10 +1186,12 @@ LIB_EXPORT bool l_dbus_message_get_arguments(struct l_dbus_message *message,
 	if (!signature || strcmp(message->signature, signature))
 		return false;
 
-	if (_dbus_message_is_gvariant(message))
-		_gvariant_iter_init(&iter, message, message->signature, NULL,
-					message->body, message->body_size);
-	else
+	if (_dbus_message_is_gvariant(message)) {
+		if (!_gvariant_iter_init(&iter, message, message->signature,
+						NULL, message->body,
+						message->body_size))
+			return false;
+	} else
 		_dbus1_iter_init(&iter, message, message->signature, NULL,
 				message->body, message->body_size);
 
-- 
2.5.0

[PATCH v2 03/11] dbus: GVariant header field identifiers are 64-bit

[Andrew Zaborowski]

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

For little-endian messages the 8-bit field identifiers were always
right-padded with zeros (or should have been) which worked out to look
same as 64-bit values in encoded messages so things would have
magically worked, but not with big-endian messages.
---
 ell/dbus-message.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 70f98b8..3bc52ea 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -559,14 +559,16 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 {
 	struct l_dbus_message_iter header;
 	struct l_dbus_message_iter array, iter;
-	uint8_t endian, message_type, flags, version, field_type;
+	uint8_t endian, message_type, flags, version;
 	uint32_t body_length, serial;
+	bool found;
 
 	if (!message->sealed)
 		return false;
 
 	if (_dbus_message_is_gvariant(message)) {
 		uint32_t header_length;
+		uint64_t field_type;
 
 		if (!_gvariant_iter_init(&header, message, "yyyyuuu", NULL,
 					message->header, message->header_size))
@@ -577,13 +579,20 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 						&serial, &header_length))
 			return false;
 
-		if (!_gvariant_iter_init(&header, message, "a(yv)", NULL,
+		if (!_gvariant_iter_init(&header, message, "a(tv)", NULL,
 					message->header + 16, header_length))
 			return false;
 
 		if (!_gvariant_iter_enter_array(&header, &array))
 			return false;
+
+		while ((found = message_iter_next_entry(&array,
+							&field_type, &iter)))
+			if (field_type == type)
+				break;
 	} else {
+		uint8_t field_type;
+
 		_dbus1_iter_init(&header, message, "yyyyuua(yv)", NULL,
 				message->header, message->header_size);
 
@@ -591,19 +600,20 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 						&message_type, &flags, &version,
 						&body_length, &serial, &array))
 			return false;
-	}
 
-	while (message_iter_next_entry(&array, &field_type, &iter)) {
-		if (field_type != type)
-			continue;
+		while ((found = message_iter_next_entry(&array,
+							&field_type, &iter)))
+			if (field_type == type)
+				break;
+	}
 
-		if (iter.sig_start[iter.sig_pos] != data_type)
-			return false;
+	if (!found)
+		return false;
 
-		return message_iter_next_entry_valist(&iter, args);
-	}
+	if (iter.sig_start[iter.sig_pos] != data_type)
+		return false;
 
-	return false;
+	return message_iter_next_entry_valist(&iter, args);
 }
 
 static inline bool get_header_field(struct l_dbus_message *message,
@@ -795,8 +805,15 @@ static void add_field(struct dbus_builder *builder,
 			struct builder_driver *driver,
 			uint8_t field, const char *type, const void *value)
 {
-	driver->enter_struct(builder, "yv");
-	driver->append_basic(builder, 'y', &field);
+	if (driver == &gvariant_driver) {
+		uint64_t long_field = field;
+
+		driver->enter_struct(builder, "tv");
+		driver->append_basic(builder, 't', &long_field);
+	} else {
+		driver->enter_struct(builder, "yv");
+		driver->append_basic(builder, 'y', &field);
+	}
 	driver->enter_variant(builder, type);
 	driver->append_basic(builder, type[0], value);
 	driver->leave_variant(builder);
@@ -823,7 +840,8 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 		driver->append_basic(builder, 'u', &field_length);
 	}
 
-	driver->enter_array(builder, "(yv)");
+	driver->enter_array(builder, _dbus_message_is_gvariant(message) ?
+				"(tv)" : "(yv)");
 
 	if (message->path) {
 		add_field(builder, driver, DBUS_MESSAGE_FIELD_PATH,
-- 
2.5.0

[PATCH v2 04/11] dbus: Fix parsing GVariant header/body information

[Andrew Zaborowski]

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

In GVariant messages there's no body size field (it is reserved and must
be sent as 0) or header size information or signature.  We do still add
the signature as a field same as in Dbus-1 because it's not prohibited
but we can't rely on it being there.  The message needs to be parsed as
a single GVariant blob if we want to split it into a header and a body.
---
 ell/dbus-kernel.c  |   3 --
 ell/dbus-message.c | 108 ++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 73 insertions(+), 38 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 09721c2..4a3aed0 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -551,9 +551,6 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 	if (hdr->version != 2)
 		return -EPROTO;
 
-	if (hdr->body_length != body_size)
-		return -EBADMSG;
-
 	r = collect_body_parts(kmsg, header_size, &header,
 						body_size, &body);
 	if (r < 0)
diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 3bc52ea..964b925 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -58,7 +58,8 @@ struct l_dbus_message {
 	int refcount;
 	void *header;
 	size_t header_size;
-	const char *signature;
+	size_t header_end;
+	char *signature;
 	void *body;
 	size_t body_size;
 	char *path;
@@ -74,6 +75,7 @@ struct l_dbus_message {
 	bool sealed : 1;
 	bool kdbus_sender : 1;
 	bool kdbus_destination : 1;
+	bool signature_free : 1;
 };
 
 struct l_dbus_message_builder {
@@ -196,10 +198,12 @@ static struct l_dbus_message *message_new_common(uint8_t type, uint8_t flags,
 
 	/*
 	 * We allocate the header with the initial 12 bytes (up to the field
-	 * length) so that we can store the basic information here
+	 * length) so that we can store the basic information here.  For
+	 * GVariant we need 16 bytes.
 	 */
-	message->header = l_realloc(NULL, 12);
-	message->header_size = 12;
+	message->header_size = version == 1 ? 12 : 16;
+	message->header_end = message->header_size;
+	message->header = l_realloc(NULL, message->header_size);
 
 	hdr = message->header;
 	hdr->endian = DBUS_NATIVE_ENDIAN;
@@ -387,6 +391,9 @@ LIB_EXPORT void l_dbus_message_unref(struct l_dbus_message *message)
 	if (message->kdbus_destination)
 		l_free(message->destination);
 
+	if (message->signature_free)
+		l_free(message->signature);
+
 	l_free(message->header);
 	l_free(message->body);
 	l_free(message);
@@ -567,20 +574,11 @@ static bool get_header_field_from_iter_valist(struct l_dbus_message *message,
 		return false;
 
 	if (_dbus_message_is_gvariant(message)) {
-		uint32_t header_length;
 		uint64_t field_type;
 
-		if (!_gvariant_iter_init(&header, message, "yyyyuuu", NULL,
-					message->header, message->header_size))
-			return false;
-
-		if (!message_iter_next_entry(&header, &endian, &message_type,
-						&flags, &version, &body_length,
-						&serial, &header_length))
-			return false;
-
 		if (!_gvariant_iter_init(&header, message, "a(tv)", NULL,
-					message->header + 16, header_length))
+						message->header + 16,
+						message->header_end - 16))
 			return false;
 
 		if (!_gvariant_iter_enter_array(&header, &array))
@@ -653,6 +651,7 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
 {
 	const struct dbus_header *hdr = data;
 	struct l_dbus_message *message;
+	size_t body_pos;
 
 	if (unlikely(size < DBUS_HEADER_SIZE))
 		return NULL;
@@ -661,28 +660,62 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
 
 	message->refcount = 1;
 
-	message->header_size = align_len(DBUS_HEADER_SIZE +
-						hdr->field_length, 8);
-	message->body_size = hdr->body_length;
+	if (hdr->version == 1) {
+		message->header_size = align_len(DBUS_HEADER_SIZE +
+							hdr->field_length, 8);
+		message->body_size = hdr->body_length;
 
-	if (message->header_size + message->body_size < size) {
-		l_free(message);
-		return NULL;
+		if (message->header_size + message->body_size < size)
+			goto free;
+
+		body_pos = message->header_size;
+	} else {
+		struct l_dbus_message_iter iter, header,
+			variant, structure, body;
+
+		if (!_gvariant_iter_init(&iter, message, "((yyyyuta{tv})v)",
+						NULL, data, size))
+			goto free;
+
+		if (!_gvariant_iter_enter_struct(&iter, &structure))
+			goto free;
+
+		if (!_gvariant_iter_enter_struct(&structure, &header))
+			goto free;
+
+		if (!_gvariant_iter_enter_variant(&structure, &variant))
+			goto free;
+
+		if (!_gvariant_iter_enter_struct(&variant, &body))
+			goto free;
+
+		message->header_size = align_len(header.len - header.pos, 8);
+		message->body_size = body.len - body.pos;
+		message->signature = l_strndup(body.sig_start + body.sig_pos,
+						body.sig_len - body.sig_pos);
+		message->signature_free = true;
+		message->header_end = header.len;
+		body_pos = body.data + body.pos - data;
 	}
 
 	message->header = l_malloc(message->header_size);
 	message->body = l_malloc(message->body_size);
 
 	memcpy(message->header, data, message->header_size);
-	memcpy(message->body, data + message->header_size, message->body_size);
+	memcpy(message->body, data + body_pos, message->body_size);
 
 	message->sealed = true;
 
 	/* If the field is absent message->signature will remain NULL */
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
-						&message->signature);
+	if (hdr->version == 1)
+		get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
+					'g', &message->signature);
 
 	return message;
+
+free:
+	l_free(message);
+	return NULL;
 }
 
 struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
@@ -698,6 +731,13 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 	if (unlikely(!valid_header(hdr)))
 		return NULL;
 
+	/*
+	 * With GVariant we need to know the signature, use
+	 * dbus_message_from_blob instead.
+	 */
+	if (unlikely(hdr->version != 1))
+		return NULL;
+
 	message = l_new(struct l_dbus_message, 1);
 
 	message->refcount = 1;
@@ -827,21 +867,18 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 	char *generated_signature;
 	struct dbus_header *hdr;
 	size_t header_size;
+	bool gvariant;
 
-	if (_dbus_message_is_gvariant(message))
+	gvariant = _dbus_message_is_gvariant(message);
+
+	if (gvariant)
 		driver = &gvariant_driver;
 	else
 		driver = &dbus1_driver;
 
 	builder = driver->new(message->header, message->header_size);
 
-	if (_dbus_message_is_gvariant(message)) {
-		uint32_t field_length = 0;
-		driver->append_basic(builder, 'u', &field_length);
-	}
-
-	driver->enter_array(builder, _dbus_message_is_gvariant(message) ?
-				"(tv)" : "(yv)");
+	driver->enter_array(builder, gvariant ? "(tv)" : "(yv)");
 
 	if (message->path) {
 		add_field(builder, driver, DBUS_MESSAGE_FIELD_PATH,
@@ -906,15 +943,16 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 	hdr = message->header;
 
 	if (_dbus_message_is_gvariant(message))
-		hdr->field_length = header_size - 16;
-
-	hdr->body_length = message->body_size;
+		hdr->body_length = 0;
+	else
+		hdr->body_length = message->body_size;
 
 	/* We must align the end of the header to an 8-byte boundary */
 	message->header_size = align_len(header_size, 8);
 	message->header = l_realloc(message->header, message->header_size);
 	memset(message->header + header_size, 0,
 			message->header_size - header_size);
+	message->header_end = header_size;
 }
 
 struct container {
-- 
2.5.0

[PATCH v2 05/11] dbus: GVariant message cookie and reply cookie are 64-bit

[Andrew Zaborowski]

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

They're 64-bit but the systemd docs discourage using the upper 32 bits,
so we take advantage of that and only support cookie values that fit 32
bits to avoid too big code changes (we use the dbus1 header layout for
the cookie storage so we'd have to move the cookie/serial out of that
header like we do with sender, path, etc.).  This only tries to fix the
parsing and encoding.
---
 ell/dbus-message.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 8 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 964b925..eb47eb3 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -111,13 +111,27 @@ void _dbus_message_set_serial(struct l_dbus_message *msg, uint32_t serial)
 {
 	struct dbus_header *hdr = msg->header;
 
-	hdr->serial = serial;
+	if (_dbus_message_is_gvariant(msg)) {
+		if (_dbus_message_get_endian(msg) ==
+				DBUS_MESSAGE_LITTLE_ENDIAN) {
+			hdr->serial = serial;
+			hdr->field_length = 0;
+		} else {
+			hdr->serial = 0;
+			hdr->field_length = serial;
+		}
+	} else
+		hdr->serial = serial;
 }
 
 uint32_t _dbus_message_get_serial(struct l_dbus_message *msg)
 {
 	struct dbus_header *hdr = msg->header;
 
+	if (_dbus_message_is_gvariant(msg) && _dbus_message_get_endian(msg) ==
+			DBUS_MESSAGE_BIG_ENDIAN)
+		return hdr->field_length;
+
 	return hdr->serial;
 }
 
@@ -294,7 +308,7 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_method_return(
 					DBUS_MESSAGE_FLAG_NO_REPLY_EXPECTED,
 					hdr->version);
 
-	message->reply_serial = hdr->serial;
+	message->reply_serial = _dbus_message_get_serial(method_call);
 
 	sender = l_dbus_message_get_sender(method_call);
 	if (sender)
@@ -321,7 +335,7 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
 					DBUS_MESSAGE_FLAG_NO_REPLY_EXPECTED,
 					hdr->version);
 
-	reply->reply_serial = hdr->serial;
+	reply->reply_serial = _dbus_message_get_serial(method_call);
 
 	sender = l_dbus_message_get_sender(method_call);
 	if (sender)
@@ -641,8 +655,17 @@ static bool valid_header(const struct dbus_header *hdr)
 	if (hdr->version != 1 && hdr->version != 2)
 		return false;
 
-	if (hdr->serial == 0)
-		return false;
+	if (hdr->version == 1) {
+		if (hdr->serial == 0)
+			return false;
+	} else if (hdr->endian == DBUS_MESSAGE_LITTLE_ENDIAN) {
+		/* We don't support 64-bit GVariant cookies */
+		if (hdr->serial == 0 || hdr->field_length != 0)
+			return false;
+	} else
+		/* We don't support 64-bit GVariant cookies */
+		if (hdr->serial != 0 || hdr->field_length == 0)
+			return false;
 
 	return true;
 }
@@ -916,8 +939,18 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 	}
 
 	if (message->reply_serial != 0) {
-		add_field(builder, driver, DBUS_MESSAGE_FIELD_REPLY_SERIAL,
+		if (gvariant) {
+			uint64_t reply_serial = message->reply_serial;
+
+			add_field(builder, driver,
+					DBUS_MESSAGE_FIELD_REPLY_SERIAL,
+					"t", &reply_serial);
+		} else {
+			add_field(builder, driver,
+					DBUS_MESSAGE_FIELD_REPLY_SERIAL,
 					"u", &message->reply_serial);
+		}
+
 		message->reply_serial = 0;
 	}
 
@@ -1351,9 +1384,20 @@ uint32_t _dbus_message_get_reply_serial(struct l_dbus_message *message)
 	if (unlikely(!message))
 		return 0;
 
-	if (message->reply_serial == 0)
-		get_header_field(message, DBUS_MESSAGE_FIELD_REPLY_SERIAL, 'u',
+	if (message->reply_serial == 0) {
+		if (_dbus_message_is_gvariant(message)) {
+			uint64_t reply_serial;
+
+			get_header_field(message,
+					DBUS_MESSAGE_FIELD_REPLY_SERIAL, 't',
+					&reply_serial);
+
+			message->reply_serial = reply_serial;
+		} else
+			get_header_field(message,
+					DBUS_MESSAGE_FIELD_REPLY_SERIAL, 'u',
 					&message->reply_serial);
+	}
 
 	return message->reply_serial;
 }
-- 
2.5.0

[PATCH v2 06/11] dbus: Don't rely on 1st KDBUS_ITEM_PAYLOAD_OFF being the header

[Andrew Zaborowski]

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

The header of the GVariant message can't be split into multiple
KDBUS_ITEM_PAYLOAD_OFF items according to the docs but it doesn't have
to be split from the body.  We need to use dbus_message_from_blob
because we can't know the header/body split positions without parsing
the received contents.
---
 ell/dbus-kernel.c | 75 +++++++++++++++----------------------------------------
 1 file changed, 20 insertions(+), 55 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 4a3aed0..7797153 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -449,84 +449,51 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 	return 0;
 }
 
-static int collect_body_parts(struct kdbus_msg *kmsg,
-					size_t header_size, void **out_header,
-					size_t body_size, void **out_body)
+static void collect_body_parts(struct kdbus_msg *kmsg, size_t size, void **data)
 {
 	struct kdbus_item *item;
-	void *body;
-	void *header;
-	bool saw_header = false;
 	size_t offset = 0;
 
-	header = l_malloc(header_size);
-	if (!header)
-		return -ENOMEM;
-
-	body = NULL;
-
-	if (body_size > 0) {
-		body = l_malloc(body_size);
-		if (!body) {
-			l_free(header);
-			return -ENOMEM;
-		}
-	}
+	*data = l_malloc(size);
 
 	KDBUS_ITEM_FOREACH(item, kmsg, items) {
 		switch (item->type) {
 		case KDBUS_ITEM_PAYLOAD_OFF:
-			if (saw_header) {
-				memcpy(body + offset,
-					(void *)kmsg + item->vec.offset,
-					item->vec.size);
+			memcpy(*data + offset, (void *) kmsg + item->vec.offset,
+				item->vec.size);
 
-				offset += item->vec.size;
-			} else {
-				memcpy(header, (void *)kmsg + item->vec.offset,
-					item->vec.size);
-				saw_header = true;
-			}
+			offset += item->vec.size;
 
 			break;
 		}
 	}
-
-	*out_body = body;
-	*out_header = header;
-
-	return 0;
 }
 
 static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 				struct l_dbus_message **out_message)
 {
 	struct kdbus_item *item;
-	void *header = 0;
-	size_t header_size = 0;
+	void *data = 0;
+	size_t size = 0;
 	struct dbus_header *hdr;
-	void *body = 0;
-	size_t body_size = 0;
-	int r;
 	const char *destination = 0;
 	char unique_bus_name[128];
 
 	KDBUS_ITEM_FOREACH(item, kmsg, items) {
 		switch (item->type) {
 		case KDBUS_ITEM_PAYLOAD_OFF:
-			if (!header_size) {
-				header = (void *)kmsg + item->vec.offset;
+			if (!size) {
+				if (item->vec.size < sizeof(struct dbus_header))
+					return -EBADMSG;
 
-				header_size = item->vec.size;
+				hdr = (void *)kmsg + item->vec.offset;
+			}
 
-				if (!_dbus_header_is_valid(header, header_size))
-					return -EBADMSG;
-			} else
-				body_size += item->vec.size;
+			size += item->vec.size;
 
 			break;
 		case KDBUS_ITEM_PAYLOAD_MEMFD:
-			if (!header_size)
+			if (!size)
 				return -EBADMSG;
 
 			return -ENOTSUP;
@@ -541,23 +508,21 @@ static int _dbus_kernel_make_message(struct kdbus_msg *kmsg,
 		}
 	}
 
-	if (!header)
+	if (!size)
 		return -EBADMSG;
 
-	hdr = header;
 	if (hdr->endian != DBUS_NATIVE_ENDIAN)
 		return -EPROTOTYPE;
 
 	if (hdr->version != 2)
 		return -EPROTO;
 
-	r = collect_body_parts(kmsg, header_size, &header,
-						body_size, &body);
-	if (r < 0)
-		return r;
+	collect_body_parts(kmsg, size, &data);
+
+	*out_message = dbus_message_from_blob(data, size);
+
+	l_free(data);
 
-	*out_message = dbus_message_build(header, header_size, body, body_size,
-						NULL, 0);
 	if (!*out_message)
 		return -EBADMSG;
 
-- 
2.5.0

[PATCH v2 07/11] gvariant: Utility to build GVariant message out of header+body

[Andrew Zaborowski]

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

Build a single parseable GVariant blob out of the header blob,
the body blob and the signature string.
---
 ell/gvariant-private.h |  4 ++++
 ell/gvariant-util.c    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/ell/gvariant-private.h b/ell/gvariant-private.h
index f8f1313..592ee8f 100644
--- a/ell/gvariant-private.h
+++ b/ell/gvariant-private.h
@@ -63,3 +63,7 @@ bool _gvariant_builder_leave_variant(struct dbus_builder *builder);
 bool _gvariant_builder_enter_array(struct dbus_builder *builder,
 					const char *signature);
 bool _gvariant_builder_leave_array(struct dbus_builder *builder);
+
+void _gvariant_message_build_contents(size_t header_end, const void *body,
+					size_t body_size, const char *signature,
+					uint8_t *out_buf, size_t *out_size);
diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 0baf1c6..fa1ca45 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -1317,3 +1317,49 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 
 	return signature;
 }
+
+/*
+ * Write the part of a GVariant message after the header + padding, i.e.
+ * starting at first byte of the body variant, into a buffer.  This includes
+ * the variant with its signature and the framing offset of the header
+ * fields array.
+ */
+void _gvariant_message_build_contents(size_t header_end, const void *body,
+					size_t body_size, const char *signature,
+					uint8_t *out_buf, size_t *out_size)
+{
+	size_t size;
+	size_t offset_size;
+	size_t pos;
+
+	if (!signature)
+		signature = "";
+
+	size = body_size + 1 +			/* Body plus zero byte */
+		strlen(signature) + 2 +		/* Msg signature plus '()' */
+		(signature[0] == '\0' ? 1 : 0);	/* Zero byte for empty struct */
+
+	offset_size = offset_length(align_len(header_end, 8) + size, 1);
+	size += offset_size;
+
+	if (out_size)
+		*out_size = size;
+
+	if (!out_buf)
+		return;
+
+	if (signature[0] != '\0') {
+		memcpy(out_buf, body, body_size);
+		pos = body_size;
+	} else {
+		out_buf[0] = 0;
+		pos = 1;
+	}
+
+	out_buf[pos++] = 0;
+	out_buf[pos++] = '(';
+	memcpy(out_buf + pos, signature, strlen(signature));
+	pos += strlen(signature);
+	out_buf[pos++] = ')';
+	write_word_le(out_buf + pos, header_end, offset_size);
+}
-- 
2.5.0

[PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Andrew Zaborowski]

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

Keep sending the message header+padding as a separate
KDBUS_ITEM_PAYLOAD_OFF buffer like in DBus-1, the systemd busctl utility
also seems to do that.  But fix what we send after the header.
---
 ell/dbus-kernel.c  | 17 +++++++++--------
 ell/dbus-message.c | 19 +++++++++++++++++++
 ell/dbus-private.h |  3 +++
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 7797153..da5920f 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -427,14 +427,15 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 	item->vec.size = header_size;
 	item = KDBUS_ITEM_NEXT(item);
 
-	body = _dbus_message_get_body(message, &body_size);
-	if (body_size > 0) {
-		item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct kdbus_vec);
-		item->type = KDBUS_ITEM_PAYLOAD_VEC;
-		item->vec.address = (uintptr_t) body;
-		item->vec.size = body_size;
-		item = KDBUS_ITEM_NEXT(item);
-	}
+	_dbus_message_build_contents(message, NULL, &body_size);
+	body = alloca(body_size);
+	_dbus_message_build_contents(message, body, NULL);
+
+	item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct kdbus_vec);
+	item->type = KDBUS_ITEM_PAYLOAD_VEC;
+	item->vec.address = (uintptr_t) body;
+	item->vec.size = body_size;
+	item = KDBUS_ITEM_NEXT(item);
 
 	kmsg->size = (void *)item - (void *)kmsg;
 
diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index eb47eb3..1a1e3ab 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -107,6 +107,25 @@ void *_dbus_message_get_body(struct l_dbus_message *msg, size_t *out_size)
 	return msg->body;
 }
 
+void _dbus_message_build_contents(struct l_dbus_message *msg,
+					uint8_t *out_buf, size_t *out_size)
+{
+	if (_dbus_message_is_gvariant(msg)) {
+		_gvariant_message_build_contents(msg->header_end, msg->body,
+							msg->body_size,
+							msg->signature,
+							out_buf, out_size);
+
+		return;
+	}
+
+	if (out_size)
+		*out_size = msg->body_size;
+
+	if (out_buf)
+		memcpy(out_buf, msg->body, msg->body_size);
+}
+
 void _dbus_message_set_serial(struct l_dbus_message *msg, uint32_t serial)
 {
 	struct dbus_header *hdr = msg->header;
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 764ed42..b7f00b6 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -141,6 +141,9 @@ bool dbus_message_compare(struct l_dbus_message *message,
 bool _dbus_message_builder_mark(struct l_dbus_message_builder *builder);
 bool _dbus_message_builder_rewind(struct l_dbus_message_builder *builder);
 
+void _dbus_message_build_contents(struct l_dbus_message *msg,
+					uint8_t *out_buf, size_t *out_size);
+
 const char *_dbus_signature_end(const char *signature);
 
 bool _dbus_valid_object_path(const char *path);
-- 
2.5.0

[PATCH v2 09/11] dbus: Rename _dbus_header_is_valid to _dbus1_header_is_valid

[Andrew Zaborowski]

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

---
 ell/dbus-private.h | 2 +-
 ell/dbus-util.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index b7f00b6..c921cf3 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -153,7 +153,7 @@ bool _dbus_valid_method(const char *method);
 bool _dbus_parse_unique_name(const char *name, uint64_t *out_id);
 bool _dbus_valid_bus_name(const char *bus_name);
 
-bool _dbus_header_is_valid(void *data, size_t size);
+bool _dbus1_header_is_valid(void *data, size_t size);
 
 void _dbus_method_introspection(struct _dbus_method *info,
 					struct l_string *buf);
diff --git a/ell/dbus-util.c b/ell/dbus-util.c
index ab0600a..e0e2bc7 100644
--- a/ell/dbus-util.c
+++ b/ell/dbus-util.c
@@ -412,7 +412,7 @@ const char *_dbus_signature_end(const char *signature)
 	return NULL;
 }
 
-bool _dbus_header_is_valid(void *data, size_t size)
+bool _dbus1_header_is_valid(void *data, size_t size)
 {
 	struct dbus_header *hdr;
 	size_t header_len;
-- 
2.5.0

[PATCH v2 10/11] gvariant: Fix empty structure/array parsing and error check

[Andrew Zaborowski]

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

Allow empty signatures in _gvariant_num_children and check the return
value for errors or we might crash.
---
 ell/gvariant-util.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index fa1ca45..07b4874 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -201,14 +201,14 @@ int _gvariant_num_children(const char *sig)
 	if (strlen(sig) > 255)
 		return false;
 
-	do {
+	while (*s) {
 		s = validate_next_type(s, &a);
 
 		if (!s)
 			return -1;
 
 		num_children += 1;
-	} while (*s);
+	}
 
 	return num_children;
 }
@@ -374,7 +374,7 @@ static bool gvariant_iter_init_internal(struct l_dbus_message_iter *iter,
 		unsigned int alignment : 4;
 		size_t end;		/* Index past the end of the type */
 	} *children;
-	uint8_t n_children;
+	int n_children;
 
 	if (sig_end) {
 		size_t len = sig_end - sig_start;
@@ -392,6 +392,9 @@ static bool gvariant_iter_init_internal(struct l_dbus_message_iter *iter,
 	iter->pos = 0;
 
 	n_children = _gvariant_num_children(subsig);
+	if (n_children < 0)
+		return false;
+
 	children = l_new(struct gvariant_type_info, n_children);
 
 	for (p = sig_start, i = 0; i < n_children; i++) {
-- 
2.5.0

[PATCH v2 11/11] unit: Fix GVariant test messages

[Andrew Zaborowski]

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

---
 unit/test-gvariant-message.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/unit/test-gvariant-message.c b/unit/test-gvariant-message.c
index c2cb811..b163a67 100644
--- a/unit/test-gvariant-message.c
+++ b/unit/test-gvariant-message.c
@@ -50,8 +50,8 @@ struct message_data {
 };
 
 static const unsigned char basic_1[] = {
-	0x6c, 0x01, 0x00, 0x02, 0x28, 0x00, 0x00, 0x00, 0x57, 0x04, 0x00, 0x00,
-	0x79, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x6c, 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x57, 0x04, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 	0x2f, 0x66, 0x6f, 0x6f, 0x2f, 0x62, 0x61, 0x72, 0x00, 0x00, 0x6f, 0x00,
 	0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 	0x46, 0x6f, 0x6f, 0x62, 0x61, 0x72, 0x00, 0x00, 0x73, 0x00, 0x00, 0x00,
@@ -65,7 +65,8 @@ static const unsigned char basic_1[] = {
 	0x01, 0xff, 0xe0, 0xff, 0x20, 0x00, 0x00, 0x00, 0xe8, 0xff, 0xff, 0xff,
 	0x18, 0x00, 0x00, 0x00, 0x9d, 0xff, 0xff, 0xff, 0x7d, 0x7f, 0x00, 0x00,
 	0x63, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-	0x00, 0x00, 0x14, 0x40,
+	0x00, 0x00, 0x14, 0x40, 0x00, 0x28, 0x62, 0x79, 0x6e, 0x71, 0x69, 0x75,
+	0x78, 0x74, 0x64, 0x29, 0x89,
 };
 
 static const struct message_data message_data_basic_1 = {
@@ -76,12 +77,12 @@ static const struct message_data message_data_basic_1 = {
 	.destination	= "foo.bar",
 	.signature	= "bynqiuxtd",
 	.binary		= basic_1,
-	.binary_len	= 184,
+	.binary_len	= sizeof(basic_1),
 };
 
 static const unsigned char message_binary_complex_1[] = {
-	0x6c, 0x01, 0x00, 0x02, 0x4f, 0x00, 0x00, 0x00, 0x57, 0x04, 0x00, 0x00,
-	0x86, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x6c, 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x57, 0x04, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 	0x2f, 0x63, 0x6f, 0x6d, 0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65,
 	0x2f, 0x6f, 0x62, 0x6a, 0x65, 0x63, 0x74, 0x00, 0x00, 0x6f, 0x00, 0x00,
 	0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x6d, 0x65, 0x74, 0x68,
@@ -99,7 +100,8 @@ static const unsigned char message_binary_complex_1[] = {
 	0x72, 0x76, 0x61, 0x6c, 0x64, 0x73, 0x00, 0x00, 0x73, 0x05, 0x00, 0x00,
 	0x00, 0x00, 0x00, 0x00, 0x44, 0x65, 0x76, 0x65, 0x6c, 0x6f, 0x70, 0x65,
 	0x72, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x62, 0x0a,
-	0x1a, 0x34, 0x14,
+	0x1a, 0x34, 0x14, 0x00, 0x28, 0x6f, 0x61, 0x7b, 0x73, 0x76, 0x7d, 0x29,
+	0x96,
 };
 
 static const struct message_data message_data_complex_1 = {
-- 
2.5.0

Re: [PATCH v2 01/11] dbus: Validate field type in get_header_field

[Denis Kenzior]

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

Hi Andrew,

On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
> It seems there's no place where we are validating the header
> fields of incoming messages so let's do it here.
> ---
>   ell/dbus-message.c | 35 ++++++++++++++++++++---------------
>   1 file changed, 20 insertions(+), 15 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v2 02/11] dbus: Check some more return values when parsing messages

[Denis Kenzior]

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

Hi Andrew,

On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
> Without this we will still segfault on some incoming messages.  Add a
> comment to clarify no get_header_field value check.
> ---
>   ell/dbus-kernel.c  |  2 ++
>   ell/dbus-message.c | 27 ++++++++++++++++++---------
>   2 files changed, 20 insertions(+), 9 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v2 04/11] dbus: Fix parsing GVariant header/body information

[Denis Kenzior]

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

Hi Andrew,

On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
> In GVariant messages there's no body size field (it is reserved and must
> be sent as 0) or header size information or signature.  We do still add
> the signature as a field same as in Dbus-1 because it's not prohibited
> but we can't rely on it being there.  The message needs to be parsed as
> a single GVariant blob if we want to split it into a header and a body.
> ---
>   ell/dbus-kernel.c  |   3 --
>   ell/dbus-message.c | 108 ++++++++++++++++++++++++++++++++++++-----------------
>   2 files changed, 73 insertions(+), 38 deletions(-)
>

<snip>

> @@ -653,6 +651,7 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
>   {
>   	const struct dbus_header *hdr = data;
>   	struct l_dbus_message *message;
> +	size_t body_pos;
>
>   	if (unlikely(size < DBUS_HEADER_SIZE))
>   		return NULL;
> @@ -661,28 +660,62 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
>
>   	message->refcount = 1;
>
> -	message->header_size = align_len(DBUS_HEADER_SIZE +
> -						hdr->field_length, 8);
> -	message->body_size = hdr->body_length;
> +	if (hdr->version == 1) {
> +		message->header_size = align_len(DBUS_HEADER_SIZE +
> +							hdr->field_length, 8);
> +		message->body_size = hdr->body_length;
>
> -	if (message->header_size + message->body_size < size) {
> -		l_free(message);
> -		return NULL;
> +		if (message->header_size + message->body_size < size)
> +			goto free;
> +
> +		body_pos = message->header_size;
> +	} else {
> +		struct l_dbus_message_iter iter, header,
> +			variant, structure, body;

Please break this up into multiple struct declarations.

e.g.

struct l_dbus_message_iter iter;
struct l_dbus_message_iter header, variant, body;

Just to group them better and make this more readable.

> +
> +		if (!_gvariant_iter_init(&iter, message, "((yyyyuta{tv})v)",
> +						NULL, data, size))

So quoting or pointing to:
https://wiki.gnome.org/Projects/GLib/GDBus/Version2
might be in order here.

It says: "The canonical type of a D-Bus version two message is 
(yyyyuta{tv}v)."

Also, you might want to put in a quick blurb why we're not using the 
canonical structure.

> +			goto free;
> +
> +		if (!_gvariant_iter_enter_struct(&iter, &structure))
> +			goto free;

Since the outer structure is implicit, do we really need this part? 
Can't we simply omit the outer '()'s? If so, the code can be simplified.

> +
> +		if (!_gvariant_iter_enter_struct(&structure, &header))
> +			goto free;
> +
> +		if (!_gvariant_iter_enter_variant(&structure, &variant))
> +			goto free;
> +
> +		if (!_gvariant_iter_enter_struct(&variant, &body))
> +			goto free;
> +
> +		message->header_size = align_len(header.len - header.pos, 8);

why do we need an align_len here?  This was probably specific to the 
classic and old kdbus formats.

> +		message->body_size = body.len - body.pos;
> +		message->signature = l_strndup(body.sig_start + body.sig_pos,
> +						body.sig_len - body.sig_pos);
> +		message->signature_free = true;
> +		message->header_end = header.len;
> +		body_pos = body.data + body.pos - data;
>   	}
>
>   	message->header = l_malloc(message->header_size);
>   	message->body = l_malloc(message->body_size);
>
>   	memcpy(message->header, data, message->header_size);
> -	memcpy(message->body, data + message->header_size, message->body_size);
> +	memcpy(message->body, data + body_pos, message->body_size);
>
>   	message->sealed = true;
>
>   	/* If the field is absent message->signature will remain NULL */
> -	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
> -						&message->signature);
> +	if (hdr->version == 1)
> +		get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
> +					'g', &message->signature);
>
>   	return message;
> +
> +free:
> +	l_free(message);
> +	return NULL;
>   }
>
>   struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
> @@ -698,6 +731,13 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
>   	if (unlikely(!valid_header(hdr)))
>   		return NULL;
>
> +	/*
> +	 * With GVariant we need to know the signature, use
> +	 * dbus_message_from_blob instead.
> +	 */
> +	if (unlikely(hdr->version != 1))
> +		return NULL;
> +
>   	message = l_new(struct l_dbus_message, 1);
>
>   	message->refcount = 1;
> @@ -827,21 +867,18 @@ static void build_header(struct l_dbus_message *message, const char *signature)
>   	char *generated_signature;
>   	struct dbus_header *hdr;
>   	size_t header_size;
> +	bool gvariant;
>
> -	if (_dbus_message_is_gvariant(message))
> +	gvariant = _dbus_message_is_gvariant(message);
> +
> +	if (gvariant)
>   		driver = &gvariant_driver;
>   	else
>   		driver = &dbus1_driver;
>
>   	builder = driver->new(message->header, message->header_size);
>
> -	if (_dbus_message_is_gvariant(message)) {
> -		uint32_t field_length = 0;
> -		driver->append_basic(builder, 'u', &field_length);
> -	}
> -
> -	driver->enter_array(builder, _dbus_message_is_gvariant(message) ?
> -				"(tv)" : "(yv)");
> +	driver->enter_array(builder, gvariant ? "(tv)" : "(yv)");
>
>   	if (message->path) {
>   		add_field(builder, driver, DBUS_MESSAGE_FIELD_PATH,
> @@ -906,15 +943,16 @@ static void build_header(struct l_dbus_message *message, const char *signature)
>   	hdr = message->header;
>
>   	if (_dbus_message_is_gvariant(message))
> -		hdr->field_length = header_size - 16;
> -
> -	hdr->body_length = message->body_size;
> +		hdr->body_length = 0;
> +	else
> +		hdr->body_length = message->body_size;
>
>   	/* We must align the end of the header to an 8-byte boundary */
>   	message->header_size = align_len(header_size, 8);
>   	message->header = l_realloc(message->header, message->header_size);
>   	memset(message->header + header_size, 0,
>   			message->header_size - header_size);
> +	message->header_end = header_size;
>   }
>
>   struct container {
>

Regards,
-Denis

Re: [PATCH v2 05/11] dbus: GVariant message cookie and reply cookie are 64-bit

[Denis Kenzior]

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

Hi Andrew,

On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
> They're 64-bit but the systemd docs discourage using the upper 32 bits,
> so we take advantage of that and only support cookie values that fit 32
> bits to avoid too big code changes (we use the dbus1 header layout for
> the cookie storage so we'd have to move the cookie/serial out of that
> header like we do with sender, path, etc.).  This only tries to fix the
> parsing and encoding.
> ---
>   ell/dbus-message.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 52 insertions(+), 8 deletions(-)
>
> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
> index 964b925..eb47eb3 100644
> --- a/ell/dbus-message.c
> +++ b/ell/dbus-message.c
> @@ -111,13 +111,27 @@ void _dbus_message_set_serial(struct l_dbus_message *msg, uint32_t serial)
>   {
>   	struct dbus_header *hdr = msg->header;
>
> -	hdr->serial = serial;
> +	if (_dbus_message_is_gvariant(msg)) {
> +		if (_dbus_message_get_endian(msg) ==
> +				DBUS_MESSAGE_LITTLE_ENDIAN) {
> +			hdr->serial = serial;
> +			hdr->field_length = 0;
> +		} else {
> +			hdr->serial = 0;
> +			hdr->field_length = serial;
> +		}

This looks really complicated.  Can we just add a union to dbus_header 
instead to reflect the new GVariant serialization format?

> +	} else
> +		hdr->serial = serial;
>   }
>
>   uint32_t _dbus_message_get_serial(struct l_dbus_message *msg)
>   {
>   	struct dbus_header *hdr = msg->header;
>
> +	if (_dbus_message_is_gvariant(msg) && _dbus_message_get_endian(msg) ==
> +			DBUS_MESSAGE_BIG_ENDIAN)
> +		return hdr->field_length;
> +
>   	return hdr->serial;
>   }
>

Regards,
-Denis

Re: [PATCH v2 07/11] gvariant: Utility to build GVariant message out of header+body

[Denis Kenzior]

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

Hi Andrew,

 > +	out_buf[pos++] = 0;
> +	out_buf[pos++] = '(';
> +	memcpy(out_buf + pos, signature, strlen(signature));
> +	pos += strlen(signature);
> +	out_buf[pos++] = ')';

Is this written down anywhere?  Why is the body enclosed in a structure?

Do we handle this structure in the parsing phase somewhere?  Can't seem 
to locate that.

> +	write_word_le(out_buf + pos, header_end, offset_size);
> +}
>

Regards,
-Denis

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew,

On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
> Keep sending the message header+padding as a separate
> KDBUS_ITEM_PAYLOAD_OFF buffer like in DBus-1, the systemd busctl utility
> also seems to do that.  But fix what we send after the header.
> ---
>   ell/dbus-kernel.c  | 17 +++++++++--------
>   ell/dbus-message.c | 19 +++++++++++++++++++
>   ell/dbus-private.h |  3 +++
>   3 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
> index 7797153..da5920f 100644
> --- a/ell/dbus-kernel.c
> +++ b/ell/dbus-kernel.c
> @@ -427,14 +427,15 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
>   	item->vec.size = header_size;
>   	item = KDBUS_ITEM_NEXT(item);
>
> -	body = _dbus_message_get_body(message, &body_size);
> -	if (body_size > 0) {
> -		item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct kdbus_vec);
> -		item->type = KDBUS_ITEM_PAYLOAD_VEC;
> -		item->vec.address = (uintptr_t) body;
> -		item->vec.size = body_size;
> -		item = KDBUS_ITEM_NEXT(item);
> -	}
> +	_dbus_message_build_contents(message, NULL, &body_size);
> +	body = alloca(body_size);

This will fail badly somewhere around body_size >= 64k, depending on the 
platform.  Why can't we write the contents directly like we used to?

> +	_dbus_message_build_contents(message, body, NULL);
> +
> +	item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct kdbus_vec);
> +	item->type = KDBUS_ITEM_PAYLOAD_VEC;
> +	item->vec.address = (uintptr_t) body;
> +	item->vec.size = body_size;
> +	item = KDBUS_ITEM_NEXT(item);
>
>   	kmsg->size = (void *)item - (void *)kmsg;
>

Regards,
-Denis

Re: [PATCH v2 05/11] dbus: GVariant message cookie and reply cookie are 64-bit

[Andrzej Zaborowski]

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

Hi Denis,

On 12 April 2016 at 23:18, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
>>
>> They're 64-bit but the systemd docs discourage using the upper 32 bits,
>> so we take advantage of that and only support cookie values that fit 32
>> bits to avoid too big code changes (we use the dbus1 header layout for
>> the cookie storage so we'd have to move the cookie/serial out of that
>> header like we do with sender, path, etc.).  This only tries to fix the
>> parsing and encoding.
>> ---
>>   ell/dbus-message.c | 60
>> ++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 52 insertions(+), 8 deletions(-)
>>
>> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
>> index 964b925..eb47eb3 100644
>> --- a/ell/dbus-message.c
>> +++ b/ell/dbus-message.c
>> @@ -111,13 +111,27 @@ void _dbus_message_set_serial(struct l_dbus_message
>> *msg, uint32_t serial)
>>   {
>>         struct dbus_header *hdr = msg->header;
>>
>> -       hdr->serial = serial;
>> +       if (_dbus_message_is_gvariant(msg)) {
>> +               if (_dbus_message_get_endian(msg) ==
>> +                               DBUS_MESSAGE_LITTLE_ENDIAN) {
>> +                       hdr->serial = serial;
>> +                       hdr->field_length = 0;
>> +               } else {
>> +                       hdr->serial = 0;
>> +                       hdr->field_length = serial;
>> +               }
>
>
> This looks really complicated.  Can we just add a union to dbus_header
> instead to reflect the new GVariant serialization format?

Ok, I also think that will be cleaner but will need to touch more
places in dbus-message.c to do this change.

Best regards

Re: [PATCH v2 07/11] gvariant: Utility to build GVariant message out of header+body

[Andrzej Zaborowski]

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

On 12 April 2016 at 23:44, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
>> +     out_buf[pos++] = 0;
>>
>> +       out_buf[pos++] = '(';
>> +       memcpy(out_buf + pos, signature, strlen(signature));
>> +       pos += strlen(signature);
>> +       out_buf[pos++] = ')';
>
>
> Is this written down anywhere?  Why is the body enclosed in a structure?

Well, the message signature is not a single dbus type but a sequence
of types (possibly empty) and variant only accepts one dbus type so we
need a structure.

>
> Do we handle this structure in the parsing phase somewhere?  Can't seem to
> locate that.

In patch 04 in dbus_message_from_blob I believe the last
_gvariant_iter_enter_struct() handles that.

Best regards

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Andrzej Zaborowski]

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

Hi Denis,

On 12 April 2016 at 23:46, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
>>
>> Keep sending the message header+padding as a separate
>> KDBUS_ITEM_PAYLOAD_OFF buffer like in DBus-1, the systemd busctl utility
>> also seems to do that.  But fix what we send after the header.
>> ---
>>   ell/dbus-kernel.c  | 17 +++++++++--------
>>   ell/dbus-message.c | 19 +++++++++++++++++++
>>   ell/dbus-private.h |  3 +++
>>   3 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
>> index 7797153..da5920f 100644
>> --- a/ell/dbus-kernel.c
>> +++ b/ell/dbus-kernel.c
>> @@ -427,14 +427,15 @@ int _dbus_kernel_send(int fd, size_t bloom_size,
>> uint8_t bloom_n_hash,
>>         item->vec.size = header_size;
>>         item = KDBUS_ITEM_NEXT(item);
>>
>> -       body = _dbus_message_get_body(message, &body_size);
>> -       if (body_size > 0) {
>> -               item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct
>> kdbus_vec);
>> -               item->type = KDBUS_ITEM_PAYLOAD_VEC;
>> -               item->vec.address = (uintptr_t) body;
>> -               item->vec.size = body_size;
>> -               item = KDBUS_ITEM_NEXT(item);
>> -       }
>> +       _dbus_message_build_contents(message, NULL, &body_size);
>> +       body = alloca(body_size);
>
>
> This will fail badly somewhere around body_size >= 64k, depending on the
> platform.  Why can't we write the contents directly like we used to?

Hmm true.  But the body is now enclosed in a variant, so what buffer
do you suggest to write that into and assign to item->ver.address?

Best regards

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew,

On 04/12/2016 05:10 PM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 12 April 2016 at 23:46, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
>>>
>>> Keep sending the message header+padding as a separate
>>> KDBUS_ITEM_PAYLOAD_OFF buffer like in DBus-1, the systemd busctl utility
>>> also seems to do that.  But fix what we send after the header.
>>> ---
>>>    ell/dbus-kernel.c  | 17 +++++++++--------
>>>    ell/dbus-message.c | 19 +++++++++++++++++++
>>>    ell/dbus-private.h |  3 +++
>>>    3 files changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
>>> index 7797153..da5920f 100644
>>> --- a/ell/dbus-kernel.c
>>> +++ b/ell/dbus-kernel.c
>>> @@ -427,14 +427,15 @@ int _dbus_kernel_send(int fd, size_t bloom_size,
>>> uint8_t bloom_n_hash,
>>>          item->vec.size = header_size;
>>>          item = KDBUS_ITEM_NEXT(item);
>>>
>>> -       body = _dbus_message_get_body(message, &body_size);
>>> -       if (body_size > 0) {
>>> -               item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct
>>> kdbus_vec);
>>> -               item->type = KDBUS_ITEM_PAYLOAD_VEC;
>>> -               item->vec.address = (uintptr_t) body;
>>> -               item->vec.size = body_size;
>>> -               item = KDBUS_ITEM_NEXT(item);
>>> -       }
>>> +       _dbus_message_build_contents(message, NULL, &body_size);
>>> +       body = alloca(body_size);
>>
>>
>> This will fail badly somewhere around body_size >= 64k, depending on the
>> platform.  Why can't we write the contents directly like we used to?
>
> Hmm true.  But the body is now enclosed in a variant, so what buffer
> do you suggest to write that into and assign to item->ver.address?
>

This is actually a nasty issue with kdbus.  As you say, variant must be 
a single type, this is specified in DBus-1 Spec somewhere.  So if we are 
to enclose it in a variant like kdbus wants, then it either needs to be:
1. enclosed in '()'
2. special cased.

You're doing 1, however won't this result in the message signature being 
different depending on the the transport being used?

Can we special case the building of this variant for gvariant based 
messages?  Similar to how we're generating the signature on the fly in 
the builder?

Regards,
-Denis

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Andrzej Zaborowski]

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

Hi Denis,

On 13 April 2016 at 00:25, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/12/2016 05:10 PM, Andrzej Zaborowski wrote:
>> Hmm true.  But the body is now enclosed in a variant, so what buffer
>> do you suggest to write that into and assign to item->ver.address?
>>
>
> This is actually a nasty issue with kdbus.  As you say, variant must be a
> single type, this is specified in DBus-1 Spec somewhere.  So if we are to
> enclose it in a variant like kdbus wants, then it either needs to be:
> 1. enclosed in '()'
> 2. special cased.
>
> You're doing 1, however won't this result in the message signature being
> different depending on the the transport being used?

Why?  If we assume that the message's signature is the type in the
variant witout the () I don't see any ambiguity there.

I didn't find a specific statement of that in the docs although
https://wiki.gnome.org/Projects/GLib/GDBus/Version2 does say:

"The body of the message is inside of the variant at the end. The body
must be a tuple. "

>
> Can we special case the building of this variant for gvariant based
> messages?  Similar to how we're generating the signature on the fly in the
> builder?

Is it worth it though, just to save a memcpy?

Best regards

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Andrzej Zaborowski]

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

On 13 April 2016 at 01:06, Andrzej Zaborowski
<andrew.zaborowski@intel.com> wrote:
> On 13 April 2016 at 00:25, Denis Kenzior <denkenz@gmail.com> wrote:
>> Can we special case the building of this variant for gvariant based
>> messages?  Similar to how we're generating the signature on the fly in the
>> builder?
>
> Is it worth it though, just to save a memcpy?

BTW we still need to add the offset after the variant, which is the
header size, so it wouldn't save the memcpy.

Best regards

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew,

On 04/12/2016 06:06 PM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 13 April 2016 at 00:25, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 04/12/2016 05:10 PM, Andrzej Zaborowski wrote:
>>> Hmm true.  But the body is now enclosed in a variant, so what buffer
>>> do you suggest to write that into and assign to item->ver.address?
>>>
>>
>> This is actually a nasty issue with kdbus.  As you say, variant must be a
>> single type, this is specified in DBus-1 Spec somewhere.  So if we are to
>> enclose it in a variant like kdbus wants, then it either needs to be:
>> 1. enclosed in '()'
>> 2. special cased.
>>
>> You're doing 1, however won't this result in the message signature being
>> different depending on the the transport being used?
>
> Why?  If we assume that the message's signature is the type in the
> variant witout the () I don't see any ambiguity there.
>

 From what I understood, the current setup will result in 
l_dbus_message_get_signature() returning different results, depending on 
transport.

So we probably need to some extra handling to make that extra set of 
'()' hidden.

> I didn't find a specific statement of that in the docs although
> https://wiki.gnome.org/Projects/GLib/GDBus/Version2 does say:
>
> "The body of the message is inside of the variant at the end. The body
> must be a tuple. "
>

That's not terribly specific.  How does the kdbus signature look like?

>>
>> Can we special case the building of this variant for gvariant based
>> messages?  Similar to how we're generating the signature on the fly in the
>> builder?
>
> Is it worth it though, just to save a memcpy?
>

If the memcpy is like 64k, then it might be worth it.

Can we generate the needed variant inside dbus_message_builder_finalize? 
or somewhere around that time?

And this whole thing is a great example of how not to design a 
marshaling format.

Regards,
-Denis

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew

On 04/12/2016 06:11 PM, Andrzej Zaborowski wrote:
> On 13 April 2016 at 01:06, Andrzej Zaborowski
> <andrew.zaborowski@intel.com> wrote:
>> On 13 April 2016 at 00:25, Denis Kenzior <denkenz@gmail.com> wrote:
>>> Can we special case the building of this variant for gvariant based
>>> messages?  Similar to how we're generating the signature on the fly in the
>>> builder?
>>
>> Is it worth it though, just to save a memcpy?
>
> BTW we still need to add the offset after the variant, which is the
> header size, so it wouldn't save the memcpy.
>

Yikes, these people are more insane than I thought ;)

Why do they insist on:
"When transmitting over kdbus, the entire content of the header (all 
parts up to and including the extended header dictionary) must be 
transmitted inside of the first payload item (either vector or memfd). 
The content may be merged with the body (or part of the body) into the 
same item, but the entire header must appear entirely within the first 
item. "

If the actual field length is only known WAAAY later?

Anyway, can we add the offsets inside builder_finalize again.  Or 
alternatively as a separate KDBUS_ITEM?

Regards,
-Denis

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Andrzej Zaborowski]

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

Hi Denis,

On 13 April 2016 at 01:18, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/12/2016 06:06 PM, Andrzej Zaborowski wrote:
>> On 13 April 2016 at 00:25, Denis Kenzior <denkenz@gmail.com> wrote:
>>> On 04/12/2016 05:10 PM, Andrzej Zaborowski wrote:
>>>> Hmm true.  But the body is now enclosed in a variant, so what buffer
>>>> do you suggest to write that into and assign to item->ver.address?
>>>>
>>>
>>> This is actually a nasty issue with kdbus.  As you say, variant must be a
>>> single type, this is specified in DBus-1 Spec somewhere.  So if we are to
>>> enclose it in a variant like kdbus wants, then it either needs to be:
>>> 1. enclosed in '()'
>>> 2. special cased.
>>>
>>> You're doing 1, however won't this result in the message signature being
>>> different depending on the the transport being used?
>>
>>
>> Why?  If we assume that the message's signature is the type in the
>> variant witout the () I don't see any ambiguity there.
>>
>
> From what I understood, the current setup will result in
> l_dbus_message_get_signature() returning different results, depending on
> transport.

Could you give an example?  For a GetProperties reply
l_dbus_message_get_signature() should return "a{sv}" on both kdbus and
dbus1.  On kdbus the we'll have a (yyyyuta{tv}v) gvariant where the
variant contians a (a{sv}) gvariant, but the API should completely
hide that unless I screwed it up.

>
> So we probably need to some extra handling to make that extra set of '()'
> hidden.
>
>> I didn't find a specific statement of that in the docs although
>> https://wiki.gnome.org/Projects/GLib/GDBus/Version2 does say:
>>
>> "The body of the message is inside of the variant at the end. The body
>> must be a tuple. "
>>
>
> That's not terribly specific.  How does the kdbus signature look like?
>
>>>
>>> Can we special case the building of this variant for gvariant based
>>> messages?  Similar to how we're generating the signature on the fly in
>>> the
>>> builder?
>>
>>
>> Is it worth it though, just to save a memcpy?
>>
>
> If the memcpy is like 64k, then it might be worth it.
>
> Can we generate the needed variant inside dbus_message_builder_finalize? or
> somewhere around that time?

But we'll still need a memcpy, unless we use a separate io vector when
sending the message to the kdbus FD, but I'd have to check if it
doesn't break the splitting rules.

>
> And this whole thing is a great example of how not to design a marshaling
> format.

Well, for one it gets rid of the split between the header and body in
Dbus1 which was unneeded and while doing that is saves bytes because
the variant adds fewer extra bytes than the
DBUS_MESSAGE_FIELD_SIGNATURE field in Dbus1 ;)

Best regards

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew,

 >>  From what I understood, the current setup will result in
>> l_dbus_message_get_signature() returning different results, depending on
>> transport.
>
> Could you give an example?  For a GetProperties reply
> l_dbus_message_get_signature() should return "a{sv}" on both kdbus and
> dbus1.  On kdbus the we'll have a (yyyyuta{tv}v) gvariant where the
> variant contians a (a{sv}) gvariant, but the API should completely
> hide that unless I screwed it up.

Let me look at this again later, I might not have understood correctly. 
  If you say this works, then I'm okay with this.

>> And this whole thing is a great example of how not to design a marshaling
>> format.
>
> Well, for one it gets rid of the split between the header and body in
> Dbus1 which was unneeded and while doing that is saves bytes because
> the variant adds fewer extra bytes than the
> DBUS_MESSAGE_FIELD_SIGNATURE field in Dbus1 ;)
>

Uh-huh.  DBus-1 was still sane compared to this.  You could peek at the 
header and discard messages you didn't care about.  No so here.

So the major improvement to DBus-1 is that the protocol becomes a 
nightmare to implement.  You basically have to have everything 
implemented before getting to hello world.

Not to mention the cache-miss city with the 
offset-list-in-little-endian-at-the-end crap ;)

Oh well, it is what it is.

Regards,
-Denis

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew,

 > But we'll still need a memcpy, unless we use a separate io vector when
> sending the message to the kdbus FD, but I'd have to check if it
> doesn't break the splitting rules.
>

So one thought here would be to add extra space in the beginning of the 
body used by gvariant builder.  That way we can fill it with the variant 
contents when we finalize the builder.  We might need to keep an extra 
book-keeping variable, something like body_offset, to know where the 
body actually begins.  It will be always 0 for dbus-1.

Perhaps you can think of something better.

Regards,
-Denis

Re: [PATCH v2 08/11] dbus: Fix the kdbus message encoding

[Andrzej Zaborowski]

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

Hi Denis,

On 13 April 2016 at 02:14, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
>> But we'll still need a memcpy, unless we use a separate io vector when
>>
>> sending the message to the kdbus FD, but I'd have to check if it
>> doesn't break the splitting rules.
>>
>
> So one thought here would be to add extra space in the beginning of the body
> used by gvariant builder.  That way we can fill it with the variant contents
> when we finalize the builder.  We might need to keep an extra book-keeping
> variable, something like body_offset, to know where the body actually
> begins.  It will be always 0 for dbus-1.

This is something we could do.  Actually the variant stuff (the 0 byte
and the signature) and the offset are all at the end so it's easy.

So we could have the gvariant builder fill in the signature when
finalizing the body and another function fill in the offset byte(s).
Perhaps I need to rename _dbus_message_build_contents to something
like _dbus_message_get_post_header or something else to diferentiate
if from _dbus_message_get_body.

Best regards