From 9ce22da0d834f0c9f57bd36f5d0d10e5e2f4992c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:12 +0100 Subject: [PATCH 01/11] test-util-sockets: Plug file descriptor leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- tests/test-util-sockets.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index f6336e0f91..15da867b8f 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -252,6 +252,7 @@ static gpointer unix_server_thread_func(gpointer user_data) connfd = accept(fd, (struct sockaddr *)&un, &len); g_assert_cmpint(connfd, !=, -1); + close(connfd); close(fd); From d1a393211b5333f9374b439394424f594f69d282 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:13 +0100 Subject: [PATCH 02/11] test-util-sockets: Correct to set has_abstract, has_tight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code tested doesn't care, which is a bug I will fix shortly. Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- tests/test-util-sockets.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 15da867b8f..9d317e73a6 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -243,7 +243,9 @@ static gpointer unix_server_thread_func(gpointer user_data) addr.type = SOCKET_ADDRESS_TYPE_UNIX; addr.u.q_unix.path = abstract_sock_name; + addr.u.q_unix.has_tight = true; addr.u.q_unix.tight = user_data != NULL; + addr.u.q_unix.has_abstract = true; addr.u.q_unix.abstract = true; fd = socket_listen(&addr, 1, &err); @@ -267,7 +269,9 @@ static gpointer unix_client_thread_func(gpointer user_data) addr.type = SOCKET_ADDRESS_TYPE_UNIX; addr.u.q_unix.path = abstract_sock_name; + addr.u.q_unix.has_tight = true; addr.u.q_unix.tight = user_data != NULL; + addr.u.q_unix.has_abstract = true; addr.u.q_unix.abstract = true; fd = socket_connect(&addr, &err); From 718a9be02df880ca4b4e34ce253daf2bfc5d059c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:14 +0100 Subject: [PATCH 03/11] test-util-sockets: Clean up SocketAddress construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The thread functions build the SocketAddress from global variable @abstract_sock_name and the tight flag passed as pointer argument (either NULL or (gpointer)1). There is no need for such hackery; simply pass the SocketAddress instead. While there, dumb down g_rand_int_range() to g_random_int(). Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- tests/test-util-sockets.c | 64 ++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 9d317e73a6..a4792253ba 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -230,25 +230,15 @@ static void test_socket_fd_pass_num_nocli(void) #endif #ifdef __linux__ -static gchar *abstract_sock_name; - static gpointer unix_server_thread_func(gpointer user_data) { - SocketAddress addr; - Error *err = NULL; - int fd = -1; - int connfd = -1; + SocketAddress *addr = user_data; + int fd; + int connfd; struct sockaddr_un un; socklen_t len = sizeof(un); - addr.type = SOCKET_ADDRESS_TYPE_UNIX; - addr.u.q_unix.path = abstract_sock_name; - addr.u.q_unix.has_tight = true; - addr.u.q_unix.tight = user_data != NULL; - addr.u.q_unix.has_abstract = true; - addr.u.q_unix.abstract = true; - - fd = socket_listen(&addr, 1, &err); + fd = socket_listen(addr, 1, &error_abort); g_assert_cmpint(fd, >=, 0); g_assert(fd_is_socket(fd)); @@ -257,69 +247,67 @@ static gpointer unix_server_thread_func(gpointer user_data) close(connfd); close(fd); - return NULL; } static gpointer unix_client_thread_func(gpointer user_data) { - SocketAddress addr; - Error *err = NULL; - int fd = -1; - - addr.type = SOCKET_ADDRESS_TYPE_UNIX; - addr.u.q_unix.path = abstract_sock_name; - addr.u.q_unix.has_tight = true; - addr.u.q_unix.tight = user_data != NULL; - addr.u.q_unix.has_abstract = true; - addr.u.q_unix.abstract = true; - - fd = socket_connect(&addr, &err); + SocketAddress *addr = user_data; + int fd; + fd = socket_connect(addr, &error_abort); g_assert_cmpint(fd, >=, 0); - close(fd); - return NULL; } static void test_socket_unix_abstract_good(void) { - GRand *r = g_rand_new(); + SocketAddress addr; - abstract_sock_name = g_strdup_printf("unix-%d-%d", getpid(), - g_rand_int_range(r, 100, 1000)); + addr.type = SOCKET_ADDRESS_TYPE_UNIX; + addr.u.q_unix.path = g_strdup_printf("unix-%d-%u", + getpid(), g_random_int()); + addr.u.q_unix.has_abstract = true; + addr.u.q_unix.abstract = true; /* non tight socklen serv and cli */ + + addr.u.q_unix.has_tight = false; + addr.u.q_unix.tight = false; + GThread *serv = g_thread_new("abstract_unix_server", unix_server_thread_func, - NULL); + &addr); sleep(1); GThread *cli = g_thread_new("abstract_unix_client", unix_client_thread_func, - NULL); + &addr); g_thread_join(cli); g_thread_join(serv); /* tight socklen serv and cli */ + + addr.u.q_unix.has_tight = true; + addr.u.q_unix.tight = true; + serv = g_thread_new("abstract_unix_server", unix_server_thread_func, - (gpointer)1); + &addr); sleep(1); cli = g_thread_new("abstract_unix_client", unix_client_thread_func, - (gpointer)1); + &addr); g_thread_join(cli); g_thread_join(serv); - g_free(abstract_sock_name); - g_rand_free(r); + g_free(addr.u.q_unix.path); } #endif From 89cb0bb554ee2365d948d3f593ea04f03d5bc4f8 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:15 +0100 Subject: [PATCH 04/11] test-util-sockets: Factor out test_socket_unix_abstract_one() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- tests/test-util-sockets.c | 48 ++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index a4792253ba..40ff893e64 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -261,6 +261,24 @@ static gpointer unix_client_thread_func(gpointer user_data) return NULL; } +static void test_socket_unix_abstract_one(SocketAddress *addr) +{ + GThread *serv, *cli; + + serv = g_thread_new("abstract_unix_server", + unix_server_thread_func, + addr); + + sleep(1); + + cli = g_thread_new("abstract_unix_client", + unix_client_thread_func, + addr); + + g_thread_join(cli); + g_thread_join(serv); +} + static void test_socket_unix_abstract_good(void) { SocketAddress addr; @@ -272,40 +290,14 @@ static void test_socket_unix_abstract_good(void) addr.u.q_unix.abstract = true; /* non tight socklen serv and cli */ - addr.u.q_unix.has_tight = false; addr.u.q_unix.tight = false; - - GThread *serv = g_thread_new("abstract_unix_server", - unix_server_thread_func, - &addr); - - sleep(1); - - GThread *cli = g_thread_new("abstract_unix_client", - unix_client_thread_func, - &addr); - - g_thread_join(cli); - g_thread_join(serv); + test_socket_unix_abstract_one(&addr); /* tight socklen serv and cli */ - addr.u.q_unix.has_tight = true; addr.u.q_unix.tight = true; - - serv = g_thread_new("abstract_unix_server", - unix_server_thread_func, - &addr); - - sleep(1); - - cli = g_thread_new("abstract_unix_client", - unix_client_thread_func, - &addr); - - g_thread_join(cli); - g_thread_join(serv); + test_socket_unix_abstract_one(&addr); g_free(addr.u.q_unix.path); } From 39458d4e3059d37e3331258a50fd77f8cf5b365e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:16 +0100 Subject: [PATCH 05/11] test-util-sockets: Synchronize properly, don't sleep(1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abstract sockets test spawns a thread to listen and accept, and a second one to connect, with a sleep(1) in between to "ensure" the former is listening when the latter tries to connect. Review fail. Risks spurious test failure, say when a heavily loaded machine doesn't schedule the first thread quickly enough. It's also slow. Listen and accept in the main thread, and start the connect thread in between. Look ma, no sleep! Run time drops from 2s wall clock to a few milliseconds. Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- tests/test-util-sockets.c | 40 +++++++++++++-------------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 40ff893e64..4cedf622f0 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -230,26 +230,6 @@ static void test_socket_fd_pass_num_nocli(void) #endif #ifdef __linux__ -static gpointer unix_server_thread_func(gpointer user_data) -{ - SocketAddress *addr = user_data; - int fd; - int connfd; - struct sockaddr_un un; - socklen_t len = sizeof(un); - - fd = socket_listen(addr, 1, &error_abort); - g_assert_cmpint(fd, >=, 0); - g_assert(fd_is_socket(fd)); - - connfd = accept(fd, (struct sockaddr *)&un, &len); - g_assert_cmpint(connfd, !=, -1); - close(connfd); - - close(fd); - return NULL; -} - static gpointer unix_client_thread_func(gpointer user_data) { SocketAddress *addr = user_data; @@ -263,20 +243,26 @@ static gpointer unix_client_thread_func(gpointer user_data) static void test_socket_unix_abstract_one(SocketAddress *addr) { - GThread *serv, *cli; + int fd, connfd; + GThread *cli; + struct sockaddr_un un; + socklen_t len = sizeof(un); - serv = g_thread_new("abstract_unix_server", - unix_server_thread_func, - addr); - - sleep(1); + fd = socket_listen(addr, 1, &error_abort); + g_assert_cmpint(fd, >=, 0); + g_assert(fd_is_socket(fd)); cli = g_thread_new("abstract_unix_client", unix_client_thread_func, addr); + connfd = accept(fd, (struct sockaddr *)&un, &len); + g_assert_cmpint(connfd, !=, -1); + close(connfd); + + close(fd); + g_thread_join(cli); - g_thread_join(serv); } static void test_socket_unix_abstract_good(void) From a72f6754a10ea4f4bf76e83ecaa7f82931991c24 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:17 +0100 Subject: [PATCH 06/11] test-util-sockets: Test the complete abstract socket matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test covers only two out of nine combinations. Test all nine. Four turn out to be broken. Marked /* BUG */. Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- tests/test-util-sockets.c | 87 ++++++++++++++++++++++++++++----------- 1 file changed, 63 insertions(+), 24 deletions(-) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 4cedf622f0..f8b6586e70 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -230,60 +230,99 @@ static void test_socket_fd_pass_num_nocli(void) #endif #ifdef __linux__ + +#define ABSTRACT_SOCKET_VARIANTS 3 + +typedef struct { + SocketAddress *server, *client[ABSTRACT_SOCKET_VARIANTS]; + bool expect_connect[ABSTRACT_SOCKET_VARIANTS]; +} abstract_socket_matrix_row; + static gpointer unix_client_thread_func(gpointer user_data) { - SocketAddress *addr = user_data; - int fd; + abstract_socket_matrix_row *row = user_data; + Error *err = NULL; + int i, fd; - fd = socket_connect(addr, &error_abort); - g_assert_cmpint(fd, >=, 0); - close(fd); + for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) { + if (row->expect_connect[i]) { + fd = socket_connect(row->client[i], &error_abort); + g_assert_cmpint(fd, >=, 0); + } else { + fd = socket_connect(row->client[i], &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + } + close(fd); + } return NULL; } -static void test_socket_unix_abstract_one(SocketAddress *addr) +static void test_socket_unix_abstract_row(abstract_socket_matrix_row *test) { - int fd, connfd; + int fd, connfd, i; GThread *cli; struct sockaddr_un un; socklen_t len = sizeof(un); - fd = socket_listen(addr, 1, &error_abort); + /* Last one must connect, or else accept() below hangs */ + assert(test->expect_connect[ABSTRACT_SOCKET_VARIANTS - 1]); + + fd = socket_listen(test->server, 1, &error_abort); g_assert_cmpint(fd, >=, 0); g_assert(fd_is_socket(fd)); cli = g_thread_new("abstract_unix_client", unix_client_thread_func, - addr); + test); - connfd = accept(fd, (struct sockaddr *)&un, &len); - g_assert_cmpint(connfd, !=, -1); - close(connfd); + for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) { + if (test->expect_connect[i]) { + connfd = accept(fd, (struct sockaddr *)&un, &len); + g_assert_cmpint(connfd, !=, -1); + close(connfd); + } + } close(fd); - g_thread_join(cli); } -static void test_socket_unix_abstract_good(void) +static void test_socket_unix_abstract(void) { - SocketAddress addr; + SocketAddress addr, addr_tight, addr_padded; + abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = { + { &addr, + { &addr_tight, &addr_padded, &addr }, + { false /* BUG */, true /* BUG */, true } }, + { &addr_tight, + { &addr_padded, &addr, &addr_tight }, + { false, false /* BUG */, true } }, + { &addr_padded, + { &addr, &addr_tight, &addr_padded }, + { true /* BUG */, false, true } } + }; + int i; addr.type = SOCKET_ADDRESS_TYPE_UNIX; addr.u.q_unix.path = g_strdup_printf("unix-%d-%u", getpid(), g_random_int()); addr.u.q_unix.has_abstract = true; addr.u.q_unix.abstract = true; - - /* non tight socklen serv and cli */ addr.u.q_unix.has_tight = false; addr.u.q_unix.tight = false; - test_socket_unix_abstract_one(&addr); - /* tight socklen serv and cli */ - addr.u.q_unix.has_tight = true; - addr.u.q_unix.tight = true; - test_socket_unix_abstract_one(&addr); + addr_tight = addr; + addr_tight.u.q_unix.has_tight = true; + addr_tight.u.q_unix.tight = true; + + addr_padded = addr; + addr_padded.u.q_unix.has_tight = true; + addr_padded.u.q_unix.tight = false; + + for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) { + test_socket_unix_abstract_row(&matrix[i]); + } g_free(addr.u.q_unix.path); } @@ -330,8 +369,8 @@ int main(int argc, char **argv) } #ifdef __linux__ - g_test_add_func("/util/socket/unix-abstract/good", - test_socket_unix_abstract_good); + g_test_add_func("/util/socket/unix-abstract", + test_socket_unix_abstract); #endif end: From b08cc97d6ba4250439829a8a1d476064a1cb54da Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:18 +0100 Subject: [PATCH 07/11] sockets: Fix default of UnixSocketAddress member @tight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An optional bool member of a QAPI struct can be false, true, or absent. The previous commit demonstrated that socket_listen() and socket_connect() are broken for absent @tight, and indeed QMP chardev- add also defaults absent member @tight to false instead of true. In C, QAPI members are represented by two fields, has_MEMBER and MEMBER. We have: has_MEMBER MEMBER false true false true true true absent false false/ignore When has_MEMBER is false, MEMBER should be set to false on write, and ignored on read. For QMP, the QAPI visitors handle absent @tight by setting both @has_tight and @tight to false. unix_listen_saddr() and unix_connect_saddr() however use @tight only, disregarding @has_tight. This is wrong and means that absent @tight defaults to false whereas it should default to true. The same is true for @has_abstract, though @abstract defaults to false and therefore has the same behavior for all of QMP, HMP and CLI. Fix unix_listen_saddr() and unix_connect_saddr() to check @has_abstract/@has_tight, and to default absent @tight to true. However, this is only half of the story. HMP chardev-add and CLI -chardev so far correctly defaulted @tight to true, but defaults to false again with the above fix for HMP and CLI. In fact, the "tight" and "abstract" options now break completely. Digging deeper, we find that qemu_chr_parse_socket() also ignores @has_tight, leaving it false when it sets @tight. That is also wrong, but the two wrongs cancelled out. Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract; writing testcases for HMP and CLI is left for another day. Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9 Reported-by: Kevin Wolf Reviewed-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- chardev/char-socket.c | 2 ++ tests/test-util-sockets.c | 6 +++--- util/qemu-sockets.c | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 95e45812d5..1ee5a8c295 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -1439,7 +1439,9 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX; q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1); q_unix->path = g_strdup(path); + q_unix->has_tight = true; q_unix->tight = tight; + q_unix->has_abstract = true; q_unix->abstract = abstract; } else if (host) { addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET; diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index f8b6586e70..7ecf95579b 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -294,13 +294,13 @@ static void test_socket_unix_abstract(void) abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = { { &addr, { &addr_tight, &addr_padded, &addr }, - { false /* BUG */, true /* BUG */, true } }, + { true, false, true } }, { &addr_tight, { &addr_padded, &addr, &addr_tight }, - { false, false /* BUG */, true } }, + { false, true, true } }, { &addr_padded, { &addr, &addr_tight, &addr_padded }, - { true /* BUG */, false, true } } + { false, false, true } } }; int i; diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 38f82179b0..3ceaa81226 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -925,7 +925,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, if (saddr->abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], path, pathlen); - if (saddr->tight) { + if (!saddr->has_tight || saddr->tight) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { @@ -985,7 +985,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) if (saddr->abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], saddr->path, pathlen); - if (saddr->tight) { + if (!saddr->has_tight || saddr->tight) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { From 3b14b4ec49a801067da19d6b8469eb1c1911c020 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:19 +0100 Subject: [PATCH 08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket support" neglected to update socket_sockaddr_to_address_unix(). The function returns a non-abstract socket address for abstract sockets (wrong) with a null @path (also wrong; a non-optional QAPI str member must never be null). The null @path is due to confused code going back all the way to commit 17c55decec "sockets: add helpers for creating SocketAddress from a socket". Add the required special case, and simplify the confused code. Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 3ceaa81226..a578c434c2 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1270,10 +1270,20 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa, addr = g_new0(SocketAddress, 1); addr->type = SOCKET_ADDRESS_TYPE_UNIX; - if (su->sun_path[0]) { - addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path)); +#ifdef CONFIG_LINUX + if (!su->sun_path[0]) { + /* Linux abstract socket */ + addr->u.q_unix.path = g_strndup(su->sun_path + 1, + sizeof(su->sun_path) - 1); + addr->u.q_unix.has_abstract = true; + addr->u.q_unix.abstract = true; + addr->u.q_unix.has_tight = true; + addr->u.q_unix.tight = salen < sizeof(*su); + return addr; } +#endif + addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path)); return addr; } #endif /* WIN32 */ From dea7cd1794f33c52e4b59fe085daffb318a4bb07 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:20 +0100 Subject: [PATCH 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket support" neglected to update qemu_chr_socket_address(). It shows shows neither @abstract nor @tight. Fix that. Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- chardev/char-socket.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 1ee5a8c295..27a2954f47 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -443,10 +443,22 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) s->is_listen ? ",server" : ""); break; case SOCKET_ADDRESS_TYPE_UNIX: - return g_strdup_printf("%sunix:%s%s", prefix, - s->addr->u.q_unix.path, + { + const char *tight = "", *abstract = ""; + UnixSocketAddress *sa = &s->addr->u.q_unix; + + if (sa->has_abstract && sa->abstract) { + abstract = ",abstract"; + if (sa->has_tight && sa->tight) { + tight = ",tight"; + } + } + + return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path, + abstract, tight, s->is_listen ? ",server" : ""); break; + } case SOCKET_ADDRESS_TYPE_FD: return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str, s->is_listen ? ",server" : ""); From ef298e3826e574c712d10e38a5f2a3629d6f5e01 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:21 +0100 Subject: [PATCH 10/11] sockets: Bypass "replace empty @path" for abstract unix sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit unix_listen_saddr() replaces empty @path by unique value. It obtains the value by creating and deleting a unique temporary file with mkstemp(). This is racy, as the comment explains. It's also entirely undocumented as far as I can tell. Goes back to commit d247d25f18 "sockets: helper functions for qemu (Gerd Hoffman)", v0.10.0. Since abstract socket addresses have no connection with filesystem pathnames, making them up with mkstemp() seems inappropriate. Bypass the replacement of empty @path. Reviewed-by: Eric Blake Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index a578c434c2..671717499f 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -877,7 +877,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, return -1; } - if (saddr->path && saddr->path[0]) { + if (saddr->path[0] || saddr->abstract) { path = saddr->path; } else { const char *tmpdir = getenv("TMPDIR"); From 8acefc79deaab1c7ee2ab07b540b0e3edf0f9f47 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 2 Nov 2020 10:44:22 +0100 Subject: [PATCH 11/11] sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abstract socket namespace is a non-portable Linux extension. An attempt to use it elsewhere should fail with ENOENT (the abstract address looks like a "" pathname, which does not resolve). We report this failure like Failed to connect socket abc: No such file or directory Tolerable, although ENOTSUP would be better. However, introspection lies: it has @abstract regardless of host support. Easy enough to fix: since Linux provides them since 2.2, 'if': 'defined(CONFIG_LINUX)' should do. The above failure becomes Parameter 'backend.data.addr.data.abstract' is unexpected I consider this an improvement. Reviewed-by: Paolo Bonzini Signed-off-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- chardev/char-socket.c | 6 ++++++ chardev/char.c | 2 ++ qapi/sockets.json | 14 ++++++++------ tests/test-util-sockets.c | 7 ++++--- util/qemu-sockets.c | 40 +++++++++++++++++++++++++++++---------- 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 27a2954f47..213a4c8dd0 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -447,12 +447,14 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) const char *tight = "", *abstract = ""; UnixSocketAddress *sa = &s->addr->u.q_unix; +#ifdef CONFIG_LINUX if (sa->has_abstract && sa->abstract) { abstract = ",abstract"; if (sa->has_tight && sa->tight) { tight = ",tight"; } } +#endif return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path, abstract, tight, @@ -1398,8 +1400,10 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, const char *host = qemu_opt_get(opts, "host"); const char *port = qemu_opt_get(opts, "port"); const char *fd = qemu_opt_get(opts, "fd"); +#ifdef CONFIG_LINUX bool tight = qemu_opt_get_bool(opts, "tight", true); bool abstract = qemu_opt_get_bool(opts, "abstract", false); +#endif SocketAddressLegacy *addr; ChardevSocket *sock; @@ -1451,10 +1455,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX; q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1); q_unix->path = g_strdup(path); +#ifdef CONFIG_LINUX q_unix->has_tight = true; q_unix->tight = tight; q_unix->has_abstract = true; q_unix->abstract = abstract; +#endif } else if (host) { addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET; addr->u.inet.data = g_new(InetSocketAddress, 1); diff --git a/chardev/char.c b/chardev/char.c index 78553125d3..aa4282164a 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -928,6 +928,7 @@ QemuOptsList qemu_chardev_opts = { },{ .name = "logappend", .type = QEMU_OPT_BOOL, +#ifdef CONFIG_LINUX },{ .name = "tight", .type = QEMU_OPT_BOOL, @@ -935,6 +936,7 @@ QemuOptsList qemu_chardev_opts = { },{ .name = "abstract", .type = QEMU_OPT_BOOL, +#endif }, { /* end of list */ } }, diff --git a/qapi/sockets.json b/qapi/sockets.json index c0c640a5b0..2e83452797 100644 --- a/qapi/sockets.json +++ b/qapi/sockets.json @@ -74,18 +74,20 @@ # Captures a socket address in the local ("Unix socket") namespace. # # @path: filesystem path to use -# @tight: pass a socket address length confined to the minimum length of the -# abstract string, rather than the full sockaddr_un record length -# (only matters for abstract sockets, default true). (Since 5.1) -# @abstract: whether this is an abstract address, default false. (Since 5.1) +# @abstract: if true, this is a Linux abstract socket address. @path +# will be prefixed by a null byte, and optionally padded +# with null bytes. Defaults to false. (Since 5.1) +# @tight: if false, pad an abstract socket address with enough null +# bytes to make it fill struct sockaddr_un member sun_path. +# Defaults to true. (Since 5.1) # # Since: 1.3 ## { 'struct': 'UnixSocketAddress', 'data': { 'path': 'str', - '*tight': 'bool', - '*abstract': 'bool' } } + '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' }, + '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } } ## # @VsockSocketAddress: diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 7ecf95579b..67486055ed 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -229,7 +229,7 @@ static void test_socket_fd_pass_num_nocli(void) } #endif -#ifdef __linux__ +#ifdef CONFIG_LINUX #define ABSTRACT_SOCKET_VARIANTS 3 @@ -326,7 +326,8 @@ static void test_socket_unix_abstract(void) g_free(addr.u.q_unix.path); } -#endif + +#endif /* CONFIG_LINUX */ int main(int argc, char **argv) { @@ -368,7 +369,7 @@ int main(int argc, char **argv) #endif } -#ifdef __linux__ +#ifdef CONFIG_LINUX g_test_add_func("/util/socket/unix-abstract", test_socket_unix_abstract); #endif diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 671717499f..8af0278f15 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -860,10 +860,29 @@ static int vsock_parse(VsockSocketAddress *addr, const char *str, #ifndef _WIN32 +static bool saddr_is_abstract(UnixSocketAddress *saddr) +{ +#ifdef CONFIG_LINUX + return saddr->abstract; +#else + return false; +#endif +} + +static bool saddr_is_tight(UnixSocketAddress *saddr) +{ +#ifdef CONFIG_LINUX + return !saddr->has_tight || saddr->tight; +#else + return false; +#endif +} + static int unix_listen_saddr(UnixSocketAddress *saddr, int num, Error **errp) { + bool abstract = saddr_is_abstract(saddr); struct sockaddr_un un; int sock, fd; char *pathbuf = NULL; @@ -877,7 +896,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, return -1; } - if (saddr->path[0] || saddr->abstract) { + if (saddr->path[0] || abstract) { path = saddr->path; } else { const char *tmpdir = getenv("TMPDIR"); @@ -887,10 +906,10 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, pathlen = strlen(path); if (pathlen > sizeof(un.sun_path) || - (saddr->abstract && pathlen > (sizeof(un.sun_path) - 1))) { + (abstract && pathlen > (sizeof(un.sun_path) - 1))) { error_setg(errp, "UNIX socket path '%s' is too long", path); error_append_hint(errp, "Path must be less than %zu bytes\n", - saddr->abstract ? sizeof(un.sun_path) - 1 : + abstract ? sizeof(un.sun_path) - 1 : sizeof(un.sun_path)); goto err; } @@ -912,7 +931,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, close(fd); } - if (!saddr->abstract && unlink(path) < 0 && errno != ENOENT) { + if (!abstract && unlink(path) < 0 && errno != ENOENT) { error_setg_errno(errp, errno, "Failed to unlink socket %s", path); goto err; @@ -922,10 +941,10 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, un.sun_family = AF_UNIX; addrlen = sizeof(un); - if (saddr->abstract) { + if (abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], path, pathlen); - if (!saddr->has_tight || saddr->tight) { + if (saddr_is_tight(saddr)) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else { @@ -952,6 +971,7 @@ err: static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) { + bool abstract = saddr_is_abstract(saddr); struct sockaddr_un un; int sock, rc; size_t pathlen; @@ -970,10 +990,10 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) pathlen = strlen(saddr->path); if (pathlen > sizeof(un.sun_path) || - (saddr->abstract && pathlen > (sizeof(un.sun_path) - 1))) { + (abstract && pathlen > (sizeof(un.sun_path) - 1))) { error_setg(errp, "UNIX socket path '%s' is too long", saddr->path); error_append_hint(errp, "Path must be less than %zu bytes\n", - saddr->abstract ? sizeof(un.sun_path) - 1 : + abstract ? sizeof(un.sun_path) - 1 : sizeof(un.sun_path)); goto err; } @@ -982,10 +1002,10 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) un.sun_family = AF_UNIX; addrlen = sizeof(un); - if (saddr->abstract) { + if (abstract) { un.sun_path[0] = '\0'; memcpy(&un.sun_path[1], saddr->path, pathlen); - if (!saddr->has_tight || saddr->tight) { + if (saddr_is_tight(saddr)) { addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen; } } else {