From patchwork Thu Jan 24 12:26:14 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= X-Patchwork-Id: 10778841 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id F24861515 for ; Thu, 24 Jan 2019 12:27:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E156C2EF45 for ; Thu, 24 Jan 2019 12:27:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D5D4F2EF47; Thu, 24 Jan 2019 12:27:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 0F8D42EF45 for ; Thu, 24 Jan 2019 12:27:42 +0000 (UTC) Received: from localhost ([127.0.0.1]:52650 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gme6f-00025q-AG for patchwork-qemu-devel@patchwork.kernel.org; Thu, 24 Jan 2019 07:27:41 -0500 Received: from eggs.gnu.org ([209.51.188.92]:37313) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gme5T-0001D7-Kc for qemu-devel@nongnu.org; Thu, 24 Jan 2019 07:26:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gme5Q-0005HV-T6 for qemu-devel@nongnu.org; Thu, 24 Jan 2019 07:26:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43480) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gme5O-0005B4-Kl for qemu-devel@nongnu.org; Thu, 24 Jan 2019 07:26:22 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7F38D637E5; Thu, 24 Jan 2019 12:26:20 +0000 (UTC) Received: from localhost.localdomain.com (ovpn-112-61.ams2.redhat.com [10.36.112.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6862726DFA; Thu, 24 Jan 2019 12:26:18 +0000 (UTC) From: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= To: qemu-devel@nongnu.org Date: Thu, 24 Jan 2019 12:26:14 +0000 Message-Id: <20190124122614.16624-2-berrange@redhat.com> In-Reply-To: <20190124122614.16624-1-berrange@redhat.com> References: <20190124122614.16624-1-berrange@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 24 Jan 2019 12:26:20 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL 1/1] io: ensure UNIX client doesn't unlink server socket X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP The qio_channel_socket_close method for was mistakenly unlinking the UNIX server socket, even if the channel was a client connection. This was not noticed with chardevs, since they never call close, but with the VNC server, this caused the VNC server socket to be deleted after the first client quit. The qio_channel_socket_close method also needlessly reimplemented the logic that already exists in socket_listen_cleanup(). Just call that method directly, for listen sockets only. This fixes a regression introduced in QEMU 3.0.0 with commit d66f78e1eaa832f73c771d9df1b606fe75d52a50 Author: Pavel Balaev Date: Mon May 21 19:17:35 2018 +0300 Delete AF_UNIX socket after close Fixes launchpad #1795100 Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- io/channel-socket.c | 19 ++------ tests/test-io-channel-socket.c | 86 ++++++++++++++++++++++++++++++---- 2 files changed, 80 insertions(+), 25 deletions(-) diff --git a/io/channel-socket.c b/io/channel-socket.c index b50e63a053..bc5f80e780 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -688,10 +688,13 @@ qio_channel_socket_close(QIOChannel *ioc, int rc = 0; if (sioc->fd != -1) { - SocketAddress *addr = socket_local_address(sioc->fd, errp); #ifdef WIN32 WSAEventSelect(sioc->fd, NULL, 0); #endif + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) { + socket_listen_cleanup(sioc->fd, errp); + } + if (closesocket(sioc->fd) < 0) { sioc->fd = -1; error_setg_errno(errp, errno, @@ -699,20 +702,6 @@ qio_channel_socket_close(QIOChannel *ioc, return -1; } sioc->fd = -1; - - if (addr && addr->type == SOCKET_ADDRESS_TYPE_UNIX - && addr->u.q_unix.path) { - if (unlink(addr->u.q_unix.path) < 0 && errno != ENOENT) { - error_setg_errno(errp, errno, - "Failed to unlink socket %s", - addr->u.q_unix.path); - rc = -1; - } - } - - if (addr) { - qapi_free_SocketAddress(addr); - } } return rc; } diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c index 0597213f93..c253ae30f5 100644 --- a/tests/test-io-channel-socket.c +++ b/tests/test-io-channel-socket.c @@ -49,6 +49,7 @@ static void test_io_channel_set_socket_bufs(QIOChannel *src, static void test_io_channel_setup_sync(SocketAddress *listen_addr, SocketAddress *connect_addr, + QIOChannel **srv, QIOChannel **src, QIOChannel **dst) { @@ -78,7 +79,7 @@ static void test_io_channel_setup_sync(SocketAddress *listen_addr, test_io_channel_set_socket_bufs(*src, *dst); - object_unref(OBJECT(lioc)); + *srv = QIO_CHANNEL(lioc); } @@ -99,6 +100,7 @@ static void test_io_channel_complete(QIOTask *task, static void test_io_channel_setup_async(SocketAddress *listen_addr, SocketAddress *connect_addr, + QIOChannel **srv, QIOChannel **src, QIOChannel **dst) { @@ -146,21 +148,34 @@ static void test_io_channel_setup_async(SocketAddress *listen_addr, qio_channel_set_delay(*src, false); test_io_channel_set_socket_bufs(*src, *dst); - object_unref(OBJECT(lioc)); + *srv = QIO_CHANNEL(lioc); g_main_loop_unref(data.loop); } +static void test_io_channel_socket_path_exists(SocketAddress *addr, + bool expectExists) +{ + if (addr->type != SOCKET_ADDRESS_TYPE_UNIX) { + return; + } + + g_assert(g_file_test(addr->u.q_unix.path, + G_FILE_TEST_EXISTS) == expectExists); +} + + static void test_io_channel(bool async, SocketAddress *listen_addr, SocketAddress *connect_addr, bool passFD) { - QIOChannel *src, *dst; + QIOChannel *src, *dst, *srv; QIOChannelTest *test; if (async) { - test_io_channel_setup_async(listen_addr, connect_addr, &src, &dst); + test_io_channel_setup_async(listen_addr, connect_addr, + &srv, &src, &dst); g_assert(!passFD || qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS)); @@ -169,14 +184,25 @@ static void test_io_channel(bool async, g_assert(qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_SHUTDOWN)); g_assert(qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_SHUTDOWN)); + test_io_channel_socket_path_exists(listen_addr, true); + test = qio_channel_test_new(); qio_channel_test_run_threads(test, true, src, dst); qio_channel_test_validate(test); + test_io_channel_socket_path_exists(listen_addr, true); + + /* unref without close, to ensure finalize() cleans up */ + object_unref(OBJECT(src)); object_unref(OBJECT(dst)); + test_io_channel_socket_path_exists(listen_addr, true); - test_io_channel_setup_async(listen_addr, connect_addr, &src, &dst); + object_unref(OBJECT(srv)); + test_io_channel_socket_path_exists(listen_addr, false); + + test_io_channel_setup_async(listen_addr, connect_addr, + &srv, &src, &dst); g_assert(!passFD || qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS)); @@ -189,10 +215,24 @@ static void test_io_channel(bool async, qio_channel_test_run_threads(test, false, src, dst); qio_channel_test_validate(test); + /* close before unref, to ensure finalize copes with already closed */ + + qio_channel_close(src, &error_abort); + qio_channel_close(dst, &error_abort); + test_io_channel_socket_path_exists(listen_addr, true); + object_unref(OBJECT(src)); object_unref(OBJECT(dst)); + test_io_channel_socket_path_exists(listen_addr, true); + + qio_channel_close(srv, &error_abort); + test_io_channel_socket_path_exists(listen_addr, false); + + object_unref(OBJECT(srv)); + test_io_channel_socket_path_exists(listen_addr, false); } else { - test_io_channel_setup_sync(listen_addr, connect_addr, &src, &dst); + test_io_channel_setup_sync(listen_addr, connect_addr, + &srv, &src, &dst); g_assert(!passFD || qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS)); @@ -201,14 +241,25 @@ static void test_io_channel(bool async, g_assert(qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_SHUTDOWN)); g_assert(qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_SHUTDOWN)); + test_io_channel_socket_path_exists(listen_addr, true); + test = qio_channel_test_new(); qio_channel_test_run_threads(test, true, src, dst); qio_channel_test_validate(test); + test_io_channel_socket_path_exists(listen_addr, true); + + /* unref without close, to ensure finalize() cleans up */ + object_unref(OBJECT(src)); object_unref(OBJECT(dst)); + test_io_channel_socket_path_exists(listen_addr, true); + + object_unref(OBJECT(srv)); + test_io_channel_socket_path_exists(listen_addr, false); - test_io_channel_setup_sync(listen_addr, connect_addr, &src, &dst); + test_io_channel_setup_sync(listen_addr, connect_addr, + &srv, &src, &dst); g_assert(!passFD || qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS)); @@ -221,8 +272,23 @@ static void test_io_channel(bool async, qio_channel_test_run_threads(test, false, src, dst); qio_channel_test_validate(test); + test_io_channel_socket_path_exists(listen_addr, true); + + /* close before unref, to ensure finalize copes with already closed */ + + qio_channel_close(src, &error_abort); + qio_channel_close(dst, &error_abort); + test_io_channel_socket_path_exists(listen_addr, true); + object_unref(OBJECT(src)); object_unref(OBJECT(dst)); + test_io_channel_socket_path_exists(listen_addr, true); + + qio_channel_close(srv, &error_abort); + test_io_channel_socket_path_exists(listen_addr, false); + + object_unref(OBJECT(srv)); + test_io_channel_socket_path_exists(listen_addr, false); } } @@ -316,7 +382,6 @@ static void test_io_channel_unix(bool async) qapi_free_SocketAddress(listen_addr); qapi_free_SocketAddress(connect_addr); - g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS) == FALSE); } @@ -335,7 +400,7 @@ static void test_io_channel_unix_fd_pass(void) { SocketAddress *listen_addr = g_new0(SocketAddress, 1); SocketAddress *connect_addr = g_new0(SocketAddress, 1); - QIOChannel *src, *dst; + QIOChannel *src, *dst, *srv; int testfd; int fdsend[3]; int *fdrecv = NULL; @@ -359,7 +424,7 @@ static void test_io_channel_unix_fd_pass(void) connect_addr->type = SOCKET_ADDRESS_TYPE_UNIX; connect_addr->u.q_unix.path = g_strdup(TEST_SOCKET); - test_io_channel_setup_sync(listen_addr, connect_addr, &src, &dst); + test_io_channel_setup_sync(listen_addr, connect_addr, &srv, &src, &dst); memcpy(bufsend, "Hello World", G_N_ELEMENTS(bufsend)); @@ -412,6 +477,7 @@ static void test_io_channel_unix_fd_pass(void) object_unref(OBJECT(src)); object_unref(OBJECT(dst)); + object_unref(OBJECT(srv)); qapi_free_SocketAddress(listen_addr); qapi_free_SocketAddress(connect_addr); unlink(TEST_SOCKET);