[Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
the default behavior is equivalent to SO_REUSEADDR on other operating
systems. SO_REUSEADDR can still be set but results in undesired bahvior
instead. It may even lead to situations were system behavior is
unspecified. More information on this can be found at:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx

I originally encountered this issue when accidentally launching two QEMU
instances with identical GDB ports at the same time. In which case QEMU won't
fail as one might expect. I am sending this as RFC as I A) only checked that
this fixes issues for the GDB server and B) am not sure if this is the correct
format for this patchset.

gdbstub: do not set SO_REUSEADDR on Windows
net: do not set SO_REUSEADDR on Windows
slirp: do not set SO_REUSEADDR on Windows
util: do not set SO_REUSEADDR on Windows

gdbstub.c           |    2 ++
net/socket.c        |    6 ++++++
slirp/misc.c        |    2 ++
slirp/tcp_subr.c    |    4 ++++
slirp/udp.c         |    2 ++
util/qemu-sockets.c |    7 ++++++-
6 files changed, 22 insertions(+), 1 deletion(-)

[Qemu-devel] [PATCH RFC 1/4] gdbstub: do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

If a socket is closed it may remain in TIME_WAIT state for some time. On most
operating systems the local port of the connection or socket may not be reeused
while in this state unless SO_REUSEADDR was set on the socket. On windows on the
other hand the default behaviour is to allow reuse (i.e. identical to
SO_REUSEADDR on other operating systems) and setting SO_REUSEADDR on a socket
allows it to be bound to a endpoint even if the endpoint is already used by
another socket independently of the other sockets state. This may result in
undefined behaviour. Fix this issue by no setting SO_REUSEADDR on windows.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 gdbstub.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 2b7f22b..3d60acf 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1565,8 +1565,10 @@ static int gdbserver_open(int port)
 #endif
 
     /* allow fast reuse */
+#ifndef _WIN32
     val = 1;
     qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
+#endif
 
     sockaddr.sin_family = AF_INET;
     sockaddr.sin_port = htons(port);
-- 
1.7.9.5

[Qemu-devel] [PATCH RFC 2/4] net: do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

If a socket is closed it may remain in TIME_WAIT state for some time. On most
operating systems the local port of the connection or socket may not be reeused
while in this state unless SO_REUSEADDR was set on the socket. On windows on the
other hand the default behaviour is to allow reuse (i.e. identical to
SO_REUSEADDR on other operating systems) and setting SO_REUSEADDR on a socket
allows it to be bound to a endpoint even if the endpoint is already used by
another socket independently of the other sockets state. This may result in
undefined behaviour. Fix this issue by no setting SO_REUSEADDR on windows.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 net/socket.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index e61309d..f44ebcb 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -262,12 +262,14 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         return -1;
     }
 
+#ifndef _WIN32
     val = 1;
     ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
     if (ret < 0) {
         perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
         goto fail;
     }
+#endif
 
     ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
     if (ret < 0) {
@@ -523,8 +525,10 @@ static int net_socket_listen_init(NetClientState *peer,
     qemu_set_nonblock(fd);
 
     /* allow fast reuse */
+#ifndef _WIN32
     val = 1;
     qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
+#endif
 
     ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
@@ -661,6 +665,7 @@ static int net_socket_udp_init(NetClientState *peer,
         perror("socket(PF_INET, SOCK_DGRAM)");
         return -1;
     }
+#ifndef _WIN32
     val = 1;
     ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
                           &val, sizeof(val));
@@ -669,6 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
         closesocket(fd);
         return -1;
     }
+#endif
     ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
     if (ret < 0) {
         perror("bind");
-- 
1.7.9.5

[Qemu-devel] [PATCH RFC 3/4] slirp: do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

If a socket is closed it may remain in TIME_WAIT state for some time. On most
operating systems the local port of the connection or socket may not be reeused
while in this state unless SO_REUSEADDR was set on the socket. On windows on the
other hand the default behaviour is to allow reuse (i.e. identical to
SO_REUSEADDR on other operating systems) and setting SO_REUSEADDR on a socket
allows it to be bound to a endpoint even if the endpoint is already used by
another socket independently of the other sockets state. This may result in
undefined behaviour. Fix this issue by no setting SO_REUSEADDR on windows.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 slirp/misc.c     |    2 ++
 slirp/tcp_subr.c |    4 ++++
 slirp/udp.c      |    2 ++
 3 files changed, 8 insertions(+)

diff --git a/slirp/misc.c b/slirp/misc.c
index c0d4899..fe9c1c3 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -212,8 +212,10 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                     so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
                 } while (so->s < 0 && errno == EINTR);
                 closesocket(s);
+#ifndef _WIN32
                 opt = 1;
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
+#endif
                 opt = 1;
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
 		qemu_set_nonblock(so->s);
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 043f28f..148d513 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -337,8 +337,10 @@ int tcp_fconnect(struct socket *so)
     struct sockaddr_in addr;
 
     qemu_set_nonblock(s);
+#ifndef _WIN32
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+#endif
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(opt));
 
@@ -426,8 +428,10 @@ void tcp_connect(struct socket *inso)
         return;
     }
     qemu_set_nonblock(s);
+#ifndef _WIN32
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
+#endif
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
     socket_set_nodelay(s);
diff --git a/slirp/udp.c b/slirp/udp.c
index b105f87..6ee45d6 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -372,7 +372,9 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 		udp_detach(so);
 		return NULL;
 	}
+#ifndef _WIN32
 	qemu_setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
+#endif
 
 	getsockname(so->s,(struct sockaddr *)&addr,&addrlen);
 	so->so_fport = addr.sin_port;
-- 
1.7.9.5

[Qemu-devel] [PATCH RFC 4/4] util: do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

If a socket is closed it may remain in TIME_WAIT state for some time. On most
operating systems the local port of the connection or socket may not be reeused
while in this state unless SO_REUSEADDR was set on the socket. On windows on the
other hand the default behaviour is to allow reuse (i.e. identical to
SO_REUSEADDR on other operating systems) and setting SO_REUSEADDR on a socket
allows it to be bound to a endpoint even if the endpoint is already used by
another socket independently of the other sockets state. This may result in
undefined behaviour. Fix this issue by no setting SO_REUSEADDR on windows.

Signed-off-by: Sebastian Ottlik <ottlik@fzi.de>
---
 util/qemu-sockets.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 095716e..b5ea66a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -154,8 +154,9 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
             }
             continue;
         }
-
+#ifndef _WIN32
         qemu_setsockopt(slisten, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+#endif
 #ifdef IPV6_V6ONLY
         if (e->ai_family == PF_INET6) {
             /* listen on both ipv4 and ipv6 */
@@ -274,7 +275,9 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
         error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
+#ifndef _WIN32
     qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+#endif
     if (connect_state != NULL) {
         qemu_set_nonblock(sock);
     }
@@ -455,7 +458,9 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
         error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         goto err;
     }
+#ifndef _WIN32
     qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+#endif
 
     /* bind socket */
     if (bind(sock, local->ai_addr, local->ai_addrlen) < 0) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Paolo Bonzini]

Il 04/09/2013 16:22, Sebastian Ottlik ha scritto:
> This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
> the default behavior is equivalent to SO_REUSEADDR on other operating
> systems. SO_REUSEADDR can still be set but results in undesired bahvior
> instead. It may even lead to situations were system behavior is
> unspecified. More information on this can be found at:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx
> 
> I originally encountered this issue when accidentally launching two QEMU
> instances with identical GDB ports at the same time. In which case QEMU won't
> fail as one might expect. I am sending this as RFC as I A) only checked that
> this fixes issues for the GDB server and B) am not sure if this is the correct
> format for this patchset.
> 
> gdbstub: do not set SO_REUSEADDR on Windows
> net: do not set SO_REUSEADDR on Windows
> slirp: do not set SO_REUSEADDR on Windows
> util: do not set SO_REUSEADDR on Windows

Makes sense.

Can you make a different patch that introduces a new function
qemu_set_reuseaddr is include/qemu/sockets.h & util/oslib-*, and makes
it a stub for Windows?

This way we don't have a proliferation of #ifs.

Paolo

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Jan Kiszka]

On 2013-09-04 16:27, Paolo Bonzini wrote:
> Il 04/09/2013 16:22, Sebastian Ottlik ha scritto:
>> This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
>> the default behavior is equivalent to SO_REUSEADDR on other operating
>> systems. SO_REUSEADDR can still be set but results in undesired bahvior
>> instead. It may even lead to situations were system behavior is
>> unspecified. More information on this can be found at:
>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx
>>
>> I originally encountered this issue when accidentally launching two QEMU
>> instances with identical GDB ports at the same time. In which case QEMU won't
>> fail as one might expect. I am sending this as RFC as I A) only checked that
>> this fixes issues for the GDB server and B) am not sure if this is the correct
>> format for this patchset.
>>
>> gdbstub: do not set SO_REUSEADDR on Windows
>> net: do not set SO_REUSEADDR on Windows
>> slirp: do not set SO_REUSEADDR on Windows
>> util: do not set SO_REUSEADDR on Windows
> 
> Makes sense.
> 
> Can you make a different patch that introduces a new function
> qemu_set_reuseaddr is include/qemu/sockets.h & util/oslib-*, and makes
> it a stub for Windows?
> 
> This way we don't have a proliferation of #ifs.

Yeah, this is definitely better then. IIRC, Michael has some version of
patch 3 in his queue right now - should be dropped in this light.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Sebastian Ottlik]

On 04.09.2013 16:27, Paolo Bonzini wrote:
> Il 04/09/2013 16:22, Sebastian Ottlik ha scritto:
>> This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
>> the default behavior is equivalent to SO_REUSEADDR on other operating
>> systems. SO_REUSEADDR can still be set but results in undesired bahvior
>> instead. It may even lead to situations were system behavior is
>> unspecified. More information on this can be found at:
>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx
>>
>> I originally encountered this issue when accidentally launching two QEMU
>> instances with identical GDB ports at the same time. In which case QEMU won't
>> fail as one might expect. I am sending this as RFC as I A) only checked that
>> this fixes issues for the GDB server and B) am not sure if this is the correct
>> format for this patchset.
>>
>> gdbstub: do not set SO_REUSEADDR on Windows
>> net: do not set SO_REUSEADDR on Windows
>> slirp: do not set SO_REUSEADDR on Windows
>> util: do not set SO_REUSEADDR on Windows
> Makes sense.
>
> Can you make a different patch that introduces a new function
> qemu_set_reuseaddr is include/qemu/sockets.h & util/oslib-*, and makes
> it a stub for Windows?
>
> This way we don't have a proliferation of #ifs.
>
> Paolo
Sounds like a much cleaner approach. I will wait to see if there is more 
feedback and then submit a new version including your suggestion.

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Peter Maydell]

On 4 September 2013 15:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/09/2013 16:22, Sebastian Ottlik ha scritto:
>> This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
>> the default behavior is equivalent to SO_REUSEADDR on other operating
>> systems. SO_REUSEADDR can still be set but results in undesired bahvior
>> instead. It may even lead to situations were system behavior is
>> unspecified. More information on this can be found at:
>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx

Yep. The issue's come up before:
 https://lists.nongnu.org/archive/html/qemu-devel/2011-03/msg01794.html
but I guess nobody ever got round to writing the patch.

> Can you make a different patch that introduces a new function
> qemu_set_reuseaddr is include/qemu/sockets.h & util/oslib-*, and makes
> it a stub for Windows?

Yes please, and include a comment in the Windows stub
explaining why it does nothing (with the links to MSDN docs)
because otherwise it's pretty unobvious and an invitation
for somebody to incorrectly reinstate the brokenness.

-- PMM

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Stefan Weil]

Am 04.09.2013 16:41, schrieb Peter Maydell:
> On 4 September 2013 15:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 04/09/2013 16:22, Sebastian Ottlik ha scritto:
>>> This patchset disabels all use of SO_REUSEADDR on Windows. On
>>> Windows systems the default behavior is equivalent to SO_REUSEADDR
>>> on other operating systems. SO_REUSEADDR can still be set but
>>> results in undesired bahvior instead. It may even lead to situations
>>> were system behavior is unspecified. More information on this can be
>>> found at:
>>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx 
> Yep. The issue's come up before:
> https://lists.nongnu.org/archive/html/qemu-devel/2011-03/msg01794.html
> but I guess nobody ever got round to writing the patch.
>> Can you make a different patch that introduces a new function
>> qemu_set_reuseaddr is include/qemu/sockets.h & util/oslib-*, and
>> makes it a stub for Windows? 
> Yes please, and include a comment in the Windows stub explaining why
> it does nothing (with the links to MSDN docs) because otherwise it's
> pretty unobvious and an invitation for somebody to incorrectly
> reinstate the brokenness. -- PMM 

May I suggest a slightly different approach? Instead of a new function
qemu_set_reuseaddr I'd prefer extending the existing qemu_setsockopt.
These steps / patches are required:

1. Move *sock* lines from include/qemu-common.h to include/qemu/sockets.h
   and addqemu/sockets.h to the include statements in bt-host.c and
   linux-user/syscall.c (I hope this list is complete).

   This step is needed because we don't want to add socket includes to
   qemu-common.h (which would be needed when we add a function prototype
   for qemu_setsockopt).

2. Replace the Win32 defines for qemu_getsockopt, qemu_setsockopt by
function
   prototypes (see Linux getsockopt, setsockopt for reference) and
   implement both functions in util/oslib-win32.c. Ignore SO_REUSEADDR in
   qemu_setsockopt.

   Using these two functions allows easy implementation of OS specific hacks
   (we might need more in the future).

3. Revert commit efcb7e45290ecc8633f7c5bdf02ac86f6289fa7d. It is no longer
   needed after patch 2.

Regards,
Stefan

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Paolo Bonzini]

Il 04/09/2013 19:31, Stefan Weil ha scritto:
> May I suggest a slightly different approach? Instead of a new function
> qemu_set_reuseaddr I'd prefer extending the existing qemu_setsockopt.
> These steps / patches are required:
> 
> 1. Move *sock* lines from include/qemu-common.h to include/qemu/sockets.h
>    and addqemu/sockets.h to the include statements in bt-host.c and
>    linux-user/syscall.c (I hope this list is complete).
> 
>    This step is needed because we don't want to add socket includes to
>    qemu-common.h (which would be needed when we add a function prototype
>    for qemu_setsockopt).
> 
> 2. Replace the Win32 defines for qemu_getsockopt, qemu_setsockopt by
> function
>    prototypes (see Linux getsockopt, setsockopt for reference) and
>    implement both functions in util/oslib-win32.c. Ignore SO_REUSEADDR in
>    qemu_setsockopt.
> 
>    Using these two functions allows easy implementation of OS specific hacks
>    (we might need more in the future).
> 
> 3. Revert commit efcb7e45290ecc8633f7c5bdf02ac86f6289fa7d. It is no longer
>    needed after patch 2.

Yeah, that's also an interesting way to do it!

Paolo

Re: [Qemu-devel] [PATCH RFC] Do not set SO_REUSEADDR on Windows

[Michael Tokarev]

04.09.2013 18:34, Jan Kiszka wrote:
> On 2013-09-04 16:27, Paolo Bonzini wrote:
>> Il 04/09/2013 16:22, Sebastian Ottlik ha scritto:
>>> This patchset disabels all use of SO_REUSEADDR on Windows. On Windows systems
>>> the default behavior is equivalent to SO_REUSEADDR on other operating
>>> systems. SO_REUSEADDR can still be set but results in undesired bahvior
>>> instead. It may even lead to situations were system behavior is
>>> unspecified. More information on this can be found at:
>>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx
>>>
>>> I originally encountered this issue when accidentally launching two QEMU
>>> instances with identical GDB ports at the same time. In which case QEMU won't
>>> fail as one might expect. I am sending this as RFC as I A) only checked that
>>> this fixes issues for the GDB server and B) am not sure if this is the correct
>>> format for this patchset.
>>>
>>> gdbstub: do not set SO_REUSEADDR on Windows
>>> net: do not set SO_REUSEADDR on Windows
>>> slirp: do not set SO_REUSEADDR on Windows
>>> util: do not set SO_REUSEADDR on Windows
>>
>> Makes sense.
>>
>> Can you make a different patch that introduces a new function
>> qemu_set_reuseaddr is include/qemu/sockets.h & util/oslib-*, and makes
>> it a stub for Windows?

Maybe a better approach will be to introduce something like
qemu_[stream_]listen_socket(int family, sockaddr_t *sa, int salen) function
which opens and prepares a socket with all necessary platform-dependent
stuff?

> Yeah, this is definitely better then. IIRC, Michael has some version of
> patch 3 in his queue right now - should be dropped in this light.

It's been pulled by Anthony yesterday so it is a bit too late now.
But the change is trivial, and can be reworked easily.

Thanks,

/mjt