[PATCH 01/12] dbus: Kdbus message timeout_ns is absolute time

[PATCH 01/12] dbus: Kdbus message timeout_ns is absolute time

[Andrew Zaborowski]

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

It is even documented in systemd, although not very clearly, that
timeout_ns is a CLOCK_MONOTONIC_COARSE-based absolute value.
---
 ell/dbus-kernel.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index f45ec04..2b41177 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -34,6 +34,7 @@
 #include <errno.h>
 #include <sys/mman.h>
 #include <sys/ioctl.h>
+#include <time.h>
 
 #include "linux/kdbus.h"
 
@@ -392,8 +393,16 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 		break;
 	}
 	case DBUS_MESSAGE_TYPE_METHOD_CALL:
-		if (!l_dbus_message_get_no_reply(message))
-			kmsg->timeout_ns = 30000 * 1000000ULL;
+		if (!l_dbus_message_get_no_reply(message)) {
+			struct timespec now;
+
+			clock_gettime(CLOCK_MONOTONIC_COARSE, &now);
+
+			kmsg->timeout_ns =
+				now.tv_sec * 1000000000ULL + now.tv_nsec +
+				30000 * 1000000ULL;
+		}
+
 		break;
 	case DBUS_MESSAGE_TYPE_SIGNAL:
 		kmsg->flags |= KDBUS_MSG_SIGNAL;
-- 
2.5.0

[PATCH 02/12] dbus: Validate field type in get_header_field

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 5260 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..6e2831b 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 03/12] dbus: Check some more return values when parsing messages

[Andrew Zaborowski]

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

Without this we will still segfault on some incoming messages.
---
 ell/dbus-kernel.c  |  2 ++
 ell/dbus-message.c | 32 +++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 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 6e2831b..92054d1 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);
@@ -695,8 +700,11 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 
 	message->sealed = true;
 
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
-						&message->signature);
+	if (!get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
+						&message->signature)) {
+		l_free(message);
+		return NULL;
+	}
 
 	return message;
 }
@@ -1179,10 +1187,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 04/12] dbus: GVariant header field identifiers are 64-bit

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3719 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 | 47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 92054d1..085c5e1 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,19 @@ 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,
-					message->header + 16, header_length))
+		if (!_gvariant_iter_init(&header, message, "a(tv)", NULL,
 			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 +599,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,
@@ -796,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);
@@ -824,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 05/12] dbus: Fix parsing GVariant header/body information

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 7051 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 | 105 +++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 74 insertions(+), 34 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 085c5e1..b5bbd5c 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 {
@@ -387,6 +389,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,19 +572,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,
+					message->header_end - 16))
 			return false;
 
 		if (!_gvariant_iter_enter_array(&header, &array))
@@ -652,6 +649,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;
@@ -660,27 +658,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;
 
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
-						&message->signature);
+	if (hdr->version == 1)
+		if (!get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
+					'g', &message->signature))
+			goto free;
 
 	return message;
+
+free:
+	l_free(message);
+	return NULL;
 }
 
 struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
@@ -696,6 +729,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,23 @@ 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);
+	header_size = message->header_size;
 
-	if (_dbus_message_is_gvariant(message)) {
-		uint32_t field_length = 0;
-		driver->append_basic(builder, 'u', &field_length);
-	}
+	if (gvariant)
+		header_size += 4;
+
+	builder = driver->new(message->header, header_size);
 
-	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 +948,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 06/12] 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 b5bbd5c..dc05694 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;
 }
 
@@ -292,7 +306,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)
@@ -319,7 +333,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)
@@ -639,8 +653,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;
 }
@@ -921,8 +944,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;
 	}
 
@@ -1356,9 +1389,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 07/12] dbus: Don't rely on 1st KDBUS_ITEM_PAYLOAD_OFF being the header

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3560 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 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 08/12] 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 09/12] 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 dc05694..e46bfaa 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

Re: [PATCH 01/12] dbus: Kdbus message timeout_ns is absolute time

[Denis Kenzior]

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

Hi Andrew,

On 04/09/2016 11:02 PM, Andrew Zaborowski wrote:
> It is even documented in systemd, although not very clearly, that
> timeout_ns is a CLOCK_MONOTONIC_COARSE-based absolute value.
> ---
>   ell/dbus-kernel.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 02/12] dbus: Validate field type in get_header_field

[Denis Kenzior]

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

Hi Andrew,

On 04/09/2016 11:02 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(-)
>
> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
> index ece98a4..6e2831b 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;
> +

   CC       ell/dbus-message.lo
ell/dbus-message.c: In function ‘get_header_field_from_iter_valist’:
ell/dbus-message.c:595:26: error: invalid type argument of ‘->’ (have 
‘struct l_dbus_message_iter’)
    if (iter.sig_start[iter->sig_pos] != data_type)
                           ^
Should that be iter.sig_pos?

>   		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);
>
>

Regards,
-Denis

Re: [PATCH 02/12] dbus: Validate field type in get_header_field

[Andrzej Zaborowski]

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

Hi Denis,

On 11 April 2016 at 22:00, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 04/09/2016 11:02 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(-)
>>
>> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
>> index ece98a4..6e2831b 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;
>> +
>
>
>   CC       ell/dbus-message.lo
> ell/dbus-message.c: In function ‘get_header_field_from_iter_valist’:
> ell/dbus-message.c:595:26: error: invalid type argument of ‘->’ (have
> ‘struct l_dbus_message_iter’)
>    if (iter.sig_start[iter->sig_pos] != data_type)
>                           ^
> Should that be iter.sig_pos?

Yes, unfortunately I did a poor job splitting my changes into patches
and the intermediate states don't compile, for example after this
patch 2 and after patch 4.  Note patches 4-5 also breaks something,
I'm going to resend them when I fix the issue.

Best regards

Re: [PATCH 03/12] dbus: Check some more return values when parsing messages

[Denis Kenzior]

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

Hi Andrew,

On 04/09/2016 11:02 PM, Andrew Zaborowski wrote:
> Without this we will still segfault on some incoming messages.
> ---
>   ell/dbus-kernel.c  |  2 ++
>   ell/dbus-message.c | 32 +++++++++++++++++++++-----------
>   2 files changed, 23 insertions(+), 11 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 6e2831b..92054d1 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);
> @@ -695,8 +700,11 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
>
>   	message->sealed = true;
>
> -	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
> -						&message->signature);
> +	if (!get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
> +						&message->signature)) {

Don't think you can do that actually.

According to DBus Spec:
"The signature of the message body. If omitted, it is assumed to be the 
empty signature "" (i.e. the body must be 0-length)."

> +		l_free(message);
> +		return NULL;
> +	}
>
>   	return message;
>   }
> @@ -1179,10 +1187,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);
>
>

Regards,
-Denis

Re: [PATCH 03/12] dbus: Check some more return values when parsing messages

[Andrzej Zaborowski]

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

Hi Denis,

On 11 April 2016 at 22:11, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/09/2016 11:02 PM, Andrew Zaborowski wrote:
>>
>> Without this we will still segfault on some incoming messages.
>> ---
>>   ell/dbus-kernel.c  |  2 ++
>>   ell/dbus-message.c | 32 +++++++++++++++++++++-----------
>>   2 files changed, 23 insertions(+), 11 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 6e2831b..92054d1 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);
>> @@ -695,8 +700,11 @@ struct l_dbus_message *dbus_message_build(void
>> *header, size_t header_size,
>>
>>         message->sealed = true;
>>
>> -       get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
>> -                                               &message->signature);
>> +       if (!get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
>> +                                               &message->signature)) {
>
>
> Don't think you can do that actually.
>
> According to DBus Spec:
> "The signature of the message body. If omitted, it is assumed to be the
> empty signature "" (i.e. the body must be 0-length)."


Good point! This is what made the dbus-properties unit test fail for me.

Best regards