[PATCH] dbus: Handle kdbus KDBUS_ITEM_REPLY_TIMEOUT / _DEAD

[PATCH] dbus: Handle kdbus KDBUS_ITEM_REPLY_TIMEOUT / _DEAD

[Andrew Zaborowski]

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

Generate DBus1-like error messages on kernel error notifications.
---
 ell/dbus-kernel.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index b5a07a5..4fd2f29 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -559,6 +559,7 @@ int _dbus_kernel_recv(int fd, void *kdbus_pool,
 	struct kdbus_item *item;
 	int r;
 	size_t min_size;
+	const char *error;
 
 	memset(&recv_cmd, 0, sizeof(recv_cmd));
 
@@ -601,6 +602,23 @@ int _dbus_kernel_recv(int fd, void *kdbus_pool,
 						user_data);
 			break;
 
+		case KDBUS_ITEM_REPLY_TIMEOUT:
+		case KDBUS_ITEM_REPLY_DEAD:
+			if (item->type == KDBUS_ITEM_REPLY_TIMEOUT)
+				error = "Did not receive a reply.";
+			else
+				error = "Message recipient disconnected from "
+					"message bus without replying.";
+
+			dbus_msg = _dbus_message_new_error(
+					2, msg->cookie_reply, NULL,
+					"org.freedesktop.DBus.Error.NoReply",
+					error);
+			if (dbus_msg)
+				message_func(dbus_msg, user_data);
+
+			break;
+
 		default:
 			break;
 		}
-- 
2.5.0

[PATCH 1/5] gvariant: _gvariant_num_children error check

[Andrew Zaborowski]

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

---
 ell/gvariant-util.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 88bb74e..17606f8 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -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 2/5] gvariant: Allow empty signature in enter_struct_dict_common

[Andrew Zaborowski]

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

gvariant_iter_init_internal now checks if the signature is empty before
calling _gvariant_num_children which would return an error in that case.
The gvariant_iter_init_internal caller is responsible for checking if
the signature is allowed in the specific context.
---
 ell/gvariant-util.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 17606f8..1f493e0 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -391,11 +391,17 @@ static bool gvariant_iter_init_internal(struct l_dbus_message_iter *iter,
 	iter->len = len;
 	iter->pos = 0;
 
-	n_children = _gvariant_num_children(subsig);
-	if (n_children < 0)
-		return false;
+	if (subsig[0] != '\0') {
+		n_children = _gvariant_num_children(subsig);
+		if (n_children < 0)
+			return false;
 
-	children = l_new(struct gvariant_type_info, n_children);
+		children = l_new(struct gvariant_type_info, n_children);
+	} else {
+		n_children = 0;
+
+		children = NULL;
+	}
 
 	for (p = sig_start, i = 0; i < n_children; i++) {
 		int alignment;
@@ -966,7 +972,7 @@ static bool enter_struct_dict_common(struct dbus_builder *builder,
 bool _gvariant_builder_enter_struct(struct dbus_builder *builder,
 					const char *signature)
 {
-	if (!_gvariant_valid_signature(signature))
+	if (signature[0] && !_gvariant_valid_signature(signature))
 		return false;
 
 	return enter_struct_dict_common(builder, signature,
-- 
2.5.0

[PATCH 3/5] gvariant: Make sure builder outputs a zero for empty struct

[Andrew Zaborowski]

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

Make sure the single byte being written by the builder is a zero.
I had assumed grow_body would zero the newly allocated body bytes but on
a closer look it zeros the alignment padding only.
---
 ell/gvariant-util.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 1f493e0..2172ad9 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -1016,9 +1016,12 @@ 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);
+		/* Empty struct or "unit type" is encoded as a zero byte */
+		if (container->signature[0] == '\0') {
+			size_t start = grow_body(builder, 1, 1);
+
+			memset(builder->body + start, 0, 1);
+		}
 
 		parent->variable_is_last = false;
 	} else {
@@ -1323,9 +1326,12 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 		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);
+		/* Empty struct or "unit type" is encoded as a zero byte */
+		if (signature[0] == '\0') {
+			size_t start = grow_body(builder, 1, 1);
+
+			memset(builder->body + start, 0, 1);
+		}
 	} else
 		container_append_struct_offsets(root, builder);
 
-- 
2.5.0

[PATCH 4/5] unit: Gvariant iterator and builder test for empty structs

[Andrew Zaborowski]

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

---
 unit/test-gvariant-util.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/unit/test-gvariant-util.c b/unit/test-gvariant-util.c
index 9714df0..331b1f7 100644
--- a/unit/test-gvariant-util.c
+++ b/unit/test-gvariant-util.c
@@ -553,6 +553,32 @@ static void test_iter_nested_struct_1(const void *test_data)
 	assert(i == -1);
 }
 
+static const unsigned char empty_struct_data_1[] = {
+	0x00
+};
+
+static struct parser_data empty_struct_1 = {
+	.data = empty_struct_data_1,
+	.len = sizeof(empty_struct_data_1),
+	.signature = "()",
+};
+
+static void test_iter_empty_struct_1(const void *test_data)
+{
+	const struct parser_data *test = test_data;
+	struct l_dbus_message_iter iter;
+	bool ret;
+	struct l_dbus_message_iter str;
+
+	_gvariant_iter_init(&iter, NULL, test->signature,
+				test->signature + strlen(test->signature),
+				test->data, test->len);
+
+	ret = _gvariant_iter_enter_struct(&iter, &str);
+	assert(ret);
+	assert(str.sig_len == 0);
+}
+
 static const unsigned char variant_data_1[] = {
 	0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x66, 0x6f, 0x6f, 0x62,
 	0x61, 0x72, 0x00, 0x00, 0x73, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00,
@@ -1360,6 +1386,25 @@ static void test_builder_nested_struct_1(const void *test_data)
 	FINISH_AND_CHECK_BUILT_RESULT();
 }
 
+static void test_builder_empty_struct_1(const void *test_data)
+{
+	const struct parser_data *test = test_data;
+	struct dbus_builder *builder;
+	bool ret;
+	BUILDER_TEST_HEADER();
+
+	builder = _gvariant_builder_new(NULL, 0);
+	assert(builder);
+
+	ret = _gvariant_builder_enter_struct(builder, "");
+	assert(ret);
+
+	ret = _gvariant_builder_leave_struct(builder);
+	assert(ret);
+
+	FINISH_AND_CHECK_BUILT_RESULT();
+}
+
 static void test_builder_variant_1(const void *test_data)
 {
 	const struct parser_data *test = test_data;
@@ -1840,6 +1885,9 @@ int main(int argc, char *argv[])
 	l_test_add("Iter Test Nested Struct '((us)yi)'",
 			test_iter_nested_struct_1, &nested_struct_1);
 
+	l_test_add("Iter Test Empty Struct '()'",
+			test_iter_empty_struct_1, &empty_struct_1);
+
 	l_test_add("Iter Test Variant '(uvu)i'", test_iter_variant_1,
 						&variant_1);
 	l_test_add("Iter Test Variant 'v'", test_iter_variant_2, &variant_2);
@@ -1876,6 +1924,9 @@ int main(int argc, char *argv[])
 	l_test_add("Builder Test Nested Struct '((us)yi)'",
 			test_builder_nested_struct_1, &nested_struct_1);
 
+	l_test_add("Builder Test Empty Struct '()'",
+			test_builder_empty_struct_1, &empty_struct_1);
+
 	l_test_add("Builder Test Variant '(uvu)i'", test_builder_variant_1,
 						&variant_1);
 	l_test_add("Builder Test Variant 'v'", test_builder_variant_2,
-- 
2.5.0

[PATCH 5/5] unit: Gvariant method call with no arguments tests

[Andrew Zaborowski]

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

---
 unit/test-gvariant-message.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/unit/test-gvariant-message.c b/unit/test-gvariant-message.c
index 836ba29..85326a6 100644
--- a/unit/test-gvariant-message.c
+++ b/unit/test-gvariant-message.c
@@ -110,6 +110,30 @@ static const struct message_data message_data_complex_1 = {
 	.binary_len	= sizeof(message_binary_complex_1),
 };
 
+static const unsigned char message_binary_empty_sig[] = {
+	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, 0x13, 0x29,
+	0x42, 0x5a, 0x00, 0x00, 0x00, 0x00, 0x28, 0x29, 0x6e,
+};
+
+static const struct message_data message_data_empty_sig = {
+	.type		= "method_call",
+	.path		= "/foo/bar",
+	.interface	= "foo.bar",
+	.member		= "Foobar",
+	.destination	= "foo.bar",
+	.signature	= "",
+	.binary		= message_binary_empty_sig,
+	.binary_len	= sizeof(message_binary_empty_sig),
+};
+
 static struct l_dbus_message *check_message(const struct message_data *msg_data)
 {
 	struct l_dbus_message *msg;
@@ -303,6 +327,28 @@ static void build_complex_1(const void *data)
 	compare_message(msg, data);
 }
 
+static void check_empty_sig(const void *data)
+{
+	struct l_dbus_message *msg = check_message(data);
+
+	assert(l_dbus_message_get_arguments(msg, ""));
+
+	l_dbus_message_unref(msg);
+}
+
+static void build_empty_sig(const void *data)
+{
+	struct l_dbus_message *msg = build_message(data);
+	bool result;
+
+	result = l_dbus_message_set_arguments(msg, "");
+	assert(result);
+
+	_dbus_message_set_serial(msg, 1111);
+
+	compare_message(msg, data);
+}
+
 static void builder_rewind(const void *data)
 {
 	struct l_dbus_message *msg = build_message(data);
@@ -364,6 +410,11 @@ int main(int argc, char *argv[])
 	l_test_add("Complex 1 (build)", build_complex_1,
 						&message_data_complex_1);
 
+	l_test_add("Empty signature (parse)", check_empty_sig,
+						&message_data_empty_sig);
+	l_test_add("Empty signature (build)", build_empty_sig,
+						&message_data_empty_sig);
+
 	l_test_add("Message Builder Rewind Complex 1", builder_rewind,
 						&message_data_complex_1);
 
-- 
2.5.0

Re: [PATCH] dbus: Handle kdbus KDBUS_ITEM_REPLY_TIMEOUT / _DEAD

[Denis Kenzior]

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

Hi Andrew,

On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
> Generate DBus1-like error messages on kernel error notifications.
> ---
>   ell/dbus-kernel.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 1/5] gvariant: _gvariant_num_children error check

[Denis Kenzior]

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

Hi Andrew,

On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
> ---
>   ell/gvariant-util.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] gvariant: Allow empty signature in enter_struct_dict_common

[Denis Kenzior]

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

Hi Andrew,

On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
> gvariant_iter_init_internal now checks if the signature is empty before
> calling _gvariant_num_children which would return an error in that case.
> The gvariant_iter_init_internal caller is responsible for checking if
> the signature is allowed in the specific context.
> ---
>   ell/gvariant-util.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>

I went ahead an applied this one. But one question, what is the 
practical utility of allowing iterators into the empty struct?

Regards,
-Denis

Re: [PATCH 3/5] gvariant: Make sure builder outputs a zero for empty struct

[Denis Kenzior]

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

Hi Andrew,

On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
> Make sure the single byte being written by the builder is a zero.
> I had assumed grow_body would zero the newly allocated body bytes but on
> a closer look it zeros the alignment padding only.
> ---
>   ell/gvariant-util.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 4/5] unit: Gvariant iterator and builder test for empty structs

[Denis Kenzior]

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

Hi Andrew,

On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-gvariant-util.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 5/5] unit: Gvariant method call with no arguments tests

[Denis Kenzior]

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

Hi Andrew,

On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-gvariant-message.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] gvariant: Allow empty signature in enter_struct_dict_common

[Andrzej Zaborowski]

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

Hi Denis,

On 26 April 2016 at 06:01, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 04/22/2016 08:10 PM, Andrew Zaborowski wrote:
>>
>> gvariant_iter_init_internal now checks if the signature is empty before
>> calling _gvariant_num_children which would return an error in that case.
>> The gvariant_iter_init_internal caller is responsible for checking if
>> the signature is allowed in the specific context.
>> ---
>>   ell/gvariant-util.c | 16 +++++++++++-----
>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>
>
> I went ahead an applied this one. But one question, what is the practical

Thanks

> utility of allowing iterators into the empty struct?

Well internally in ell we have a clear use case when we parse kdbus
messages, we use this to get the body start, body size and signature.
I also suspect the public function l_dbus_message_get_arguments makes
use of that when we're calling it with the empty signature to check
that the message signature is empty as expected.

Best regards