[PATCH v3 1/9] dbus: Update GVariant message header format

[PATCH v3 1/9] dbus: Update GVariant message header format

[Andrew Zaborowski]

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

In the current GVariant message format since about Jan 2015 there's no
body size field (it is reserved and must be sent as 0) or header size
information or signature.  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 | 167 ++++++++++++++++++++++++++++++++++++++---------------
 ell/dbus-private.h |  16 ++++-
 ell/dbus-util.c    |   4 +-
 ell/dbus.c         |   4 +-
 5 files changed, 137 insertions(+), 57 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..9fdc0ff 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 {
@@ -109,14 +111,20 @@ 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))
+		hdr->kdbus.cookie = serial;
+	else
+		hdr->dbus1.serial = serial;
 }
 
 uint32_t _dbus_message_get_serial(struct l_dbus_message *msg)
 {
 	struct dbus_header *hdr = msg->header;
 
-	return hdr->serial;
+	if (_dbus_message_is_gvariant(msg))
+		return hdr->kdbus.cookie;
+	else
+		return hdr->dbus1.serial;
 }
 
 LIB_EXPORT bool l_dbus_message_set_no_reply(struct l_dbus_message *msg, bool on)
@@ -196,10 +204,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;
@@ -290,7 +300,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)
@@ -317,7 +327,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)
@@ -387,6 +397,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 +580,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))
@@ -643,8 +647,14 @@ 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->dbus1.serial == 0)
+			return false;
+	} else {
+		/* We don't support 64-bit GVariant cookies */
+		if (hdr->kdbus.cookie == 0 || (hdr->kdbus.cookie >> 32))
+			return false;
+	}
 
 	return true;
 }
@@ -653,6 +663,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 +672,66 @@ 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->dbus1.field_length, 8);
+		message->body_size = hdr->dbus1.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;
+		struct l_dbus_message_iter header, variant, body;
+
+		/*
+		 * GVariant message structure as per
+		 * https://wiki.gnome.org/Projects/GLib/GDBus/Version2
+		 * is "(yyyyuta{tv}v)".  As noted this is equivalent to
+		 * some other types, this one lets us get iterators for
+		 * the header and the body in the fewest steps.
+		 */
+		if (!_gvariant_iter_init(&iter, message, "(yyyyuta{tv})v",
+						NULL, data, size))
+			goto free;
+
+		if (!_gvariant_iter_enter_struct(&iter, &header))
+			goto free;
+
+		if (!_gvariant_iter_enter_variant(&iter, &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 +747,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;
@@ -825,23 +881,19 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 	struct dbus_builder *builder;
 	struct builder_driver *driver;
 	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,
@@ -879,8 +931,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;
 	}
 
@@ -903,18 +965,18 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 
 	driver->free(builder);
 
-	hdr = message->header;
-
-	if (_dbus_message_is_gvariant(message))
-		hdr->field_length = header_size - 16;
+	if (!_dbus_message_is_gvariant(message)) {
+		struct dbus_header *hdr = message->header;
 
-	hdr->body_length = message->body_size;
+		hdr->dbus1.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 {
@@ -1313,9 +1375,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;
 }
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 764ed42..6a4ea96 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -49,9 +49,19 @@ struct dbus_header {
 	uint8_t  message_type;
 	uint8_t  flags;
 	uint8_t  version;
-	uint32_t body_length;
-	uint32_t serial;
-	uint32_t field_length;
+
+	union {
+		struct {
+			uint32_t body_length;
+			uint32_t serial;
+			uint32_t field_length;
+		} __attribute__ ((packed)) dbus1;
+
+		struct {
+			uint32_t reserved1;
+			uint64_t cookie;
+		} __attribute__ ((packed)) kdbus;
+	};
 } __attribute__ ((packed));
 #define DBUS_HEADER_SIZE 16
 
diff --git a/ell/dbus-util.c b/ell/dbus-util.c
index ab0600a..900b71b 100644
--- a/ell/dbus-util.c
+++ b/ell/dbus-util.c
@@ -423,9 +423,9 @@ bool _dbus_header_is_valid(void *data, size_t size)
 	hdr = data;
 
 	if (hdr->endian != DBUS_NATIVE_ENDIAN)
-		header_len = bswap_32(hdr->field_length);
+		header_len = bswap_32(hdr->dbus1.field_length);
 	else
-		header_len = hdr->field_length;
+		header_len = hdr->dbus1.field_length;
 
 	header_len += sizeof(struct dbus_header);
 	return size >= header_len;
diff --git a/ell/dbus.c b/ell/dbus.c
index 81a7f9e..9bb47ae 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -613,10 +613,10 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 	if (len != DBUS_HEADER_SIZE)
 		return NULL;
 
-	header_size = align_len(DBUS_HEADER_SIZE + hdr.field_length, 8);
+	header_size = align_len(DBUS_HEADER_SIZE + hdr.dbus1.field_length, 8);
 	header = l_malloc(header_size);
 
-	body_size = hdr.body_length;
+	body_size = hdr.dbus1.body_length;
 	body = l_malloc(body_size);
 
 	iov[0].iov_base = header;
-- 
2.5.0

[PATCH v3 2/9] 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 v3 3/9] gvariant: New gvariant message format support

[Andrew Zaborowski]

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

Make the builder write a complete struct enclosed in a variant and add a
utility to add the final framing offset to produce a single parseable
GVariant blob.
---
 ell/gvariant-private.h |  4 ++++
 ell/gvariant-util.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/ell/gvariant-private.h b/ell/gvariant-private.h
index f8f1313..6c0005a 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);
+
+size_t _gvariant_message_finalize(size_t header_end,
+					void *body, size_t body_size,
+					const char *signature);
diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 0baf1c6..e07bd2f 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -1292,6 +1292,8 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 {
 	char *signature;
 	struct container *root;
+	uint8_t *variant_buf;
+	size_t size;
 
 	if (unlikely(!builder))
 		return NULL;
@@ -1307,9 +1309,30 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 	if (_gvariant_is_fixed_size(signature)) {
 		int alignment = _gvariant_get_alignment(signature);
 		grow_body(builder, 0, alignment);
+
+		/* Empty struct or "unit type" is encoded one zero byte */
+		if (signature[0] == '\0')
+			grow_body(builder, 1, 1);
 	} else
 		container_append_struct_offsets(root, builder);
 
+	/*
+	 * Make sure there's enough space after the body for the variant
+	 * signature written here but not included in the body size and
+	 * one framing offset value to be written in
+	 * _gvariant_message_finalize.
+	 */
+	size = 3 + strlen(signature) + 8;
+	if (builder->body_pos + size > builder->body_size)
+		builder->body = l_realloc(builder->body,
+						builder->body_pos + size);
+
+	variant_buf = builder->body + builder->body_pos;
+	*variant_buf++ = 0;
+	*variant_buf++ = '(';
+	variant_buf = mempcpy(variant_buf, signature, strlen(signature));
+	*variant_buf++ = ')';
+
 	*body = builder->body;
 	*body_size = builder->body_pos;
 	builder->body = NULL;
@@ -1317,3 +1340,24 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 
 	return signature;
 }
+
+/*
+ * Write the header's framing offset after the body variant which is the
+ * last piece of data in the message after the header, the padding and
+ * the builder has written the message body.
+ */
+size_t _gvariant_message_finalize(size_t header_end,
+					void *body, size_t body_size,
+					const char *signature)
+{
+	size_t offset_start;
+	size_t offset_size;
+
+	offset_start = body_size + 3 + strlen(signature);
+
+	offset_size = offset_length(align_len(header_end, 8) + offset_start, 1);
+
+	write_word_le(body + offset_start, header_end, offset_size);
+
+	return align_len(header_end, 8) + offset_start + offset_size;
+}
-- 
2.5.0

[PATCH v3 4/9] 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 e07bd2f..791fc41 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 v3 5/9] gvariant: Fix empty struct encoding

[Andrew Zaborowski]

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

Empty structs are encoded as a single zero byte.
---
 ell/gvariant-util.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 791fc41..d5d6772 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -1010,6 +1010,10 @@ static bool leave_struct_dict_common(struct dbus_builder *builder,
 		int alignment = _gvariant_get_alignment(container->signature);
 		grow_body(builder, 0, alignment);
 
+		/* Empty struct or "unit type" is a encoded as a zero byte */
+		if (container->signature[0] == '\0')
+			grow_body(builder, 1, 1);
+
 		parent->variable_is_last = false;
 	} else {
 		size_t offset;
-- 
2.5.0

[PATCH v3 6/9] dbus: Fix the kdbus message encoding

[Andrew Zaborowski]

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

Keep sending the message header+padding as a separate
KDBUS_ITEM_PAYLOAD_OFF buffer like in DBus-1, systemd also seems to do
that.  But fix the structure of the message body.

The term footer is used in systemd, probably there's a better name.
---
 ell/dbus-kernel.c  | 19 +++++++++----------
 ell/dbus-message.c | 19 +++++++++++++++++++
 ell/dbus-private.h |  1 +
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index 7797153..b5a07a5 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -323,8 +323,8 @@ int _dbus_kernel_send(int fd, size_t bloom_size, uint8_t bloom_n_hash,
 	struct kdbus_item *item;
 	void *header;
 	size_t header_size;
-	void *body;
-	size_t body_size;
+	void *footer;
+	size_t footer_size;
 	int ret;
 	struct kdbus_cmd_send cmd;
 	L_AUTO_FREE_VAR(struct kdbus_msg *, kmsg);
@@ -427,14 +427,13 @@ 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);
-	}
+	footer = _dbus_message_get_footer(message, &footer_size);
+
+	item->size = KDBUS_ITEM_HEADER_SIZE + sizeof(struct kdbus_vec);
+	item->type = KDBUS_ITEM_PAYLOAD_VEC;
+	item->vec.address = (uintptr_t) footer;
+	item->vec.size = footer_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 9fdc0ff..00d9838 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;
 }
 
+/* Get a buffer containing the final message contents except the header */
+void *_dbus_message_get_footer(struct l_dbus_message *msg, size_t *out_size)
+{
+	size_t size;
+
+	if (_dbus_message_is_gvariant(msg)) {
+		size = _gvariant_message_finalize(msg->header_end,
+						msg->body, msg->body_size,
+						msg->signature);
+		size -= msg->header_size;
+	} else
+		size = msg->body_size;
+
+	if (out_size)
+		*out_size = size;
+
+	return msg->body;
+}
+
 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 6a4ea96..71767a4 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -115,6 +115,7 @@ bool _dbus1_builder_rewind(struct dbus_builder *builder);
 
 void *_dbus_message_get_body(struct l_dbus_message *msg, size_t *out_size);
 void *_dbus_message_get_header(struct l_dbus_message *msg, size_t *out_size);
+void *_dbus_message_get_footer(struct l_dbus_message *msg, size_t *out_size);
 void _dbus_message_set_serial(struct l_dbus_message *msg, uint32_t serial);
 uint32_t _dbus_message_get_serial(struct l_dbus_message *msg);
 uint32_t _dbus_message_get_reply_serial(struct l_dbus_message *message);
-- 
2.5.0

[PATCH v3 7/9] dbus: Remove signature field from gvariant header

[Andrew Zaborowski]

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

The main reason to do that is that systemd thinks it's an error and will
ignore received messages with a dbus1-like signature field in the
header.
---
 ell/dbus-message.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 00d9838..a409e53 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -972,7 +972,7 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 		message->sender = NULL;
 	}
 
-	if (signature[0] != '\0')
+	if (signature[0] != '\0' && !gvariant)
 		add_field(builder, driver, DBUS_MESSAGE_FIELD_SIGNATURE,
 				"g", signature);
 
@@ -1201,13 +1201,9 @@ static bool append_arguments(struct l_dbus_message *message,
 	if (strcmp(signature, generated_signature))
 		return false;
 
-	l_free(generated_signature);
-
 	build_header(message, signature);
 	message->sealed = true;
-
-	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
-						&message->signature);
+	message->signature = generated_signature;
 
 	return true;
 
@@ -1932,10 +1928,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, 'g',
-						&builder->message->signature);
-	l_free(generated_signature);
+	builder->message->signature = generated_signature;
 
 	return builder->message;
 }
-- 
2.5.0

[PATCH v3 8/9] unit: Update GVariant test messages

[Andrew Zaborowski]

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

---
 unit/test-gvariant-message.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/unit/test-gvariant-message.c b/unit/test-gvariant-message.c
index c2cb811..836ba29 100644
--- a/unit/test-gvariant-message.c
+++ b/unit/test-gvariant-message.c
@@ -50,22 +50,20 @@ 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,
 	0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 	0x66, 0x6f, 0x6f, 0x2e, 0x62, 0x61, 0x72, 0x00, 0x00, 0x73, 0x00, 0x00,
 	0x00, 0x00, 0x00, 0x00, 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-	0x66, 0x6f, 0x6f, 0x2e, 0x62, 0x61, 0x72, 0x00, 0x00, 0x73, 0x00, 0x00,
-	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-	0x62, 0x79, 0x6e, 0x71, 0x69, 0x75, 0x78, 0x74, 0x64, 0x00, 0x00, 0x67,
-	0x13, 0x29, 0x42, 0x5a, 0x74, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-	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,
+	0x66, 0x6f, 0x6f, 0x2e, 0x62, 0x61, 0x72, 0x00, 0x00, 0x73, 0x13, 0x29,
+	0x42, 0x5a, 0x00, 0x00, 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, 0x28, 0x62, 0x79,
+	0x6e, 0x71, 0x69, 0x75, 0x78, 0x74, 0x64, 0x29, 0x6e,
 };
 
 static const struct message_data message_data_basic_1 = {
@@ -76,12 +74,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,
@@ -90,16 +88,15 @@ static const unsigned char message_binary_complex_1[] = {
 	0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x69, 0x6e, 0x74, 0x65,
 	0x72, 0x66, 0x61, 0x63, 0x65, 0x00, 0x00, 0x73, 0x06, 0x00, 0x00, 0x00,
 	0x00, 0x00, 0x00, 0x00, 0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
-	0x70, 0x6c, 0x65, 0x00, 0x00, 0x73, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00,
-	0x00, 0x00, 0x00, 0x00, 0x6f, 0x61, 0x7b, 0x73, 0x76, 0x7d, 0x00, 0x00,
-	0x67, 0x1e, 0x31, 0x58, 0x6e, 0x81, 0x00, 0x00, 0x2f, 0x63, 0x6f, 0x6d,
-	0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2f, 0x6f, 0x62, 0x6a,
-	0x65, 0x63, 0x74, 0x00, 0x00, 0x00, 0x00, 0x00, 0x4e, 0x61, 0x6d, 0x65,
-	0x00, 0x00, 0x00, 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x73, 0x20, 0x54, 0x6f,
-	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,
+	0x70, 0x6c, 0x65, 0x00, 0x00, 0x73, 0x1e, 0x31, 0x58, 0x6e, 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, 0x00, 0x00, 0x00, 0x4e, 0x61, 0x6d, 0x65, 0x00, 0x00, 0x00, 0x00,
+	0x4c, 0x69, 0x6e, 0x75, 0x73, 0x20, 0x54, 0x6f, 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, 0x00,
+	0x28, 0x6f, 0x61, 0x7b, 0x73, 0x76, 0x7d, 0x29, 0x82,
 };
 
 static const struct message_data message_data_complex_1 = {
-- 
2.5.0

[PATCH v3 9/9] 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 71767a4..63c239b 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -161,7 +161,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 900b71b..4d79d11 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

Re: [PATCH v3 1/9] dbus: Update GVariant message header format

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> In the current GVariant message format since about Jan 2015 there's no
> body size field (it is reserved and must be sent as 0) or header size
> information or signature.  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 | 167 ++++++++++++++++++++++++++++++++++++++---------------
>   ell/dbus-private.h |  16 ++++-
>   ell/dbus-util.c    |   4 +-
>   ell/dbus.c         |   4 +-
>   5 files changed, 137 insertions(+), 57 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 2/9] dbus: Don't rely on 1st KDBUS_ITEM_PAYLOAD_OFF being the header

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> 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(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 9/9] dbus: Rename _dbus_header_is_valid to _dbus1_header_is_valid

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:34 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-private.h | 2 +-
>   ell/dbus-util.c    | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 7/9] dbus: Remove signature field from gvariant header

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:34 PM, Andrew Zaborowski wrote:
> The main reason to do that is that systemd thinks it's an error and will
> ignore received messages with a dbus1-like signature field in the
> header.
> ---
>   ell/dbus-message.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
> index 00d9838..a409e53 100644
> --- a/ell/dbus-message.c
> +++ b/ell/dbus-message.c
> @@ -972,7 +972,7 @@ static void build_header(struct l_dbus_message *message, const char *signature)
>   		message->sender = NULL;
>   	}
>
> -	if (signature[0] != '\0')
> +	if (signature[0] != '\0' && !gvariant)
>   		add_field(builder, driver, DBUS_MESSAGE_FIELD_SIGNATURE,
>   				"g", signature);
>
> @@ -1201,13 +1201,9 @@ static bool append_arguments(struct l_dbus_message *message,
>   	if (strcmp(signature, generated_signature))
>   		return false;
>
> -	l_free(generated_signature);
> -
>   	build_header(message, signature);
>   	message->sealed = true;
> -
> -	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
> -						&message->signature);
> +	message->signature = generated_signature;

Are you forgetting to set signature_free?

>
>   	return true;
>
> @@ -1932,10 +1928,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, 'g',
> -						&builder->message->signature);
> -	l_free(generated_signature);
> +	builder->message->signature = generated_signature;
>

Are you forgetting to set signature_free?

>   	return builder->message;
>   }
>

Regards,
-Denis

Re: [PATCH v3 8/9] unit: Update GVariant test messages

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:34 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-gvariant-message.c | 43 ++++++++++++++++++++-----------------------
>   1 file changed, 20 insertions(+), 23 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 3/9] gvariant: New gvariant message format support

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> Make the builder write a complete struct enclosed in a variant and add a
> utility to add the final framing offset to produce a single parseable
> GVariant blob.
> ---
>   ell/gvariant-private.h |  4 ++++
>   ell/gvariant-util.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 48 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 6/9] dbus: Fix the kdbus message encoding

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> Keep sending the message header+padding as a separate
> KDBUS_ITEM_PAYLOAD_OFF buffer like in DBus-1, systemd also seems to do
> that.  But fix the structure of the message body.
>
> The term footer is used in systemd, probably there's a better name.
> ---
>   ell/dbus-kernel.c  | 19 +++++++++----------
>   ell/dbus-message.c | 19 +++++++++++++++++++
>   ell/dbus-private.h |  1 +
>   3 files changed, 29 insertions(+), 10 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 5/9] gvariant: Fix empty struct encoding

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> Empty structs are encoded as a single zero byte.
> ---
>   ell/gvariant-util.c | 4 ++++
>   1 file changed, 4 insertions(+)
>

I went ahead and applied this one.  But can we get a unit test for this?

Regards,
-Denis

Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Denis Kenzior]

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

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> 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 e07bd2f..791fc41 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);
> +	}
>

So I have a nagging bad feeling about this one.  The code of this 
function was copied from _gvariant_valid_signature.  So changing this 
strongly implies changing _gvariant_valid_signature as well...

Passing in "" won't crash this function, so where do we crash?

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

So why would we be allocating a zero-sized array?  Do we need to special 
case this somehow?

Can I get some unit test data to play with?

>
>   	for (p = sig_start, i = 0; i < n_children; i++) {
>

Regards,
-Denis

Re: [PATCH v3 7/9] dbus: Remove signature field from gvariant header

[Andrzej Zaborowski]

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

Hi Denis,

On 18 April 2016 at 18:59, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/14/2016 07:34 PM, Andrew Zaborowski wrote:
>> -       l_free(generated_signature);
>> -
>>         build_header(message, signature);
>>         message->sealed = true;
>> -
>> -       get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
>> -                                               &message->signature);
>> +       message->signature = generated_signature;
>
>
> Are you forgetting to set signature_free?

Oops, yes, I'll resend the patch.

Best regards

Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Andrzej Zaborowski]

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

Hi Denis,

On 18 April 2016 at 21:39, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
>> 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 e07bd2f..791fc41 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);
>> +       }
>>
>
> So I have a nagging bad feeling about this one.  The code of this function
> was copied from _gvariant_valid_signature.  So changing this strongly
> implies changing _gvariant_valid_signature as well...

So _gvariant_valid_signature is used in two places in ell, one is
_gvariant_builder_enter_struct where we want to check for a sequence
of dbus types like in a message signature, so yes, it should accept an
empty string.  The other place is unit/test-gvariant-util.c where
we're trying to validate a single data type signature so the current
logic is ok.  I'll change the unit test to do "valid =
(_gvariant_num_children(test->signature) == 1)" instead, and add a
struct unit test...

>
> Passing in "" won't crash this function, so where do we crash?

The crash referred to the lack of error check.  This function returns
-1 and we assign it to an unsigned variable below, then use the value
to allocate memory and as a loop limit.

>
>>         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);
>
>
> So why would we be allocating a zero-sized array?  Do we need to special
> case this somehow?

The 0-sized malloc doesn't hurt and lets us avoid a few conditional
blocks, so probably it's ok?

>
> Can I get some unit test data to play with?

Ok, good idea.

Best regards

Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Andrzej Zaborowski]

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

Hi Denis,

On 22 April 2016 at 02:01, Andrzej Zaborowski
<andrew.zaborowski@intel.com> wrote:
> On 18 April 2016 at 21:39, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
>>> 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 e07bd2f..791fc41 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);
>>> +       }
>>>
>>
>> So I have a nagging bad feeling about this one.  The code of this function
>> was copied from _gvariant_valid_signature.  So changing this strongly
>> implies changing _gvariant_valid_signature as well...
>
> So _gvariant_valid_signature is used in two places in ell, one is
> _gvariant_builder_enter_struct where we want to check for a sequence
> of dbus types like in a message signature, so yes, it should accept an
> empty string.  The other place is unit/test-gvariant-util.c where
> we're trying to validate a single data type signature so the current
> logic is ok.  I'll change the unit test to do "valid =
> (_gvariant_num_children(test->signature) == 1)" instead, and add a
> struct unit test...

I was wrong here, apparently the unit test also tests for "message
signature" type of signatures.  Why are we then treating "" as
invalid?

Best regards

Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Denis Kenzior]

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

Andrew,

 >>> So I have a nagging bad feeling about this one.  The code of this 
function
>>> was copied from _gvariant_valid_signature.  So changing this strongly
>>> implies changing _gvariant_valid_signature as well...
>>
>> So _gvariant_valid_signature is used in two places in ell, one is
>> _gvariant_builder_enter_struct where we want to check for a sequence
>> of dbus types like in a message signature, so yes, it should accept an
>> empty string.  The other place is unit/test-gvariant-util.c where
>> we're trying to validate a single data type signature so the current
>> logic is ok.  I'll change the unit test to do "valid =
>> (_gvariant_num_children(test->signature) == 1)" instead, and add a
>> struct unit test...
>
> I was wrong here, apparently the unit test also tests for "message
> signature" type of signatures.  Why are we then treating "" as
> invalid?
>

In theory treating "" as invalid is intended behavior.  See how 
_dbus_valid_signature is used.  However, I don't think we ever tested 
empty structs, so feel free to change this.

Regards,
-Denis

Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Denis Kenzior]

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

Hi Andrew,

 >>
>> So I have a nagging bad feeling about this one.  The code of this function
>> was copied from _gvariant_valid_signature.  So changing this strongly
>> implies changing _gvariant_valid_signature as well...
>
> So _gvariant_valid_signature is used in two places in ell, one is
> _gvariant_builder_enter_struct where we want to check for a sequence
> of dbus types like in a message signature, so yes, it should accept an
> empty string.  The other place is unit/test-gvariant-util.c where
> we're trying to validate a single data type signature so the current
> logic is ok.  I'll change the unit test to do "valid =
> (_gvariant_num_children(test->signature) == 1)" instead, and add a
> struct unit test...
>

This reminds me.  DBus classic does not accept empty structures.  This 
is a kdbus invention.  Refer to DBus specification:
"Empty structures are not allowed; there must be at least one type code 
between the parentheses."

I have never been able to come up with a practical utility for empty 
structures for kdbus.  Nevertheless, the signature logic in 
gvariant-util is subtly different from dbus-util.

>>
>> So why would we be allocating a zero-sized array?  Do we need to special
>> case this somehow?
>
> The 0-sized malloc doesn't hurt and lets us avoid a few conditional
> blocks, so probably it's ok?
>

Generally I find that weird, but lets see how it looks.

Regards,
-Denis

Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Andrzej Zaborowski]

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

Hi Denis,

On 22 April 2016 at 04:32, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
>>>
>>>
>>> So I have a nagging bad feeling about this one.  The code of this
>>> function
>>> was copied from _gvariant_valid_signature.  So changing this strongly
>>> implies changing _gvariant_valid_signature as well...
>>
>>
>> So _gvariant_valid_signature is used in two places in ell, one is
>> _gvariant_builder_enter_struct where we want to check for a sequence
>> of dbus types like in a message signature, so yes, it should accept an
>> empty string.  The other place is unit/test-gvariant-util.c where
>> we're trying to validate a single data type signature so the current
>> logic is ok.  I'll change the unit test to do "valid =
>> (_gvariant_num_children(test->signature) == 1)" instead, and add a
>> struct unit test...
>>
>
> This reminds me.  DBus classic does not accept empty structures.  This is a
> kdbus invention.  Refer to DBus specification:
> "Empty structures are not allowed; there must be at least one type code
> between the parentheses."
>
> I have never been able to come up with a practical utility for empty
> structures for kdbus.  Nevertheless, the signature logic in gvariant-util is
> subtly different from dbus-util.

I see now.  It seems that other than the struct enclosing the gvariant
message body we will never see empty structs, but we need to be able
to parse and build them for this one use case and for completness.
I'll add explicit checks for empty signature before the
_gvariant_valid_signature and _gvariant_num_children calls used with
structs.

Best regards