[PATCH 1/5] dbus: Handle the 'h' type in append_arguments

[PATCH 1/5] dbus: Handle the 'h' type in append_arguments

[Andrew Zaborowski]

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

FD values can't be handled same as int32 values ('i' and 'u').
---
 ell/dbus-message.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 6089295..038d44d 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -1140,7 +1140,6 @@ static bool append_arguments(struct l_dbus_message *message,
 		}
 		case 'i':
 		case 'u':
-		case 'h':
 		{
 			uint32_t u = va_arg(args, uint32_t);
 
@@ -1149,6 +1148,20 @@ static bool append_arguments(struct l_dbus_message *message,
 
 			break;
 		}
+		case 'h':
+		{
+			int fd = va_arg(args, int);
+
+			if (!driver->append_basic(builder, *s,
+							&message->num_fds))
+				goto error;
+
+			if (message->num_fds < L_ARRAY_SIZE(message->fds))
+				message->fds[message->num_fds++] =
+					fcntl(fd, F_DUPFD_CLOEXEC, 3);
+
+			break;
+		}
 		case 'x':
 		case 't':
 		{
-- 
2.5.0

[PATCH 2/5] dbus: Remove memcpy and fix setting of FD_CLOEXEC on FDs

[Andrew Zaborowski]

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

Make sure the size of the FD buffer passed to recvmsg is properly
aligned, etc. and remove the memcpy which seems unneeded.  Also it looks
like the fcntl() calls operated on one dbus socket FD instead of the
message FDs.
---
 ell/dbus.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/ell/dbus.c b/ell/dbus.c
index 59506c5..35378b2 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -628,7 +628,11 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 	ssize_t len;
 	void *header, *body;
 	size_t header_size, body_size;
-	int fds[16];
+	union {
+		uint8_t bytes[CMSG_SPACE(16 * sizeof(int))];
+		struct cmsghdr align;
+	} fd_buf;
+	int *fds = NULL;
 	uint32_t num_fds = 0;
 
 	len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
@@ -649,8 +653,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 	memset(&msg, 0, sizeof(msg));
 	msg.msg_iov = iov;
 	msg.msg_iovlen = 2;
-	msg.msg_control = &fds;
-	msg.msg_controllen = CMSG_SPACE(16 * sizeof(int));
+	msg.msg_control = &fd_buf;
+	msg.msg_controllen = sizeof(fd_buf);
 
 	len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
 	if (len < 0)
@@ -665,19 +669,18 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 			continue;
 
 		num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
-
-		memcpy(fds, CMSG_DATA(cmsg), num_fds * sizeof(int));
+		fds = (void *) CMSG_DATA(cmsg);
 
 		/* Set FD_CLOEXEC on all file descriptors */
 		for (i = 0; i < num_fds; i++) {
 			long flags;
 
-			flags = fcntl(fd, F_GETFD, NULL);
+			flags = fcntl(fds[i], F_GETFD, NULL);
 			if (flags < 0)
 				continue;
 
 			if (!(flags & FD_CLOEXEC))
-				fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+				fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
                 }
 	}
 
-- 
2.5.0

[PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Andrew Zaborowski]

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

---
 ell/dbus-message.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 038d44d..0fe76de 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -764,11 +764,22 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size,
 		get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
 					'g', &message->signature);
 
-	if (num_fds > L_ARRAY_SIZE(message->fds)) {
-		for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
-			close(fds[i]);
+	if (num_fds) {
+		uint32_t unix_fds;
 
-		num_fds = L_ARRAY_SIZE(message->fds);
+		if (!get_header_field(message, DBUS_MESSAGE_FIELD_UNIX_FDS,
+					'u', &unix_fds))
+			goto free;
+
+		if (num_fds > unix_fds)
+			num_fds = unix_fds;
+
+		if (num_fds > L_ARRAY_SIZE(message->fds)) {
+			for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
+				close(fds[i]);
+
+			num_fds = L_ARRAY_SIZE(message->fds);
+		}
 	}
 
 	message->num_fds = num_fds;
@@ -777,7 +788,8 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size,
 	return message;
 
 free:
-	l_free(message);
+	l_dbus_message_unref(message);
+
 	return NULL;
 }
 
@@ -809,19 +821,31 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
 	message->header = header;
 	message->body_size = body_size;
 	message->body = body;
+	message->sealed = true;
 
-	if (num_fds > L_ARRAY_SIZE(message->fds)) {
-		for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
-			close(fds[i]);
+	if (num_fds) {
+		uint32_t unix_fds;
 
-		num_fds = L_ARRAY_SIZE(message->fds);
+		if (!get_header_field(message, DBUS_MESSAGE_FIELD_UNIX_FDS,
+					'u', &unix_fds)) {
+			l_free(message);
+			return NULL;
+		}
+
+		if (num_fds > unix_fds)
+			num_fds = unix_fds;
+
+		if (num_fds > L_ARRAY_SIZE(message->fds)) {
+			for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
+				close(fds[i]);
+
+			num_fds = L_ARRAY_SIZE(message->fds);
+		}
 	}
 
 	message->num_fds = num_fds;
 	memcpy(message->fds, fds, num_fds * sizeof(int));
 
-	message->sealed = true;
-
 	/* If the field is absent message->signature will remain NULL */
 	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
 						&message->signature);
@@ -1013,6 +1037,10 @@ static void build_header(struct l_dbus_message *message, const char *signature)
 		add_field(builder, driver, DBUS_MESSAGE_FIELD_SIGNATURE,
 				"g", signature);
 
+	if (message->num_fds)
+		add_field(builder, driver, DBUS_MESSAGE_FIELD_UNIX_FDS,
+					"u", &message->num_fds);
+
 	driver->leave_array(builder);
 
 	generated_signature = driver->finish(builder, &message->header,
-- 
2.5.0

[PATCH 4/5] unit: Add UNIX_FDS header fields in FD test messages

[Andrew Zaborowski]

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

---
 unit/test-dbus-message.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/unit/test-dbus-message.c b/unit/test-dbus-message.c
index 52df9d8..0c14a7b 100644
--- a/unit/test-dbus-message.c
+++ b/unit/test-dbus-message.c
@@ -2608,21 +2608,22 @@ static void compare_files(int a, int b)
 
 static const unsigned char message_binary_basic_h[] = {
 	0x6c, 0x01, 0x00, 0x01, 0x04, 0x00, 0x00, 0x00,
-	0x00, 0x00, 0x00, 0x00, 0x6f, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x78, 0x00, 0x00, 0x00,
 	0x01, 0x01, 0x6f, 0x00, 0x13, 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,
-	0x06, 0x01, 0x73, 0x00, 0x0b, 0x00, 0x00, 0x00,
-	0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
-	0x70, 0x6c, 0x65, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x03, 0x01, 0x73, 0x00, 0x06, 0x00, 0x00, 0x00,
+	0x6d, 0x65, 0x74, 0x68, 0x6f, 0x64, 0x00, 0x00,
 	0x02, 0x01, 0x73, 0x00, 0x15, 0x00, 0x00, 0x00,
 	0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
 	0x70, 0x6c, 0x65, 0x2e, 0x69, 0x6e, 0x74, 0x65,
 	0x72, 0x66, 0x61, 0x63, 0x65, 0x00, 0x00, 0x00,
-	0x03, 0x01, 0x73, 0x00, 0x06, 0x00, 0x00, 0x00,
-	0x6d, 0x65, 0x74, 0x68, 0x6f, 0x64, 0x00, 0x00,
+	0x06, 0x01, 0x73, 0x00, 0x0b, 0x00, 0x00, 0x00,
+	0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
+	0x70, 0x6c, 0x65, 0x00, 0x00, 0x00, 0x00, 0x00,
 	0x08, 0x01, 0x67, 0x00, 0x01, 0x68, 0x00, 0x00,
+	0x09, 0x01, 0x75, 0x00, 0x01, 0x00, 0x00, 0x00,
 	0x00, 0x00, 0x00, 0x00,
 };
 
-- 
2.5.0

[PATCH 5/5] unit: End to end FD passing test

[Andrew Zaborowski]

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

Doesn't really belong in test-dbus-properties.c...
---
 unit/test-dbus-properties.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/unit/test-dbus-properties.c b/unit/test-dbus-properties.c
index 3238cf6..c5b2f6e 100644
--- a/unit/test-dbus-properties.c
+++ b/unit/test-dbus-properties.c
@@ -28,6 +28,9 @@
 #include <stdlib.h>
 #include <sys/wait.h>
 #include <stdio.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
 
 #include <ell/ell.h>
 #include <ell/dbus-private.h>
@@ -277,6 +280,22 @@ static bool test_path_getter(struct l_dbus *dbus,
 	return l_dbus_message_builder_append_basic(builder, 'o', "/foo/bar");
 }
 
+static struct l_dbus_message *get_random_callback(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					void *user_data)
+{
+	struct l_dbus_message *reply;
+	int fd;
+
+	reply = l_dbus_message_new_method_return(message);
+
+	fd = open("/dev/random", O_RDONLY);
+	l_dbus_message_set_arguments(reply, "h", fd);
+	close(fd);
+
+	return reply;
+}
+
 static void setup_test_interface(struct l_dbus_interface *interface)
 {
 	l_dbus_interface_property(interface, "String", 0, "s",
@@ -289,6 +308,9 @@ static void setup_test_interface(struct l_dbus_interface *interface)
 					test_string_getter, test_error_setter);
 	l_dbus_interface_property(interface, "Path", 0, "o",
 					test_path_getter, NULL);
+
+	l_dbus_interface_method(interface, "GetRandom", 0, get_random_callback,
+				"h", "", "randomfd");
 }
 
 static void validate_properties(struct l_dbus_message_iter *dict)
@@ -916,6 +938,75 @@ static void test_object_manager_signals(struct l_dbus *dbus, void *test_data)
 						NULL));
 }
 
+static int count_fds(void)
+{
+	int fd;
+	int count = 0;
+
+	for (fd = 0; fd < FD_SETSIZE; fd++)
+		if (fcntl(fd, F_GETFL) != -1 || errno != EBADF)
+			count++;
+
+	return count;
+}
+
+static bool compare_failed;
+
+static void compare_files(int a, int b)
+{
+	struct stat sa, sb;
+
+	compare_failed = true;
+
+	test_assert(fstat(a, &sa) == 0);
+	test_assert(fstat(b, &sb) == 0);
+
+	test_assert(sa.st_dev == sb.st_dev);
+	test_assert(sa.st_ino == sb.st_ino);
+	test_assert(sa.st_rdev == sb.st_rdev);
+
+	compare_failed = false;
+}
+
+static int open_fds;
+
+static void get_random_idle_callback(void *user_data)
+{
+	test_assert(count_fds() == open_fds);
+
+	test_next();
+}
+
+static void get_random_return_callback(struct l_dbus_message *message,
+					void *user_data)
+{
+	int fd0, fd1;
+
+	test_assert(!l_dbus_message_get_error(message, NULL, NULL));
+
+	test_assert(l_dbus_message_get_arguments(message, "h", &fd1));
+
+	fd0 = open("/dev/random", O_RDONLY);
+	test_assert(fd0 != -1);
+
+	compare_files(fd0, fd1);
+	if (compare_failed)
+		return;
+
+	close(fd0);
+	close(fd1);
+
+	test_assert(l_idle_oneshot(get_random_idle_callback, NULL, NULL));
+}
+
+static void test_fd_passing(struct l_dbus *dbus, void *test_data)
+{
+	open_fds = count_fds();
+
+	l_dbus_method_call(dbus, "org.test", "/test", "org.test", "GetRandom",
+				NULL, get_random_return_callback, NULL, NULL);
+}
+
 int main(int argc, char *argv[])
 {
 	struct l_signal *signal;
@@ -1003,6 +1094,8 @@ int main(int argc, char *argv[])
 	test_add("org.freedesktop.DBus.ObjectManager signals",
 			test_object_manager_signals, NULL);
 
+	test_add("FD passing", test_fd_passing, NULL);
+
 	l_main_run();
 
 	l_queue_destroy(tests, l_free);
-- 
2.5.0

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Denis Kenzior]

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

Hi Andrew,

On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-message.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
> index 038d44d..0fe76de 100644
> --- a/ell/dbus-message.c
> +++ b/ell/dbus-message.c
> @@ -764,11 +764,22 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size,
>   		get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
>   					'g', &message->signature);
>
> -	if (num_fds > L_ARRAY_SIZE(message->fds)) {
> -		for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
> -			close(fds[i]);
> +	if (num_fds) {
> +		uint32_t unix_fds;
>
> -		num_fds = L_ARRAY_SIZE(message->fds);
> +		if (!get_header_field(message, DBUS_MESSAGE_FIELD_UNIX_FDS,
> +					'u', &unix_fds))
> +			goto free;
> +
> +		if (num_fds > unix_fds)
> +			num_fds = unix_fds;

If num_fds > unix_fds, should all unused fds (e.g. fds[unix_fds .. 
num_fds-1] be closed just in case as well?

Also, what if unix_fds > num_fds?

> +
> +		if (num_fds > L_ARRAY_SIZE(message->fds)) {
> +			for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
> +				close(fds[i]);
> +
> +			num_fds = L_ARRAY_SIZE(message->fds);
> +		}
>   	}
>
>   	message->num_fds = num_fds;
> @@ -777,7 +788,8 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size,
>   	return message;
>
>   free:
> -	l_free(message);
> +	l_dbus_message_unref(message);
> +
>   	return NULL;
>   }
>
> @@ -809,19 +821,31 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
>   	message->header = header;
>   	message->body_size = body_size;
>   	message->body = body;
> +	message->sealed = true;
>
> -	if (num_fds > L_ARRAY_SIZE(message->fds)) {
> -		for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
> -			close(fds[i]);
> +	if (num_fds) {
> +		uint32_t unix_fds;
>
> -		num_fds = L_ARRAY_SIZE(message->fds);
> +		if (!get_header_field(message, DBUS_MESSAGE_FIELD_UNIX_FDS,
> +					'u', &unix_fds)) {
> +			l_free(message);
> +			return NULL;
> +		}
> +
> +		if (num_fds > unix_fds)
> +			num_fds = unix_fds;
> +
> +		if (num_fds > L_ARRAY_SIZE(message->fds)) {
> +			for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
> +				close(fds[i]);
> +
> +			num_fds = L_ARRAY_SIZE(message->fds);
> +		}
>   	}
>
>   	message->num_fds = num_fds;
>   	memcpy(message->fds, fds, num_fds * sizeof(int));
>
> -	message->sealed = true;
> -
>   	/* If the field is absent message->signature will remain NULL */
>   	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
>   						&message->signature);
> @@ -1013,6 +1037,10 @@ static void build_header(struct l_dbus_message *message, const char *signature)
>   		add_field(builder, driver, DBUS_MESSAGE_FIELD_SIGNATURE,
>   				"g", signature);
>
> +	if (message->num_fds)
> +		add_field(builder, driver, DBUS_MESSAGE_FIELD_UNIX_FDS,
> +					"u", &message->num_fds);
> +
>   	driver->leave_array(builder);
>
>   	generated_signature = driver->finish(builder, &message->header,
>

Regards,
-Denis

Re: [PATCH 1/5] dbus: Handle the 'h' type in append_arguments

[Denis Kenzior]

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

Hi Andrew,

On 04/29/2016 05:43 PM, Andrew Zaborowski wrote:
> FD values can't be handled same as int32 values ('i' and 'u').
> ---
>   ell/dbus-message.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] dbus: Remove memcpy and fix setting of FD_CLOEXEC on FDs

[Denis Kenzior]

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

Hi Andrew,

On 04/29/2016 05:43 PM, Andrew Zaborowski wrote:
> Make sure the size of the FD buffer passed to recvmsg is properly
> aligned, etc. and remove the memcpy which seems unneeded.  Also it looks
> like the fcntl() calls operated on one dbus socket FD instead of the
> message FDs.
> ---
>   ell/dbus.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 4/5] unit: Add UNIX_FDS header fields in FD test messages

[Denis Kenzior]

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

Hi Andrew,

On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
> ---
>   unit/test-dbus-message.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 5/5] unit: End to end FD passing test

[Denis Kenzior]

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

On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
> Doesn't really belong in test-dbus-properties.c...

This doesn't sound good ;)

> ---
>   unit/test-dbus-properties.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 93 insertions(+)
>

Can we make a dedicated .c test file for this, and test both 
dbus-classic and kdbus transports there?

Regards,
-Denis

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Andrzej Zaborowski]

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

On 30 April 2016 at 01:17, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
>> @@ -764,11 +764,22 @@ struct l_dbus_message *dbus_message_from_blob(const
>> void *data, size_t size,
>>                 get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
>>                                         'g', &message->signature);
>>
>> -       if (num_fds > L_ARRAY_SIZE(message->fds)) {
>> -               for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
>> -                       close(fds[i]);
>> +       if (num_fds) {
>> +               uint32_t unix_fds;
>>
>> -               num_fds = L_ARRAY_SIZE(message->fds);
>> +               if (!get_header_field(message,
>> DBUS_MESSAGE_FIELD_UNIX_FDS,
>> +                                       'u', &unix_fds))
>> +                       goto free;
>> +
>> +               if (num_fds > unix_fds)
>> +                       num_fds = unix_fds;
>
>
> If num_fds > unix_fds, should all unused fds (e.g. fds[unix_fds ..
> num_fds-1] be closed just in case as well?

Ok, let's do that.

>
> Also, what if unix_fds > num_fds?

Well, not sure if there's anything we can do.  As far as I've tried to
understand the sendmsg/recvmsg semantics the message boundaries are
not preserved and I understand FDs may also be delivered in a recvmsg
call different than that in which the message is received when there
are multiple messages in the queue.  The solution would be to keep
buffers of header/body data another buffer with FDs received and pair
them based on the UNIX_FDS headers but perhaps we don't want to
overcomplicate it.  For simplicity I take the minimum of num_fds,
unix_fds and L_ARRAY_SIZE(message->fds).

Best regards

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Denis Kenzior]

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

Hi Andrew,

On 04/29/2016 06:52 PM, Andrzej Zaborowski wrote:
> On 30 April 2016 at 01:17, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
>>> @@ -764,11 +764,22 @@ struct l_dbus_message *dbus_message_from_blob(const
>>> void *data, size_t size,
>>>                  get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
>>>                                          'g', &message->signature);
>>>
>>> -       if (num_fds > L_ARRAY_SIZE(message->fds)) {
>>> -               for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
>>> -                       close(fds[i]);
>>> +       if (num_fds) {
>>> +               uint32_t unix_fds;
>>>
>>> -               num_fds = L_ARRAY_SIZE(message->fds);
>>> +               if (!get_header_field(message,
>>> DBUS_MESSAGE_FIELD_UNIX_FDS,
>>> +                                       'u', &unix_fds))
>>> +                       goto free;
>>> +
>>> +               if (num_fds > unix_fds)
>>> +                       num_fds = unix_fds;
>>
>>
>> If num_fds > unix_fds, should all unused fds (e.g. fds[unix_fds ..
>> num_fds-1] be closed just in case as well?
>
> Ok, let's do that.
>
>>
>> Also, what if unix_fds > num_fds?
>
> Well, not sure if there's anything we can do.  As far as I've tried to
> understand the sendmsg/recvmsg semantics the message boundaries are
> not preserved and I understand FDs may also be delivered in a recvmsg
> call different than that in which the message is received when there
> are multiple messages in the queue.  The solution would be to keep
> buffers of header/body data another buffer with FDs received and pair
> them based on the UNIX_FDS headers but perhaps we don't want to
> overcomplicate it.  For simplicity I take the minimum of num_fds,
> unix_fds and L_ARRAY_SIZE(message->fds).
>

Isn't this a transport detail?  This function in particular is taking 
the full dbus-message blob.  Same with dbus_message_build().

We probably do have a separate problem inside classic_recv_message(). 
Since the UNIX socket is a stream, the message boundaries are not 
preserved, as you point out.

One way might be to use MSG_WAITALL flag.  Alternatively, allocate a 
buffer and keep reading into it until enough data is read.

This reminds me, since we utilize MSG_CMSG_CLOEXEC flag in recvmsg.  Do 
we still need a separate loop to set the O_CLOEXEC flag?  I suppose its 
safe to keep it just in case we're running on Linux kernel less than 2.6.23.

Is a similar flag implied over kdbus?

> Best regards
>

Regards,
-Denis

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Andrzej Zaborowski]

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

On 30 April 2016 at 03:15, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/29/2016 06:52 PM, Andrzej Zaborowski wrote:
>> On 30 April 2016 at 01:17, Denis Kenzior <denkenz@gmail.com> wrote:
>>>
>>> On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
>>>>
>>>> @@ -764,11 +764,22 @@ struct l_dbus_message
>>>> *dbus_message_from_blob(const
>>>> void *data, size_t size,
>>>>                  get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
>>>>                                          'g', &message->signature);
>>>>
>>>> -       if (num_fds > L_ARRAY_SIZE(message->fds)) {
>>>> -               for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
>>>> -                       close(fds[i]);
>>>> +       if (num_fds) {
>>>> +               uint32_t unix_fds;
>>>>
>>>> -               num_fds = L_ARRAY_SIZE(message->fds);
>>>> +               if (!get_header_field(message,
>>>> DBUS_MESSAGE_FIELD_UNIX_FDS,
>>>> +                                       'u', &unix_fds))
>>>> +                       goto free;
>>>> +
>>>> +               if (num_fds > unix_fds)
>>>> +                       num_fds = unix_fds;
>>>
>>>
>>>
>>> If num_fds > unix_fds, should all unused fds (e.g. fds[unix_fds ..
>>> num_fds-1] be closed just in case as well?
>>
>>
>> Ok, let's do that.
>>
>>>
>>> Also, what if unix_fds > num_fds?
>>
>>
>> Well, not sure if there's anything we can do.  As far as I've tried to
>> understand the sendmsg/recvmsg semantics the message boundaries are
>> not preserved and I understand FDs may also be delivered in a recvmsg
>> call different than that in which the message is received when there
>> are multiple messages in the queue.  The solution would be to keep
>> buffers of header/body data another buffer with FDs received and pair
>> them based on the UNIX_FDS headers but perhaps we don't want to
>> overcomplicate it.  For simplicity I take the minimum of num_fds,
>> unix_fds and L_ARRAY_SIZE(message->fds).
>>
>
> Isn't this a transport detail?  This function in particular is taking the
> full dbus-message blob.  Same with dbus_message_build().

Yeah, we should possibly handle that in dbus.c but we'd need to use
get_header_field etc., and it would get complicated.

>
> We probably do have a separate problem inside classic_recv_message(). Since
> the UNIX socket is a stream, the message boundaries are not preserved, as
> you point out.
>
> One way might be to use MSG_WAITALL flag.  Alternatively, allocate a buffer
> and keep reading into it until enough data is read.

Do you mean in case the read is interrupted by a signal?  I think
that's easy to handle but I'm not sure it can actually happen if the
dbus daemon always writes complete messages.

>
> This reminds me, since we utilize MSG_CMSG_CLOEXEC flag in recvmsg.  Do we
> still need a separate loop to set the O_CLOEXEC flag?  I suppose its safe to
> keep it just in case we're running on Linux kernel less than 2.6.23.
>
> Is a similar flag implied over kdbus?

Yeah, I'd also noticed we probably didn't need this loop.  But I don't
think kdbus supports this, there's nothing in the kernel code to set
this flag.

Best regards

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Denis Kenzior]

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

Hi Andrew,

On 04/30/2016 09:58 AM, Andrzej Zaborowski wrote:
> On 30 April 2016 at 03:15, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 04/29/2016 06:52 PM, Andrzej Zaborowski wrote:
>>> On 30 April 2016 at 01:17, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>
>>>> On 04/29/2016 05:44 PM, Andrew Zaborowski wrote:
>>>>>
>>>>> @@ -764,11 +764,22 @@ struct l_dbus_message
>>>>> *dbus_message_from_blob(const
>>>>> void *data, size_t size,
>>>>>                   get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
>>>>>                                           'g', &message->signature);
>>>>>
>>>>> -       if (num_fds > L_ARRAY_SIZE(message->fds)) {
>>>>> -               for (i = L_ARRAY_SIZE(message->fds); i < num_fds; i++)
>>>>> -                       close(fds[i]);
>>>>> +       if (num_fds) {
>>>>> +               uint32_t unix_fds;
>>>>>
>>>>> -               num_fds = L_ARRAY_SIZE(message->fds);
>>>>> +               if (!get_header_field(message,
>>>>> DBUS_MESSAGE_FIELD_UNIX_FDS,
>>>>> +                                       'u', &unix_fds))
>>>>> +                       goto free;
>>>>> +
>>>>> +               if (num_fds > unix_fds)
>>>>> +                       num_fds = unix_fds;
>>>>
>>>>
>>>>
>>>> If num_fds > unix_fds, should all unused fds (e.g. fds[unix_fds ..
>>>> num_fds-1] be closed just in case as well?
>>>
>>>
>>> Ok, let's do that.
>>>
>>>>
>>>> Also, what if unix_fds > num_fds?
>>>
>>>
>>> Well, not sure if there's anything we can do.  As far as I've tried to
>>> understand the sendmsg/recvmsg semantics the message boundaries are
>>> not preserved and I understand FDs may also be delivered in a recvmsg
>>> call different than that in which the message is received when there
>>> are multiple messages in the queue.  The solution would be to keep
>>> buffers of header/body data another buffer with FDs received and pair
>>> them based on the UNIX_FDS headers but perhaps we don't want to
>>> overcomplicate it.  For simplicity I take the minimum of num_fds,
>>> unix_fds and L_ARRAY_SIZE(message->fds).
>>>
>>
>> Isn't this a transport detail?  This function in particular is taking the
>> full dbus-message blob.  Same with dbus_message_build().
>
> Yeah, we should possibly handle that in dbus.c but we'd need to use
> get_header_field etc., and it would get complicated.
>

I'm not following.  So we have dbus_message_from_blob, which takes 
headers, body and the fds array.  If the number of fds specified in the 
header (e.g. unix_fds) doesn't match what the fds[] size is, then we 
should take some action.  This is separate from the transport details 
inside dbus.c.

>>
>> We probably do have a separate problem inside classic_recv_message(). Since
>> the UNIX socket is a stream, the message boundaries are not preserved, as
>> you point out.
>>
>> One way might be to use MSG_WAITALL flag.  Alternatively, allocate a buffer
>> and keep reading into it until enough data is read.
>
> Do you mean in case the read is interrupted by a signal?  I think
> that's easy to handle but I'm not sure it can actually happen if the
> dbus daemon always writes complete messages.

Regardless of whether dbus-daemon writes complete messages, the kernel 
is free to return however many bytes to the process calling recvfrom, 
even if more bytes are actually available in the receive buffer. 
MSG_WAITALL tells the kernel to give us all of the data the recvfrom 
caller is requesting, unless interrupted by a signal.

Without MSG_WAITALL we might start seeing partial reads on a heavily 
loaded system and things will start breaking.

The best way would be to peek at the next header, allocate space for the 
message and keep calling recvfrom until all of the message has been 
read.  Normally this would take 1 recvfrom call, but can take as many as 
needed.  Only once the entire message has been read, we can send it in 
for processing by dbus_message_from_blob or dbus_message_build.  We can 
put this more-complicated scheme on the TODO list for now...

>
>>
>> This reminds me, since we utilize MSG_CMSG_CLOEXEC flag in recvmsg.  Do we
>> still need a separate loop to set the O_CLOEXEC flag?  I suppose its safe to
>> keep it just in case we're running on Linux kernel less than 2.6.23.
>>
>> Is a similar flag implied over kdbus?
>
> Yeah, I'd also noticed we probably didn't need this loop.  But I don't
> think kdbus supports this, there's nothing in the kernel code to set
> this flag.
>

Okay, lets leave it.

Regards,
-Denis

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Andrzej Zaborowski]

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

Hi Denis,

On 2 May 2016 at 17:10, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/30/2016 09:58 AM, Andrzej Zaborowski wrote:
>> On 30 April 2016 at 03:15, Denis Kenzior <denkenz@gmail.com> wrote:
>>> On 04/29/2016 06:52 PM, Andrzej Zaborowski wrote:
>>>> On 30 April 2016 at 01:17, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>> Also, what if unix_fds > num_fds?
>>>>
>>>> Well, not sure if there's anything we can do.  As far as I've tried to
>>>> understand the sendmsg/recvmsg semantics the message boundaries are
>>>> not preserved and I understand FDs may also be delivered in a recvmsg
>>>> call different than that in which the message is received when there
>>>> are multiple messages in the queue.  The solution would be to keep
>>>> buffers of header/body data another buffer with FDs received and pair
>>>> them based on the UNIX_FDS headers but perhaps we don't want to
>>>> overcomplicate it.  For simplicity I take the minimum of num_fds,
>>>> unix_fds and L_ARRAY_SIZE(message->fds).
>>>
>>> Isn't this a transport detail?  This function in particular is taking the
>>> full dbus-message blob.  Same with dbus_message_build().
>>
>> Yeah, we should possibly handle that in dbus.c but we'd need to use
>> get_header_field etc., and it would get complicated.
>>
>
> I'm not following.  So we have dbus_message_from_blob, which takes headers,
> body and the fds array.  If the number of fds specified in the header (e.g.
> unix_fds) doesn't match what the fds[] size is, then we should take some
> action.  This is separate from the transport details inside dbus.c.

Elsewhere in the code we treat messages with too few / missing FDs as
valid and I think I commented on this in one of the commit messages,
another thing to consider is that we currently hardcode an FD limit of
16 per message and will happily generate such messages ourselves.  So
the code can handle the situation when unix_fds > num_fds and there's
nothing specific we have to do unless we decide to make it an error.

If we want to be really smart about queuing up FDs in a circular
buffer to assign to messages by unix_fds value then we would probably
want a separate function that would take a header and just return the
unix_fds value.

>
>>>
>>> We probably do have a separate problem inside classic_recv_message().
>>> Since
>>> the UNIX socket is a stream, the message boundaries are not preserved, as
>>> you point out.
>>>
>>> One way might be to use MSG_WAITALL flag.  Alternatively, allocate a
>>> buffer
>>> and keep reading into it until enough data is read.
>>
>>
>> Do you mean in case the read is interrupted by a signal?  I think
>> that's easy to handle but I'm not sure it can actually happen if the
>> dbus daemon always writes complete messages.
>
>
> Regardless of whether dbus-daemon writes complete messages, the kernel is
> free to return however many bytes to the process calling recvfrom, even if
> more bytes are actually available in the receive buffer. MSG_WAITALL tells
> the kernel to give us all of the data the recvfrom caller is requesting,
> unless interrupted by a signal.
>
> Without MSG_WAITALL we might start seeing partial reads on a heavily loaded
> system and things will start breaking.

That's the theory, i think it depends on the transport type if this
happens in practice (assuming no signal and the producer writes full
messages to the transport fd).

>
> The best way would be to peek at the next header, allocate space for the
> message and keep calling recvfrom until all of the message has been read.
> Normally this would take 1 recvfrom call, but can take as many as needed.
> Only once the entire message has been read, we can send it in for processing
> by dbus_message_from_blob or dbus_message_build.  We can put this
> more-complicated scheme on the TODO list for now...

Well, I can do the code directly, I think it's going to be a small
change.  We'll need it for sendmsg too.

Best regards

Re: [PATCH 3/5] dbus: Add and validate the UNIX_FDS msg header field

[Denis Kenzior]

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

Hi Andrew,

 > Elsewhere in the code we treat messages with too few / missing FDs as
> valid and I think I commented on this in one of the commit messages,

git show 439e259835d8b2788ca28c1dbe06badbd178dccf

This one only deals with num_fds > L_ARRAY_SIZE(message->fds), which 
makes sense.

git show 1e1aa1aa63be7b50a4d35aa01d14c001bf591fd1

Also sort of makes sense...

> another thing to consider is that we currently hardcode an FD limit of
> 16 per message and will happily generate such messages ourselves.  So
> the code can handle the situation when unix_fds > num_fds and there's
> nothing specific we have to do unless we decide to make it an error.

Which I think we should by the way.  If the message tells us we have 3 
fds, and we only receive 1 or 0, then that should be fatal.  It would 
most likely indicate an error in our transport code logic.  I'd actually 
like to make this verbose and see if this ever happens.  AFAIK, 
dbus-daemon does not allow this.

Since kdbus uses message_from_blob and dbus-1 uses message_build, do you 
want to have different constraints between those two?

>
> If we want to be really smart about queuing up FDs in a circular
> buffer to assign to messages by unix_fds value then we would probably
> want a separate function that would take a header and just return the
> unix_fds value.

I was hoping we could avoid that via use of MSG_WAITALL.  I suspect 
you're right and we need to.

Regards,
-Denis