code review 5520057: net: Export SetReuseAddr on the underlying netFD for bo...

net: Export SetReuseAddr on the underlying netFD for both TCP and UDP sockets.

Fixes issue 1692.

[dho, 2012-01-06 22:16:07]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dho, 2012-01-07 01:39:07]

On 2012/01/06 22:16:07, dho wrote:
> Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

Actually, this change isn't good.

Because there was never any explanation as to why this was happening either in
Issue 1692 or in the mailing list thread at
http://groups.google.com/group/golang-nuts/browse_thread/thread/4cbbc28d5aa7ca52,
I'll go into it.

The netFD returned by calling file, _ := conn.(*net.UDPConn).File() explicitly
dup(2)s the descriptor and returns one that is explicitly a blocking socket.
There's a comment in fd.go:635 -- "We want blocking mode for the new fd, hence
the double negative" and it's explicitly set to block (this is documented in all
of the File() exports). So the code in the example is just incorrect.
Unfortunately, there's not a way to set SO_REUSEADDR on a TCPConn or UDPConn.
The patch referenced fixes that, and the call "setNoReuseAddress(conn)" would
simply change to conn.SetReuseAddr(false).

I think we must allow setting this for UDP sockets given the unicast / multicast
use case. I'm willing to get rid of the TCPConn export in the patch.

But it goes further: in both Linux and FreeBSD, it seems not well defined which
socket gets the data (tested with Issue 1692's code + nc -4u 10000). This opens
the door for a local user denial of service attack on UDP sockets. (I'd actually
vote that we don't turn on SO_REUSEADDR by default on UDP sockets for precisely
this reason, but still export SetReuseAddr so that those writing multicast
applications over UDP can use it.

FreeBSD also makes the assumption in UDP that once set, you never unset
SO_REUSEADDR (see comment in udp_usrreq.c); I'm not sure what Linux does here.

I'll submit an updated patch removing SO_REUSEADDR from being set on UDP sockets
by default (as I think we should given the two reasons above), but let me know
regarding exporting TCPConn.SetReuseAddr.

--dho

[mikio, 2012-01-07 08:32:20]

http://codereview.appspot.com/5520057/diff/1002/src/pkg/net/udpsock_posix.go
File src/pkg/net/udpsock_posix.go (right):

http://codereview.appspot.com/5520057/diff/1002/src/pkg/net/udpsock_posix.go#...
src/pkg/net/udpsock_posix.go:146: func (c *TCPConn) SetReuseAddr(reuse bool)
error {
s/TCP/UDP/

[dho, 2012-01-07 19:45:30]

Hello rsc@golang.org, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dho, 2012-01-07 19:46:18]

On 2012/01/07 19:45:30, dho wrote:
> Hello mailto:rsc@golang.org, mailto:mikioh.mikioh@gmail.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Argh, don't. I've forgotten how to add files to these changes. Gimme a sec,
here.

[dho, 2012-01-07 19:50:25]

Hello rsc@golang.org, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2012-01-08 02:51:09]

Hi,

I'm still not convinced whether we should have SetReuseAddr/Port
methods on UDPConn, TCPConn or not.

But even if UDP multicast is the case, I don't see why because
a) we can set TCP,UDPConn to transport protocol, platform
suitable settings in socket w/ setKernelSpecificSocketOptions,
b) other methods on ProtocolConn such as JoinGroup would be
a trigger for multicast settings as someone suggested to fix the
UDP multicast on Windows issue.

-- Mikio

On Sat, Jan 7, 2012 at 10:39 AM,  <devon.odell@gmail.com> wrote:
> On 2012/01/06 22:16:07, dho wrote:
>>
>> Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com),
>
>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>
>
> Actually, this change isn't good.
>
> Because there was never any explanation as to why this was happening
> either in Issue 1692 or in the mailing list thread at
>
http://groups.google.com/group/golang-nuts/browse_thread/thread/4cbbc28d5aa7ca52,
> I'll go into it.
>
> The netFD returned by calling file, _ := conn.(*net.UDPConn).File()
> explicitly dup(2)s the descriptor and returns one that is explicitly a
> blocking socket. There's a comment in fd.go:635 -- "We want blocking
> mode for the new fd, hence the double negative" and it's explicitly set
> to block (this is documented in all of the File() exports). So the code
> in the example is just incorrect. Unfortunately, there's not a way to
> set SO_REUSEADDR on a TCPConn or UDPConn. The patch referenced fixes
> that, and the call "setNoReuseAddress(conn)" would simply change to
> conn.SetReuseAddr(false).
>
> I think we must allow setting this for UDP sockets given the unicast /
> multicast use case. I'm willing to get rid of the TCPConn export in the
> patch.
>
> But it goes further: in both Linux and FreeBSD, it seems not well
> defined which socket gets the data (tested with Issue 1692's code + nc
> -4u 10000). This opens the door for a local user denial of service
> attack on UDP sockets. (I'd actually vote that we don't turn on
> SO_REUSEADDR by default on UDP sockets for precisely this reason, but
> still export SetReuseAddr so that those writing multicast applications
> over UDP can use it.
>
> FreeBSD also makes the assumption in UDP that once set, you never unset
> SO_REUSEADDR (see comment in udp_usrreq.c); I'm not sure what Linux does
> here.
>
> I'll submit an updated patch removing SO_REUSEADDR from being set on UDP
> sockets by default (as I think we should given the two reasons above),
> but let me know regarding exporting TCPConn.SetReuseAddr.
>
> --dho
>
> http://codereview.appspot.com/5520057/

[dho, 2012-01-08 03:08:54]

2012/1/7 Mikio Hara <mikioh.mikioh@gmail.com>:
> Hi,
>
> I'm still not convinced whether we should have SetReuseAddr/Port
> methods on UDPConn, TCPConn or not.

I detail below why it should be allowed for TCP, but I don't care so
much how it works for UDP as long as they're not set for UDP by
default on at least Linux and BSD.

> But even if UDP multicast is the case, I don't see why because
> a) we can set TCP,UDPConn to transport protocol, platform
> suitable settings in socket w/ setKernelSpecificSocketOptions,

The new code only sets SO_REUSE(ADDR|PORT) on TCP sockets in
setKernelSpecificSocketOptions under Linux and BSD. Windows doesn't
appear to set it at all. I really don't think these should be set on
UDP sockets because the behavior of both in the common case is at
least unexpected, if not undesirable for the reasons I mentioned in my
previous comment (poorly defined behavior for unicast packets over
UDP, and the fact that, in FreeBSD at least, the kernel effectively
ignores unsetting these sockopts after they're set).

> b) other methods on ProtocolConn such as JoinGroup would be
> a trigger for multicast settings as someone suggested to fix the
> UDP multicast on Windows issue.

I'm not really familiar with multicast in Go (or sockets on Windows in
general), but I'm happy to make JoinGroup set SO_REUSEPORT (on BSD)
and SO_REUSEADDR (on BSD and Linux) for UDP. I don't see any
compelling argument to turn them off by default for TCP, but there is
a compelling argument to export them for TCP: namely, binding to a
port on inaddr_any or inaddr6_any with SO_REUSEADDR set allows another
application to bind to the same port on a more specific address. Not
being able to turn off this behavior can create additional local user
denial of service conditions on ports >1024 (which is a *ton* of
services these days).

It'd definitely be useful to get some input on how these things work
on Windows, but I think there's a pretty good case to export them and
turn them off by default for UDP on Linux / BSD.

--dho

> -- Mikio
>
> On Sat, Jan 7, 2012 at 10:39 AM,  <devon.odell@gmail.com> wrote:
>> On 2012/01/06 22:16:07, dho wrote:
>>>
>>> Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com),
>>
>>
>>> I'd like you to review this change to
>>> https://go.googlecode.com/hg/
>>
>>
>> Actually, this change isn't good.
>>
>> Because there was never any explanation as to why this was happening
>> either in Issue 1692 or in the mailing list thread at
>>
http://groups.google.com/group/golang-nuts/browse_thread/thread/4cbbc28d5aa7ca52,
>> I'll go into it.
>>
>> The netFD returned by calling file, _ := conn.(*net.UDPConn).File()
>> explicitly dup(2)s the descriptor and returns one that is explicitly a
>> blocking socket. There's a comment in fd.go:635 -- "We want blocking
>> mode for the new fd, hence the double negative" and it's explicitly set
>> to block (this is documented in all of the File() exports). So the code
>> in the example is just incorrect. Unfortunately, there's not a way to
>> set SO_REUSEADDR on a TCPConn or UDPConn. The patch referenced fixes
>> that, and the call "setNoReuseAddress(conn)" would simply change to
>> conn.SetReuseAddr(false).
>>
>> I think we must allow setting this for UDP sockets given the unicast /
>> multicast use case. I'm willing to get rid of the TCPConn export in the
>> patch.
>>
>> But it goes further: in both Linux and FreeBSD, it seems not well
>> defined which socket gets the data (tested with Issue 1692's code + nc
>> -4u 10000). This opens the door for a local user denial of service
>> attack on UDP sockets. (I'd actually vote that we don't turn on
>> SO_REUSEADDR by default on UDP sockets for precisely this reason, but
>> still export SetReuseAddr so that those writing multicast applications
>> over UDP can use it.
>>
>> FreeBSD also makes the assumption in UDP that once set, you never unset
>> SO_REUSEADDR (see comment in udp_usrreq.c); I'm not sure what Linux does
>> here.
>>
>> I'll submit an updated patch removing SO_REUSEADDR from being set on UDP
>> sockets by default (as I think we should given the two reasons above),
>> but let me know regarding exporting TCPConn.SetReuseAddr.
>>
>> --dho
>>
>> http://codereview.appspot.com/5520057/

[mikio, 2012-01-08 13:10:07]

Thank you for the details, now I can see your point.

On Sun, Jan 8, 2012 at 12:08 PM, Devon H. O'Dell <devon.odell@gmail.com> wrote:

> I don't see any
> compelling argument to turn them off by default for TCP, but there is
> a compelling argument to export them for TCP: namely, binding to a
> port on inaddr_any or inaddr6_any with SO_REUSEADDR set allows another
> application to bind to the same port on a more specific address. Not
> being able to turn off this behavior can create additional local user
> denial of service conditions on ports >1024 (which is a *ton* of
> services these days).

Sounds like we need a new test case, so please add it
especially you described above.

> It'd definitely be useful to get some input on how these things work
> on Windows, but I think there's a pretty good case to export them and
> turn them off by default for UDP on Linux / BSD.

SO_REUSEADDR in Windows
<http://groups.google.com/group/golang-dev/browse_thread/thread/b71aa6f44a1118...>

Also I guess you can poke Paul Lalonde. ;-)

[mikio, 2012-01-08 14:08:31]

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/Makefile
File src/pkg/net/Makefile (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/Makefile#newcode42
src/pkg/net/Makefile:42: udpsock_bsd.go\
pls move up before udpsock_posix.go.

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/Makefile#newcode71
src/pkg/net/Makefile:71: udpsock_bsd.go\
same

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/Makefile#newcode127
src/pkg/net/Makefile:127: udpsock_bsd.go\
ditto

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/Makefile#newcode149
src/pkg/net/Makefile:149: udpsock_bsd.go\
same

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/sock.go
File src/pkg/net/sock.go (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/sock.go#newcode38
src/pkg/net/sock.go:38: setKernelSpecificSockopt(s, f, t)
pls rename this guy because this guy will handle not only 
kernel specific socket options but also other socket options 
to make apps transport protocol adaptive.

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/sock_bsd.go
File src/pkg/net/sock_bsd.go (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/sock_bsd.go#newco...
src/pkg/net/sock_bsd.go:18: if (f == syscall.AF_INET || f == syscall.AF_INET6)
&& t == syscall.SOCK_STREAM {
switch t { ... }
and what about AF_UNIX? can we ignore that address family?

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/sock_linux.go
File src/pkg/net/sock_linux.go (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/sock_linux.go#new...
src/pkg/net/sock_linux.go:15: if (f == syscall.AF_INET || f == syscall.AF_INET6)
&& t == syscall.SOCK_STREAM {
same as comment on BSD.

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/tcpsock_posix.go
File src/pkg/net/tcpsock_posix.go (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/tcpsock_posix.go#...
src/pkg/net/tcpsock_posix.go:223: return setReuseAddr(c.fd, reuse)
I suspect that this makes different behavior btw BSD 
variants and Linux, Windows because on Linux, perhaps 
also Windows SO_REUSEADDR works almost same as a combo 
of SO_REUSEADDR and SO_REUSEPORT on BSD variants.
Yup, we need a test case.

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/udpsock_bsd.go
File src/pkg/net/udpsock_bsd.go (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/udpsock_bsd.go#ne...
src/pkg/net/udpsock_bsd.go:18: func (c *UDPConn) SetReusePort(reuse bool) error
{
not sure we could do without a stub, placeholder 
for other OSes. I think we should avoid to introduce 
platform-dependent public API to standard pkgs.

can we consolidate both REUSEADDR and REUSEPORT 
into one API?

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/udpsock_posix.go
File src/pkg/net/udpsock_posix.go (right):

http://codereview.appspot.com/5520057/diff/9001/src/pkg/net/udpsock_posix.go#...
src/pkg/net/udpsock_posix.go:150: return setReuseAddr(c.fd, reuse)
same as comment on TCP.

[rsc, 2012-01-09 18:22:33]

Since SO_REUSEADDR has such wildly different meanings
on Windows and on Unix, I think exposing this in the public
net API is a mistake.

Why are we doing this?  What isn't doing the right thing?
Can we make it do the right thing?

Russ

[mikio, 2012-01-12 16:42:26]

Hi,

On Tue, Jan 10, 2012 at 3:22 AM, Russ Cox <rsc@golang.org> wrote:

> Since SO_REUSEADDR has such wildly different meanings
> on Windows and on Unix, I think exposing this in the public
> net API is a mistake.

Probably you are right but I'd like to hear a bit more Devon's
use case for TCP.

> Why are we doing this?  What isn't doing the right thing?
> Can we make it do the right thing?

To make things smoother I've just sent a separate CL to fix
default socket options stuff.

[dho, 2012-01-12 17:06:16]

I like your CL better than this one.

2012/1/12 Mikio Hara <mikioh.mikioh@gmail.com>:
> Hi,
>
> On Tue, Jan 10, 2012 at 3:22 AM, Russ Cox <rsc@golang.org> wrote:
>
>> Since SO_REUSEADDR has such wildly different meanings
>> on Windows and on Unix, I think exposing this in the public
>> net API is a mistake.
>
> Probably you are right but I'd like to hear a bit more Devon's
> use case for TCP.
>
>> Why are we doing this?  What isn't doing the right thing?
>> Can we make it do the right thing?
>
> To make things smoother I've just sent a separate CL to fix
> default socket options stuff.

[rsc, 2012-01-13 17:53:41]

Now that we have Mikio's CL, we don't need to
export the ability to toggle SO_REUSEADDR directly
anymore, right?

[dho, 2012-01-19 22:19:40]

*** Abandoned ***