--- Makefile.am | 3 + unit/test-dbus-message-fds.c | 368 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 371 insertions(+) create mode 100644 unit/test-dbus-message-fds.c diff --git a/Makefile.am b/Makefile.am index 3e8e3ef..aa99bdd 100644 --- a/Makefile.am +++ b/Makefile.am @@ -140,6 +140,7 @@ dbus_tests = unit/test-kdbus \ unit/test-dbus \ unit/test-dbus-util \ unit/test-dbus-message \ + unit/test-dbus-message-fds \ unit/test-dbus-service \ unit/test-dbus-watch \ unit/test-dbus-properties \ @@ -186,6 +187,8 @@ unit_test_dbus_LDADD = ell/libell-private.la unit_test_dbus_message_LDADD = ell/libell-private.la +unit_test_dbus_message_fds_LDADD = ell/libell-private.la + unit_test_dbus_util_LDADD = ell/libell-private.la unit_test_dbus_service_LDADD = ell/libell-private.la diff --git a/unit/test-dbus-message-fds.c b/unit/test-dbus-message-fds.c new file mode 100644 index 0000000..033d0a9 --- /dev/null +++ b/unit/test-dbus-message-fds.c @@ -0,0 +1,368 @@ +/* + * + * Embedded Linux library + * + * Copyright (C) 2011-2014 Intel Corporation. All rights reserved. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +#ifdef HAVE_CONFIG_H +#include <config.h> +#endif + +#include <unistd.h> +#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> + +#define TEST_BUS_ADDRESS "unix:path=/tmp/ell-test-bus" + +static pid_t dbus_daemon_pid = -1; + +static int kdbus_fd = -1; + +static char bus_address[128]; + +static void start_dbus_daemon(void) +{ + char *prg_argv[6]; + char *prg_envp[1]; + pid_t pid; + + prg_argv[0] = "/usr/bin/dbus-daemon"; + prg_argv[1] = "--session"; + prg_argv[2] = "--address=" TEST_BUS_ADDRESS; + prg_argv[3] = "--nopidfile"; + prg_argv[4] = "--nofork"; + prg_argv[5] = NULL; + + prg_envp[0] = NULL; + + l_info("launching dbus-daemon"); + + pid = fork(); + if (pid < 0) { + l_error("failed to fork new process"); + return; + } + + if (pid == 0) { + execve(prg_argv[0], prg_argv, prg_envp); + exit(EXIT_SUCCESS); + } + + l_info("dbus-daemon process %d created", pid); + + dbus_daemon_pid = pid; + + strcpy(bus_address, TEST_BUS_ADDRESS); +} + +static void create_kdbus(void) +{ + char bus_name[64]; + + snprintf(bus_name, sizeof(bus_name), "%u-ell-test", getuid()); + + kdbus_fd = _dbus_kernel_create_bus(bus_name); + if (kdbus_fd < 0) { + l_warn("kdbus not available"); + return; + } + + snprintf(bus_address, sizeof(bus_address), + "kernel:path=/dev/kdbus/%s/bus", bus_name); +} + +static void signal_handler(struct l_signal *signal, uint32_t signo, + void *user_data) +{ + switch (signo) { + case SIGINT: + case SIGTERM: + l_info("Terminate"); + l_main_quit(); + break; + case SIGCHLD: + while (1) { + pid_t pid; + int status; + + pid = waitpid(WAIT_ANY, &status, WNOHANG); + if (pid < 0 || pid == 0) + break; + + l_info("process %d terminated with status=%d\n", + pid, status); + + if (pid == dbus_daemon_pid) { + dbus_daemon_pid = -1; + l_main_quit(); + } + } + break; + } +} + +static struct l_dbus *dbus; + +struct dbus_test { + const char *name; + void (*start)(struct l_dbus *dbus, void *); + void *data; +}; + +static bool success; +static struct l_queue *tests; + +static void test_add(const char *name, + void (*start)(struct l_dbus *dbus, void *), + void *test_data) +{ + struct dbus_test *test = l_new(struct dbus_test, 1); + + test->name = name; + test->start = start; + test->data = test_data; + + if (!tests) + tests = l_queue_new(); + + l_queue_push_tail(tests, test); +} + +static void test_next() +{ + struct dbus_test *test = l_queue_pop_head(tests); + + if (!test) { + success = true; + l_main_quit(); + return; + } + + l_info("TEST: %s", test->name); + + test->start(dbus, test->data); + + l_free(test); +} + +#define test_assert(cond) \ + do { \ + if (!(cond)) { \ + l_info("TEST FAILED in %s at %s:%i: %s", \ + __func__, __FILE__, __LINE__, \ + L_STRINGIFY(cond)); \ + l_main_quit(); \ + return; \ + } \ + } while (0) + +static void request_name_callback(struct l_dbus *dbus, bool success, + bool queued, void *user_data) +{ + l_info("request name result=%s", + success ? (queued ? "queued" : "success") : "failed"); + + test_next(); +} + +static void ready_callback(void *user_data) +{ + l_info("ready"); + + l_dbus_name_acquire(dbus, "org.test", false, false, false, + request_name_callback, NULL); +} + +static void disconnect_callback(void *user_data) +{ + l_info("Disconnected from DBus"); + l_main_quit(); +} + +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_method(interface, "GetRandom", 0, get_random_callback, + "h", "", "randomfd"); +} + +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_1(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; + sigset_t mask; + int i; + bool kdbus = false; + + for (i = 1; i < argc; i++) { + if (!strcmp(argv[i], "--kdbus")) + kdbus = true; + } + + sigemptyset(&mask); + sigaddset(&mask, SIGINT); + sigaddset(&mask, SIGTERM); + sigaddset(&mask, SIGCHLD); + + signal = l_signal_create(&mask, signal_handler, NULL, NULL); + + l_log_set_stderr(); + + if (kdbus) + create_kdbus(); + else + start_dbus_daemon(); + + if (!bus_address[0]) + return -1; + + for (i = 0; i < 10; i++) { + usleep(200 * 1000); + + dbus = l_dbus_new(bus_address); + if (dbus) + break; + } + + l_dbus_set_ready_handler(dbus, ready_callback, dbus, NULL); + l_dbus_set_disconnect_handler(dbus, disconnect_callback, NULL, NULL); + + if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface, + NULL, true)) { + l_info("Unable to register interface"); + goto done; + } + + if (!l_dbus_object_add_interface(dbus, "/test", "org.test", NULL)) { + l_info("Unable to instantiate interface"); + goto done; + } + + test_add("FD passing 1", test_fd_passing_1, NULL); + + l_main_run(); + + l_queue_destroy(tests, l_free); + +done: + l_dbus_destroy(dbus); + + if (dbus_daemon_pid > 0) + kill(dbus_daemon_pid, SIGKILL); + + if (kdbus_fd >= 0) + close(kdbus_fd); + + l_signal_remove(signal); + + if (!success) + abort(); + + return 0; +} -- 2.7.4

--- 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.7.4

Loop until we have received or sent all of the data requested. We pass MSG_WAITALL to potentially reduce the number of iterations. Make sure we free body and header in classic_recv_message when dbus_message_build() returns an error. Calling sendms and recvmsg multiple times makes it even more tricky to understand what will happen with the FDs passed as oob data when a partial read/write happens or when there are multiple messsages with FDs attached in the queue. What this patch does should be equivalent to what systemd dbus code does. It seems to assume that the FDs are attached to the last byte of the message so on a partial write, the FDs are not written and the full array can be passed to sendmsg() again. I'm not sure if this is documented anywhere but I've seen an explicit statement of this at ... --- ell/dbus.c | 179 +++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 110 insertions(+), 69 deletions(-) diff --git a/ell/dbus.c b/ell/dbus.c index 35378b2..3ca7b87 100644 --- a/ell/dbus.c +++ b/ell/dbus.c @@ -579,63 +579,126 @@ static void classic_free(struct l_dbus *dbus) l_free(classic); } +static bool msg_op(int fd, bool send, struct iovec iov[], + int **fds, uint32_t *num_fds) +{ + int r; + struct msghdr msg; + struct cmsghdr *cmsg; + uint8_t *fd_buf = NULL; + int buf_fds; + int iovlen = 2; + + if (send) + buf_fds = *num_fds; + else { + buf_fds = 16; + *num_fds = 0; + } + + if (buf_fds) { + fd_buf = alloca(CMSG_SPACE(buf_fds * sizeof(int))); + + if (send) { + msg.msg_control = fd_buf; + msg.msg_controllen = CMSG_LEN(buf_fds * sizeof(int)); + + /* Set up fd_buf contents */ + cmsg = CMSG_FIRSTHDR(&msg); + cmsg->cmsg_len = msg.msg_controllen; + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + memcpy(CMSG_DATA(cmsg), *fds, buf_fds * sizeof(int)); + } + } + + while (iovlen) { + memset(&msg, 0, sizeof(msg)); + msg.msg_iov = iov; + msg.msg_iovlen = iovlen; + msg.msg_control = fd_buf; + msg.msg_controllen = fd_buf ? + CMSG_LEN(buf_fds * sizeof(int)) : 0; + + if (send) + r = sendmsg(fd, &msg, 0); + else + r = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC | MSG_WAITALL); + + if (r <= 0) { + if (r != -1 || errno != EINTR) + return false; + + continue; + } + + while (iovlen && r >= (int) iov[0].iov_len) { + r -= iov[0].iov_len; + iov++; + iovlen--; + } + + if (iovlen) { + iov[0].iov_base += r; + iov[0].iov_len -= r; + } + + if (send) + continue; + + for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; + cmsg = CMSG_NXTHDR(&msg, cmsg)) { + int cnt; + + if (cmsg->cmsg_level != SOL_SOCKET || + cmsg->cmsg_type != SCM_RIGHTS) + continue; + + cnt = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); + + *fds = l_realloc(*fds, (*num_fds + cnt) * sizeof(int)); + + memcpy(*fds + *num_fds, CMSG_DATA(cmsg), + cnt * sizeof(int)); + + *num_fds += cnt; + } + } + + return true; +} + static bool classic_send_message(struct l_dbus *dbus, struct l_dbus_message *message) { int fd = l_io_get_fd(dbus->io); - struct msghdr msg; struct iovec iov[2]; - ssize_t len; int *fds; uint32_t num_fds = 0; - struct cmsghdr *cmsg; iov[0].iov_base = _dbus_message_get_header(message, &iov[0].iov_len); iov[1].iov_base = _dbus_message_get_body(message, &iov[1].iov_len); - memset(&msg, 0, sizeof(msg)); - msg.msg_iov = iov; - msg.msg_iovlen = 2; - if (dbus->support_unix_fd) fds = _dbus_message_get_fds(message, &num_fds); - if (num_fds) { - msg.msg_control = alloca(CMSG_SPACE(num_fds * sizeof(int))); - msg.msg_controllen = CMSG_LEN(num_fds * sizeof(int)); - - cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_len = msg.msg_controllen; - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - memcpy(CMSG_DATA(cmsg), fds, num_fds * sizeof(int)); - } - - len = sendmsg(fd, &msg, 0); - if (len < 0) - return false; - - return true; + return msg_op(fd, true, iov, &fds, &num_fds); } static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus) { int fd = l_io_get_fd(dbus->io); struct dbus_header hdr; - struct msghdr msg; struct iovec iov[2]; - struct cmsghdr *cmsg; ssize_t len; void *header, *body; size_t header_size, body_size; - union { - uint8_t bytes[CMSG_SPACE(16 * sizeof(int))]; - struct cmsghdr align; - } fd_buf; int *fds = NULL; uint32_t num_fds = 0; + struct l_dbus_message *msg; + unsigned int i; - len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK); + len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK | MSG_WAITALL); if (len != DBUS_HEADER_SIZE) return NULL; @@ -650,59 +713,37 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus) iov[1].iov_base = body; iov[1].iov_len = body_size; - memset(&msg, 0, sizeof(msg)); - msg.msg_iov = iov; - msg.msg_iovlen = 2; - msg.msg_control = &fd_buf; - msg.msg_controllen = sizeof(fd_buf); - - len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC); - if (len < 0) - goto cmsg_fail; - - for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; - cmsg = CMSG_NXTHDR(&msg, cmsg)) { - unsigned int i; - - if (cmsg->cmsg_level != SOL_SOCKET || - cmsg->cmsg_type != SCM_RIGHTS) - continue; - - num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / 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(fds[i], F_GETFD, NULL); - if (flags < 0) - continue; - - if (!(flags & FD_CLOEXEC)) - fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC); - } - } + if (!msg_op(fd, false, iov, &fds, &num_fds)) + goto fail; if (hdr.endian != DBUS_NATIVE_ENDIAN) { l_util_debug(dbus->debug_handler, dbus->debug_data, "Endianness incorrect"); - goto bad_msg; + goto fail; } if (hdr.version != 1) { l_util_debug(dbus->debug_handler, dbus->debug_data, "Protocol version incorrect"); - goto bad_msg; + goto fail; } - return dbus_message_build(header, header_size, body, body_size, + msg = dbus_message_build(header, header_size, body, body_size, fds, num_fds); + if (!msg) + goto fail; -bad_msg: -cmsg_fail: - l_free(header); + l_free(fds); + + return msg; + +fail: + for (i = 0; i < num_fds; i++) + close(fds[i]); + + l_free(fds); l_free(body); + l_free(header); return NULL; } -- 2.7.4

Hi Andrew, On 05/04/2016 06:03 AM, Andrew Zaborowski wrote: I think this is a bad assumption to make. Browsing af_unix.c makes me think this is not the case. Please don't do this, the code is so much easier to understand when the rx and tx paths are separate. Strictly speaking, you can't call sendmsg in a loop. The socket is marked non-blocking, so EWOULDBLOCK or EAGAIN is a valid error return. This means that you need to exit the loop and re-enter once POLLOUT is received. Regards, -Denis