From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH qemu-traditional] qemu: Fix race condition when binding ports Date: Mon, 9 Dec 2013 13:17:26 +0000 Message-ID: <52A5C2E6.3080005@eu.citrix.com> References: <1386594652-6953-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1386594652-6953-1-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Xen-devel Cc: Stefano Stabellini , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 12/09/2013 01:10 PM, Andrew Cooper wrote: > Two Qemus can race to bind the same VNC port. It is valid for multiple > listen()s on the same socket to succeed, but only the first bind() will > succeed. > > In the case that two Qemus are racing, the second one will fail with an > EADDRINUSE, and bail with a fatal error which renders the domain functionally > useless. > > In the case that bind() fails with EADDRINUSE, rebind the socket again and try > for the next port. Sorry if I'm being a bit dense here, but it looks like you have those mixed up: if listen() fails with EADDRINUSE, you're going back to call bind() again; which would suggest that you can have multible bind()s but only a single listen()? Or am I confused? In any case, this is clearly a bug fix, and a very well tested one at that, so: Release-acked-by: George Dunlap > > Signed-off-by: Andrew Cooper > CC: Ian Campbell > CC: Ian Jackson > CC: Stefano Stabellini > CC: George Dunlap > > --- > > This fix has been in XenServer for two and a half years > --- > qemu-sockets.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 9b9fa77..e54f6ad 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -163,6 +163,7 @@ int inet_listen(const char *str, char *ostr, int olen, > > /* create socket + bind */ > for (e = res; e != NULL; e = e->ai_next) { > + rebind: > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > uaddr,INET6_ADDRSTRLEN,uport,32, > NI_NUMERICHOST | NI_NUMERICSERV); > @@ -186,7 +187,20 @@ int inet_listen(const char *str, char *ostr, int olen, > if (sockets_debug) > fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__, > inet_strfamily(e->ai_family), uaddr, inet_getport(e)); > - goto listen; > + if (listen(slisten,1) == 0) { > + goto listened; > + } else { > + int err = errno; > + > + perror("listen"); > + closesocket(slisten); > + > + if (err == EADDRINUSE) > + goto rebind; > + freeaddrinfo(res); > + return -1; > + } > + > } > try_next = to && (inet_getport(e) <= to + port_offset); > if (!try_next || sockets_debug) > @@ -205,12 +219,7 @@ int inet_listen(const char *str, char *ostr, int olen, > freeaddrinfo(res); > return -1; > > -listen: > - if (listen(slisten,1) != 0) { > - perror("listen"); > - closesocket(slisten); > - return -1; > - } > +listened: > if (ostr) { > if (e->ai_family == PF_INET6) { > snprintf(ostr, olen, "[%s]:%d%s", uaddr,