code review 7058062: syscall: handle empty address in ReadFrom better

syscall: handle empty address in ReadFrom better

Handle return values from recvfrom correctly when the
kernel decides to not return an address.

Fixes issue 4636.
Fixes issue 4352.

[jeff.allen, 2013-01-09 12:46:10]

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

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2013-01-09 12:48:53]

We have to be careful to respect the existing go 1.0 behaviour. I don't
know if this change affects that contract, but it is important to check.
Can you please patch your test cases into go 1.0.3 and see what happens. We
may have to add additional passing tests to prevent breaking go 1.0
contracts in the future.
On 9 Jan 2013 23:46, <jeff.allen@gmail.com> wrote:

> Reviewers: rsc, mikio,
>
> Message:
> Hello rsc@golang.org, mikioh.mikioh@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: fix anonymous unixgram sockets
>
> Make it possible to call net.DialUnix("unixgram", nil, nil)
> and then make receives on anonymous sockets work correctly.
>
> Fixes issue 4636.
>
> Please review this at
https://codereview.appspot.**com/7058062/<https://codereview.appspot.com/7058...
>
> Affected files:
>   M src/pkg/net/packetconn_test.go
>   M src/pkg/net/unixsock_posix.go
>   M src/pkg/syscall/syscall_bsd.go
>   M src/pkg/syscall/syscall_linux.**go
>
>
> Index: src/pkg/net/packetconn_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/packetconn_test.**go
> +++ b/src/pkg/net/packetconn_test.**go
> @@ -98,6 +98,39 @@
>         }
>  }
>
> +func TestAnonymousReceive(t *testing.T) {
> +       addr := "/tmp/sun"
> +       go func() {
> +               time.Sleep(10 * time.Millisecond)
> +               c, err := net.DialUnix("unixgram", nil, nil)
> +               if err != nil {
> +                       return
> +               }
> +               one := [1]byte{1}
> +               c.WriteTo(one[:], &net.UnixAddr{addr, "unixgram"})
> +       }()
> +       os.Remove(addr)
> +       c, err := net.ListenPacket("unixgram", addr)
> +       if err != nil {
> +               t.Error(err)
> +       }
> +       var buf [500]byte
> +       c.SetReadDeadline(time.Now().**Add(100 * time.Millisecond))
> +       n, from, err := c.ReadFrom(buf[:])
> +       if err != nil {
> +               t.Error(err)
> +       }
> +       if n != 1 {
> +               t.Error("n is", n)
> +       }
> +       if from != nil {
> +               t.Error("from is", from)
> +       }
> +       if buf[0] != 1 {
> +               t.Error("buf[0] is wrong")
> +       }
> +}
> +
>  func TestConnAndPacketConn(t *testing.T) {
>         for _, tt := range packetConnTests {
>                 var wb []byte
> Index: src/pkg/net/unixsock_posix.go
> ==============================**==============================**=======
> --- a/src/pkg/net/unixsock_posix.**go
> +++ b/src/pkg/net/unixsock_posix.**go
> @@ -36,7 +36,7 @@
>                 }
>                 if raddr != nil {
>                         ra = &syscall.SockaddrUnix{Name: raddr.Name}
> -               } else if sotype != syscall.SOCK_DGRAM || laddr == nil {
> +               } else if sotype != syscall.SOCK_DGRAM {
>                         return nil, &OpError{Op: mode, Net: net, Err:
> errMissingAddress}
>                 }
>         case "listen":
> Index: src/pkg/syscall/syscall_bsd.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_bsd.**go
> +++ b/src/pkg/syscall/syscall_bsd.**go
> @@ -450,7 +450,9 @@
>         if n, err = recvfrom(fd, p, flags, &rsa, &len); err != nil {
>                 return
>         }
> -       from, err = anyToSockaddr(&rsa)
> +       if n > 0 {
> +               from, err = anyToSockaddr(&rsa)
> +       }
>         return
>  }
>
> Index: src/pkg/syscall/syscall_linux.**go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**linux.go
> +++ b/src/pkg/syscall/syscall_**linux.go
> @@ -547,7 +547,9 @@
>         if n, err = recvfrom(fd, p, flags, &rsa, &len); err != nil {
>                 return
>         }
> -       from, err = anyToSockaddr(&rsa)
> +       if len > 0 {
> +               from, err = anyToSockaddr(&rsa)
> +       }
>         return
>  }
>
>
>
>

[mikio, 2013-01-09 13:50:36]

Thank you for your investigations.

I guess we need to have a look at not only issue 4636 but issue 4352.
Also please add Read/Write method tests to protoconn_test.go.

[mikio, 2013-01-09 14:14:33]

I'm not sure whether we should fix DialUnix for accepting unnamed
sockets. But if you really need to extend it, please update doc too.

On Wed, Jan 9, 2013 at 10:50 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> Thank you for your investigations.
>
> I guess we need to have a look at not only issue 4636 but issue 4352.
> Also please add Read/Write method tests to protoconn_test.go.

[jeff.allen, 2013-01-09 15:43:00]

With this change, a call to net.DialUnix that would have failed before with an
error now returns a net.PacketConn that is usable. It seems to me this behavior
more correctly implements the Go 1 docs.

I will update the CL when I see how this fix behaves on 1.0.3.

[jeff.allen, 2013-01-09 16:11:17]

On 2013/01/09 13:50:36, mikio wrote:
> Also please add Read/Write method tests to protoconn_test.go.

These changes to syscall don't touch the path Read/Write follow, only ReadFrom.
I'm not sure I understand what you're asking for.

[jeff.allen, 2013-01-09 16:11:32]

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

Please take another look.

[mikio, 2013-01-09 17:16:43]

On Thu, Jan 10, 2013 at 1:11 AM,  <jeff.allen@gmail.com> wrote:

> These changes to syscall don't touch the path Read/Write follow, only
> ReadFrom. I'm not sure I understand what you're asking for.

Sorry, I'm a bit confused because... as you pointed out UnixConn
(and tests for it) has been overlooked.

- Looks like DialUnix allows to make an unnamed unix domain
  socket on unixgram but Dial doesn't, right?

- If Dial doesn't need to support it, why DialUnix do that?
  Why not ListenUnixgram?

Anyway thank you for tackling this!

[rsc, 2013-01-09 19:54:10]

As far as API:
"unix" should be like "tcp".
"unixgram" should be like "udp".

Russ

[mikio, 2013-01-14 14:04:19]

Hi Jeff,

Could you please split this into two CLs:
- syscall: tweak recvfrom stuff with a test,
- net: add unnamed socket support to unixconn.

The former would be just a bug fix, the latter might be API re-design.

https://codereview.appspot.com/7058062/diff/4003/src/pkg/syscall/syscall_linu...
File src/pkg/syscall/syscall_linux.go (right):

https://codereview.appspot.com/7058062/diff/4003/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:550: if len > 0 {
not sure len >0 is enough for all cases.
how about this?

if rsa.Addr.Family != AF_UNSPEC {
        from, err = anyToSockaddr(&rsa)
}

[jeff.allen, 2013-01-15 15:45:39]

Please take another look.

I realized I didn't need to muck with the behavior of DialUnix in order to
correctly test this. So this is the minimum change that both fixes the two
issues and tests them both, but does not change net's behavior other than fixing
the issues.

Thanks for keeping me honest...

https://codereview.appspot.com/7058062/diff/4003/src/pkg/syscall/syscall_linu...
File src/pkg/syscall/syscall_linux.go (right):

https://codereview.appspot.com/7058062/diff/4003/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:550: if len > 0 {
On 2013/01/14 14:04:19, mikio wrote:
> not sure len >0 is enough for all cases.
> how about this?
> 
> if rsa.Addr.Family != AF_UNSPEC {
>         from, err = anyToSockaddr(&rsa)
> }

Done.

[mikio, 2013-01-16 05:05:53]

Seems tests don't work well on BSDs, break Plan9/Windows build.
Please feel free to take: https://codereview.appspot.com/7130044/.
It's a good time to have dedicated tests for UnixConn.

[mikio, 2013-01-16 05:08:56]

syscall: ReadFrom on unixgram sockets gave incorrect EAFNOSUPPORT

Also please change the CL description to more appropriate one.

On Wed, Jan 16, 2013 at 2:05 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> Seems tests don't work well on BSDs, break Plan9/Windows build.
> Please feel free to take: https://codereview.appspot.com/7130044/.
> It's a good time to have dedicated tests for UnixConn.

[jeff.allen, 2013-01-18 14:12:38]

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

Please take another look.

[rsc, 2013-01-18 22:39:03]

Please check that the returned address is nil.

[mikio, 2013-01-19 14:10:21]

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go
File src/pkg/net/unix_test.go (right):

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:18: // use ioutil.TempFile to get a name that is unique
// testUnixAddr returns a unique pathname that ... blah blah.

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:19: func genAddr() string {
maybe testUnixAddr

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:24: f.Close()
seems wrong f.Close then f.Name

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:44: 
sorry, i forgot to add a channel that avoids ECONNRESET (which rarely happens)
on BSDs.

off := make(chan bool)
data := ...
go func() {
   defer func() { off<-true }()
   :
}()

<-off
c.SetReadDeadline()

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:81: defer os.Remove(addr)
delete

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:95: go func() {
sorry, pls add a channel (same as above)

[jeff.allen, 2013-01-23 15:10:14]

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go
File src/pkg/net/unix_test.go (right):

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:18: // use ioutil.TempFile to get a name that is unique
On 2013/01/19 14:10:21, mikio wrote:
> // testUnixAddr returns a unique pathname that ... blah blah.

Done.

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:19: func genAddr() string {
On 2013/01/19 14:10:21, mikio wrote:
> maybe testUnixAddr

Done.

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:24: f.Close()
On 2013/01/19 14:10:21, mikio wrote:
> seems wrong f.Close then f.Name

Done.

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:44: 
On 2013/01/19 14:10:21, mikio wrote:
> sorry, i forgot to add a channel that avoids ECONNRESET (which rarely happens)
> on BSDs.
> 
> off := make(chan bool)
> data := ...
> go func() {
>    defer func() { off<-true }()
>    :
> }()
> 
> <-off
> c.SetReadDeadline()

Done.

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:81: defer os.Remove(addr)
On 2013/01/19 14:10:21, mikio wrote:
> delete

Done.

https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:95: go func() {
On 2013/01/19 14:10:21, mikio wrote:
> sorry, pls add a channel (same as above)

Done.

[mikio, 2013-01-23 16:18:22]

https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go
File src/pkg/net/unix_test.go (right):

https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:72: t.Errorf("neighbor address is %v", from)
hm, on freebsd it returns empty sockaddr.
jeff, what about on linux?

go test -v -run=TestReadUnixgramWithUnnamedSocket
=== RUN TestReadUnixgramWithUnnamedSocket-2
--- FAIL: TestReadUnixgramWithUnnamedSocket-2 (0.00 seconds)
unix_test.go:71: 	neighbor address is &net.UnixAddr{Name:"", Net:"unixgram"}
FAIL

https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:116: if _, peer, err = c.ReadFrom(nil); err != nil {
perhaps s/peer/from/ might be consistent.

        _, from, err := c.ReadFrom(nil)
        if err != nil {

https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:121: t.Errorf("peer adddress is %v", peer)
go test -v -run=TestReadUnixgramWithZeroBytesBuffer
=== RUN TestReadUnixgramWithZeroBytesBuffer-2
--- FAIL: TestReadUnixgramWithZeroBytesBuffer-2 (0.00 seconds)
unix_test.go:120: 	neighbor adddress is &net.UnixAddr{Name:"", Net:"unixgram"}
FAIL

[mikio, 2013-01-23 23:57:36]

On Thu, Jan 24, 2013 at 1:18 AM,  <mikioh.mikioh@gmail.com> wrote:

> hm, on freebsd it returns empty sockaddr.
> jeff, what about on linux?
>
> go test -v -run=TestReadUnixgramWithUnnamedSocket
> === RUN TestReadUnixgramWithUnnamedSocket-2
> --- FAIL: TestReadUnixgramWithUnnamedSocket-2 (0.00 seconds)
> unix_test.go:71:        neighbor address is &net.UnixAddr{Name:"",
> Net:"unixgram"}
> FAIL

ouch,

=== RUN TestReadUnixgramWithUnnamedSocket-2
--- PASS: TestReadUnixgramWithUnnamedSocket-2 (0.00 seconds)
unix_test.go:74: neighbor address is <nil>
PASS

didn't know that such differences, hm.

[mikio, 2013-01-24 02:16:42]

FYI

https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go
File src/pkg/net/unix_test.go (right):

https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:72: t.Errorf("neighbor address is %v", from)
okay, just confirmed that your tests light green on freebsd with
https://codereview.appspot.com/7204050/

[jeff.allen, 2013-01-30 12:42:27]

OK, I have incorporated 7204050 into this CL, and now I think it is ready to go.
Please take a look and check it in if you're happy.

[rsc, 2013-01-30 18:00:31]

LGTM

Actually it looks awful to me, but that's not your fault. I can't believe
someone thought this was a good idea for how sockets should behave.

[rsc, 2013-01-30 18:02:05]

*** Submitted as https://code.google.com/p/go/source/detail?r=9379ab966cb2 ***

syscall: handle empty address in ReadFrom better

Handle return values from recvfrom correctly when the
kernel decides to not return an address.

Fixes issue 4636.
Fixes issue 4352.

R=rsc, mikioh.mikioh, dave
CC=golang-dev
https://codereview.appspot.com/7058062

Committer: Russ Cox <rsc@golang.org>

[mikio, 2013-01-31 00:01:39]

LGTM

Thank you for fixing this.
Hope Go 1.1 will have no bash on unix domain socket stuff.

Will revisit net tests later, until Go 1.1 cutoff.

https://codereview.appspot.com/7058062/diff/32001/src/pkg/net/unix_test.go
File src/pkg/net/unix_test.go (right):

https://codereview.appspot.com/7058062/diff/32001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:115: var peer Addr
maybe,

_, from, err := c.ReadFrom(...)
  :
if from != nil{
  t.Errorf("peer adddress is %v", from)
}

might be consistent.