From nobody Thu May 2 02:48:00 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (208.118.235.17 [208.118.235.17]) by mx.zohomail.com with SMTPS id 1509982541373742.9548718652761; Mon, 6 Nov 2017 07:35:41 -0800 (PST) Received: from localhost ([::1]:48706 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBjQy-0001Wk-E7 for importer@patchew.org; Mon, 06 Nov 2017 10:35:32 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBjPP-0000ce-Bv for qemu-devel@nongnu.org; Mon, 06 Nov 2017 10:33:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eBjPM-0002cg-Ie for qemu-devel@nongnu.org; Mon, 06 Nov 2017 10:33:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35172) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eBjPM-0002cS-7u for qemu-devel@nongnu.org; Mon, 06 Nov 2017 10:33:52 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5118A85A05; Mon, 6 Nov 2017 15:33:51 +0000 (UTC) Received: from localhost.localdomain.com (unknown [10.42.22.189]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3D36D7820A; Mon, 6 Nov 2017 15:33:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5118A85A05 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=berrange@redhat.com From: "Daniel P. Berrange" To: qemu-devel@nongnu.org Date: Mon, 6 Nov 2017 15:33:41 +0000 Message-Id: <20171106153342.24147-2-berrange@redhat.com> In-Reply-To: <20171106153342.24147-1-berrange@redhat.com> References: <20171106153342.24147-1-berrange@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 06 Nov 2017 15:33:51 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL v1 1/2] sockets: avoid leak of listen file descriptor 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+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" If we iterate over the full port range without successfully binding+listeni= ng on the socket, we'll try the next address, whereupon we overwrite the slist= en file descriptor variable without closing it. Rather than having two places where we open + close socket FDs on different iterations of nested for loops, re-arrange the code to always open+close within the same loop iteration. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrange --- util/qemu-sockets.c | 52 +++++++++++++++++++++++++++++--------------------= --- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index b47fb45885..8b75541ce4 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -207,7 +207,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr, char uaddr[INET6_ADDRSTRLEN+1]; char uport[33]; int rc, port_min, port_max, p; - int slisten =3D 0; + int slisten =3D -1; int saved_errno =3D 0; bool socket_created =3D false; Error *err =3D NULL; @@ -267,31 +267,42 @@ static int inet_listen_saddr(InetSocketAddress *saddr, uaddr,INET6_ADDRSTRLEN,uport,32, NI_NUMERICHOST | NI_NUMERICSERV); =20 - slisten =3D create_fast_reuse_socket(e); - if (slisten < 0) { - continue; - } - - socket_created =3D true; port_min =3D inet_getport(e); port_max =3D saddr->has_to ? saddr->to + port_offset : port_min; for (p =3D port_min; p <=3D port_max; p++) { inet_setport(e, p); - rc =3D try_bind(slisten, saddr, e); - if (rc) { - if (errno =3D=3D EADDRINUSE) { + + slisten =3D create_fast_reuse_socket(e); + if (slisten < 0) { + /* First time we expect we might fail to create the socket + * eg if 'e' has AF_INET6 but ipv6 kmod is not loaded. + * Later iterations should always succeed if first iterati= on + * worked though, so treat that as fatal. + */ + if (p =3D=3D port_min) { continue; } else { - error_setg_errno(errp, errno, "Failed to bind socket"); + error_setg_errno(errp, errno, + "Failed to recreate failed listening = socket"); goto listen_failed; } } - if (!listen(slisten, 1)) { - goto listen_ok; - } - if (errno !=3D EADDRINUSE) { - error_setg_errno(errp, errno, "Failed to listen on socket"= ); - goto listen_failed; + socket_created =3D true; + + rc =3D try_bind(slisten, saddr, e); + if (rc < 0) { + if (errno !=3D EADDRINUSE) { + error_setg_errno(errp, errno, "Failed to bind socket"); + goto listen_failed; + } + } else { + if (!listen(slisten, 1)) { + goto listen_ok; + } + if (errno !=3D EADDRINUSE) { + error_setg_errno(errp, errno, "Failed to listen on soc= ket"); + goto listen_failed; + } } /* Someone else managed to bind to the same port and beat us * to listen on it! Socket semantics does not allow us to @@ -299,12 +310,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr, * socket to allow bind attempts for subsequent ports: */ closesocket(slisten); - slisten =3D create_fast_reuse_socket(e); - if (slisten < 0) { - error_setg_errno(errp, errno, - "Failed to recreate failed listening sock= et"); - goto listen_failed; - } + slisten =3D -1; } } error_setg_errno(errp, errno, --=20 2.13.6 From nobody Thu May 2 02:48:00 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1509982630973656.7701807096836; Mon, 6 Nov 2017 07:37:10 -0800 (PST) Received: from localhost ([::1]:48716 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBjSX-0002er-6F for importer@patchew.org; Mon, 06 Nov 2017 10:37:09 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56836) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBjPU-0000fm-GD for qemu-devel@nongnu.org; Mon, 06 Nov 2017 10:34:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eBjPN-0002d6-Sw for qemu-devel@nongnu.org; Mon, 06 Nov 2017 10:34:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35204) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eBjPN-0002co-Jv for qemu-devel@nongnu.org; Mon, 06 Nov 2017 10:33:53 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8604C820F3; Mon, 6 Nov 2017 15:33:52 +0000 (UTC) Received: from localhost.localdomain.com (unknown [10.42.22.189]) by smtp.corp.redhat.com (Postfix) with ESMTP id 889E0708F6; Mon, 6 Nov 2017 15:33:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8604C820F3 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=berrange@redhat.com From: "Daniel P. Berrange" To: qemu-devel@nongnu.org Date: Mon, 6 Nov 2017 15:33:42 +0000 Message-Id: <20171106153342.24147-3-berrange@redhat.com> In-Reply-To: <20171106153342.24147-1-berrange@redhat.com> References: <20171106153342.24147-1-berrange@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 06 Nov 2017 15:33:52 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL v1 2/2] tests: Add test-listen - a stress test for QEMU socket listen 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 , Knut Omang Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" From: Knut Omang There's a potential race condition between multiple bind()'s attempting to bind to the same port, which occasionally allows more than one bind to succeed against the same port. When a subsequent listen() call is made with the same socket only one will succeed. The current QEMU code does however not take this situation into account and the listen will cause the code to break out and fail even when there are actually available ports to use. This test exposes two subtests: /socket/listen-serial /socket/listen-compete The "compete" subtest creates a number of threads and have them all trying = to bind to the same port with a large enough offset input to allow all threads to get it's own port. The "serial" subtest just does the same, except in series in a single thread. The serial version passes, probably in most versions of QEMU. The parallel version exposes the problem in a relatively reliable way, eg. it fails a majority of times, but not with a 100% rate, occasional passes can be seen. Nevertheless this is quite good given that the bug was tricky to reproduce and has been left undetected for a while. The problem seems to be present in all versions of QEMU. The original failure scenario occurred with VNC port allocation in a traditional Xen based build, in different code but with similar functionality. Reported-by: Bhavesh Davda Signed-off-by: Knut Omang Reviewed-by: Yuval Shaia Reviewed-by: Bhavesh Davda Reviewed-by: Girish Moodalbail Signed-off-by: Daniel P. Berrange --- tests/Makefile.include | 2 + tests/test-listen.c | 253 +++++++++++++++++++++++++++++++++++++++++++++= ++++ 2 files changed, 255 insertions(+) create mode 100644 tests/test-listen.c diff --git a/tests/Makefile.include b/tests/Makefile.include index 434a2ce868..e4bb88bd3d 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -154,6 +154,7 @@ gcov-files-check-bufferiszero-y =3D util/bufferiszero.c check-unit-y +=3D tests/test-uuid$(EXESUF) check-unit-y +=3D tests/ptimer-test$(EXESUF) gcov-files-ptimer-test-y =3D hw/core/ptimer.c +check-unit-y +=3D tests/test-listen$(EXESUF) check-unit-y +=3D tests/test-qapi-util$(EXESUF) gcov-files-test-qapi-util-y =3D qapi/qapi-util.c =20 @@ -804,6 +805,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer= .o tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y) tests/numa-test$(EXESUF): tests/numa-test.o tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o test= s/acpi-utils.o +tests/test-listen$(EXESUF): tests/test-listen.o $(test-util-obj-y) =20 tests/migration/stress$(EXESUF): tests/migration/stress.o $(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"L= INK","$(TARGET_DIR)$@") diff --git a/tests/test-listen.c b/tests/test-listen.c new file mode 100644 index 0000000000..03c4c8f03b --- /dev/null +++ b/tests/test-listen.c @@ -0,0 +1,253 @@ +/* + * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved. + * Author: Knut Omang + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 or later + * as published by the Free Software Foundation. + * + * Test parallel port listen configuration with + * dynamic port allocation + */ + +#include "qemu/osdep.h" +#include "libqtest.h" +#include "qemu-common.h" +#include "qemu/thread.h" +#include "qemu/sockets.h" +#include "qapi/error.h" + +#define NAME_LEN 1024 +#define PORT_LEN 16 + +struct thr_info { + QemuThread thread; + int to_port; + bool ipv4; + bool ipv6; + int got_port; + int eno; + int fd; + const char *errstr; + char hostname[NAME_LEN + 1]; + char port[PORT_LEN + 1]; +}; + + +/* These two functions taken from test-io-channel-socket.c */ +static int check_bind(const char *hostname, bool *has_proto) +{ + int fd =3D -1; + struct addrinfo ai, *res =3D NULL; + int rc; + int ret =3D -1; + + memset(&ai, 0, sizeof(ai)); + ai.ai_flags =3D AI_CANONNAME | AI_ADDRCONFIG; + ai.ai_family =3D AF_UNSPEC; + ai.ai_socktype =3D SOCK_STREAM; + + /* lookup */ + rc =3D getaddrinfo(hostname, NULL, &ai, &res); + if (rc !=3D 0) { + if (rc =3D=3D EAI_ADDRFAMILY || + rc =3D=3D EAI_FAMILY) { + *has_proto =3D false; + goto done; + } + goto cleanup; + } + + fd =3D qemu_socket(res->ai_family, res->ai_socktype, res->ai_protocol); + if (fd < 0) { + goto cleanup; + } + + if (bind(fd, res->ai_addr, res->ai_addrlen) < 0) { + if (errno =3D=3D EADDRNOTAVAIL) { + *has_proto =3D false; + goto done; + } + goto cleanup; + } + + *has_proto =3D true; + done: + ret =3D 0; + + cleanup: + if (fd !=3D -1) { + close(fd); + } + if (res) { + freeaddrinfo(res); + } + return ret; +} + +static int check_protocol_support(bool *has_ipv4, bool *has_ipv6) +{ + if (check_bind("127.0.0.1", has_ipv4) < 0) { + return -1; + } + if (check_bind("::1", has_ipv6) < 0) { + return -1; + } + + return 0; +} + +static void *listener_thread(void *arg) +{ + struct thr_info *thr =3D (struct thr_info *)arg; + SocketAddress addr =3D { + .type =3D SOCKET_ADDRESS_TYPE_INET, + .u =3D { + .inet =3D { + .host =3D thr->hostname, + .port =3D thr->port, + .has_ipv4 =3D thr->ipv4, + .ipv4 =3D thr->ipv4, + .has_ipv6 =3D thr->ipv6, + .ipv6 =3D thr->ipv6, + .has_to =3D true, + .to =3D thr->to_port, + }, + }, + }; + Error *err =3D NULL; + int fd; + + fd =3D socket_listen(&addr, &err); + if (fd < 0) { + thr->eno =3D errno; + thr->errstr =3D error_get_pretty(err); + } else { + struct sockaddr_in a; + socklen_t a_len =3D sizeof(a); + g_assert_cmpint(getsockname(fd, (struct sockaddr *)&a, &a_len), = =3D=3D, 0); + thr->got_port =3D ntohs(a.sin_port); + thr->fd =3D fd; + } + return arg; +} + + +static void listen_compete_nthr(bool threaded, int nthreads, + int start_port, int max_offset, + bool ipv4, bool ipv6) +{ + int i; + int failed_listens =3D 0; + struct thr_info *thr =3D g_new0(struct thr_info, nthreads); + int used[max_offset + 1]; + + memset(used, 0, sizeof(used)); + for (i =3D 0; i < nthreads; i++) { + snprintf(thr[i].port, PORT_LEN, "%d", start_port); + strcpy(thr[i].hostname, "localhost"); + thr[i].to_port =3D start_port + max_offset; + thr[i].ipv4 =3D ipv4; + thr[i].ipv6 =3D ipv6; + } + + for (i =3D 0; i < nthreads; i++) { + if (threaded) { + qemu_thread_create(&thr[i].thread, "listener", + listener_thread, &thr[i], + QEMU_THREAD_JOINABLE); + } else { + listener_thread(&thr[i]); + } + } + + if (threaded) { + for (i =3D 0; i < nthreads; i++) { + qemu_thread_join(&thr[i].thread); + } + } + for (i =3D 0; i < nthreads; i++) { + if (thr[i].got_port) { + closesocket(thr[i].fd); + } + } + + for (i =3D 0; i < nthreads; i++) { + if (thr[i].eno !=3D 0) { + const char *m; + g_printerr("** Failed to assign a port to thread %d (errno =3D= %d)\n", + i, thr[i].eno); + /* This is what we are interested in capturing - + * catch and report details if something unexpected happens: + */ + m =3D strstr(thr[i].errstr, "Failed to listen on socket"); + if (m !=3D NULL) { + g_assert_cmpstr(thr[i].errstr, =3D=3D, + "Failed to listen on socket: Address already in use"); + } + failed_listens++; + } else { + int assigned_port =3D thr[i].got_port; + g_assert_cmpint(assigned_port, <=3D , thr[i].to_port); + g_assert_cmpint(used[assigned_port - start_port], =3D=3D , 0); + } + } + g_assert_cmpint(failed_listens, =3D=3D, 0); + g_free(thr); +} + + +static void listen_compete_ipv4(void) +{ + listen_compete_nthr(true, 200, 5920, 300, true, false); +} + +static void listen_serial_ipv4(void) +{ + listen_compete_nthr(false, 200, 6300, 300, true, false); +} + +static void listen_compete_ipv6(void) +{ + listen_compete_nthr(true, 200, 5920, 300, true, false); +} + +static void listen_serial_ipv6(void) +{ + listen_compete_nthr(false, 200, 6300, 300, false, true); +} + +static void listen_compete_gen(void) +{ + listen_compete_nthr(true, 200, 5920, 300, true, true); +} + +static void listen_serial_gen(void) +{ + listen_compete_nthr(false, 200, 6300, 300, true, true); +} + + +int main(int argc, char **argv) +{ + bool has_ipv4, has_ipv6; + g_test_init(&argc, &argv, NULL); + + if (check_protocol_support(&has_ipv4, &has_ipv6) < 0) { + return 1; + } + + if (has_ipv4) { + g_test_add_func("/socket/listen-serial/ipv4", listen_serial_ipv4); + g_test_add_func("/socket/listen-compete/ipv4", listen_compete_ipv4= ); + } + if (has_ipv6) { + g_test_add_func("/socket/listen-serial/ipv6", listen_serial_ipv6); + g_test_add_func("/socket/listen-compete/ipv6", listen_compete_ipv6= ); + } + if (has_ipv4 && has_ipv6) { + g_test_add_func("/socket/listen-serial/generic", listen_serial_gen= ); + g_test_add_func("/socket/listen-compete/generic", listen_compete_g= en); + } + return g_test_run(); +} --=20 2.13.6