code review 6395055: net: avoid nil pointer dereference when RemoteAddr.Stri...

net: avoid nil pointer dereference when RemoteAddr.String method chain is called

Fixes issue 3721.

[mikio, 2012-07-17 11:36:34]

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

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

[mikio, 2012-07-19 11:12:52]

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

Please take another look.

[dave_cheney.net, 2012-07-20 05:07:57]

Hi Mikio,

Thank you for fixing this issue. I'm not keen on those empty sentinel values, is
there a way to avoid using them ?

Cheers

Dave

http://codereview.appspot.com/6395055/diff/5001/src/pkg/net/ipraw_test.go
File src/pkg/net/ipraw_test.go (right):

http://codereview.appspot.com/6395055/diff/5001/src/pkg/net/ipraw_test.go#new...
src/pkg/net/ipraw_test.go:28: t.Logf("test disabled; must be root")
test skipped;

http://codereview.appspot.com/6395055/diff/5001/src/pkg/net/ipraw_test.go#new...
src/pkg/net/ipraw_test.go:40: c.LocalAddr()
I think you should log these values. Maybe leave a comment to issue 3721, as it
is not clear from reading what these are testing.

http://codereview.appspot.com/6395055/diff/11001/src/pkg/net/tcpsock.go
File src/pkg/net/tcpsock.go (right):

http://codereview.appspot.com/6395055/diff/11001/src/pkg/net/tcpsock.go#newco...
src/pkg/net/tcpsock.go:22: case nil, tcpAddrNull:
If I revert this line, to be just

case nil:

The test still passes

http://codereview.appspot.com/6395055/diff/11001/src/pkg/net/udp_test.go
File src/pkg/net/udp_test.go (right):

http://codereview.appspot.com/6395055/diff/11001/src/pkg/net/udp_test.go#newc...
src/pkg/net/udp_test.go:29: }
Can I suggest a comment outlining what these method calls are testing.

http://codereview.appspot.com/6395055/diff/11001/src/pkg/net/udpsock.go
File src/pkg/net/udpsock.go (right):

http://codereview.appspot.com/6395055/diff/11001/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:25: case nil, udpAddrNull:
Same, 

case nil:

still passes

[mikio, 2012-07-22 13:46:46]

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

Please take another look.

[mikio, 2012-07-22 14:01:34]

Thank your for your comments. I revised CL to focus on the issue
related a datagram, unconnected socket. Please take another look.

On Fri, Jul 20, 2012 at 2:07 PM,  <dave@cheney.net> wrote:

> Thank you for fixing this issue. I'm not keen on those empty sentinel
> values, is there a way to avoid using them ?

I guess, there's no way to avoid nil pointer dereference in method chain
calls without method holders.

[mikio, 2012-07-23 09:41:11]

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

Please take another look.

[rsc, 2012-07-30 01:23:43]

Thanks for looking into this. I think I helped someone else with this offline
recently too. I am all for making RemoteAddr always return non-nil, but I think
we should do it without requiring Getsockname and Getpeername to succeed always.
Sockets are strange enough that I can't promise that they will always succeed,
and given the choice between a connection that works but returns no address
information and a socket that closes itself, I would rather have the first.

http://codereview.appspot.com/6395055/diff/5037/src/pkg/net/fd.go
File src/pkg/net/fd.go (right):

http://codereview.appspot.com/6395055/diff/5037/src/pkg/net/fd.go#newcode642
src/pkg/net/fd.go:642: lsa, err := syscall.Getsockname(netfd.sysfd)
I was ignoring the error on purpose. I'd really rather have a working connection
with no local addr than make this an error. We managed to create the socket: we
should return it.

http://codereview.appspot.com/6395055/diff/5037/src/pkg/net/file.go
File src/pkg/net/file.go (right):

http://codereview.appspot.com/6395055/diff/5037/src/pkg/net/file.go#newcode28
src/pkg/net/file.go:28: lsa, err := syscall.Getsockname(fd)
Same.

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

http://codereview.appspot.com/6395055/diff/5037/src/pkg/net/sock.go#newcode65
src/pkg/net/sock.go:65: lsa, err := syscall.Getsockname(s)
Again.

http://codereview.appspot.com/6395055/diff/5037/src/pkg/net/sock.go#newcode78
src/pkg/net/sock.go:78: rsa, err := syscall.Getpeername(s)
Again.

[mikio, 2012-08-07 14:18:47]

On 2012/07/30 01:23:43, rsc wrote:
> Thanks for looking into this. I think I helped someone else with this offline
> recently too. I am all for making RemoteAddr always return non-nil, but I
think
> we should do it without requiring Getsockname and Getpeername to succeed
always.
> Sockets are strange enough that I can't promise that they will always succeed,
> and given the choice between a connection that works but returns no address
> information and a socket that closes itself, I would rather have the first.

agreed, all done, ptal.

[mikio, 2012-08-07 14:19:14]

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

Please take another look.

[mikio, 2012-08-12 04:38:57]

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

Please take another look.

[dave_cheney.net, 2012-08-12 05:58:17]

Looks good to me, modulo the comment below. Thanks.

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

http://codereview.appspot.com/6395055/diff/32001/src/pkg/net/sock.go#newcode99
src/pkg/net/sock.go:99: )
You can use typed nils instead of sentinels here:

func nullProtocolAddr(f, t int) Addr {
        switch f {
        case syscall.AF_INET, syscall.AF_INET6:
                switch t {
                case syscall.SOCK_STREAM:
                        return (*TCPAddr)(nil)
                case syscall.SOCK_DGRAM:
                        return (*UDPAddr)(nil)
                case syscall.SOCK_RAW:
                        return (*IPAddr)(nil)
                }
        case syscall.AF_UNIX:
                switch t {
                case syscall.SOCK_STREAM, syscall.SOCK_DGRAM,
syscall.SOCK_SEQPACKET:
                        return (*UnixAddr)(nil)
                }
        }
        panic("unreachable")
}

[mikio, 2012-08-12 06:22:32]

On Sun, Aug 12, 2012 at 2:58 PM,  <dave@cheney.net> wrote:

> You can use typed nils instead of sentinels here:

Nice catch. I thought that those placeholders might be useful
for fixing issue 3838, but should be another CL. Thanks.

[mikio, 2012-08-12 06:49:08]

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

Please take another look.

[dave_cheney.net, 2012-08-12 06:52:09]

LGTM. My only remaining comment would be to consider if it is possible to
replace the multiple occurrences of this

+		if lsa, _ := syscall.Getsockname(s); lsa == nil {
+			laddr = nullProtocolAddr(fd.family, fd.sotype)
+		} else {
+			laddr = toAddr(lsa)
+		}

With a helper method.

[dave_cheney.net, 2012-08-21 00:41:35]

ping ?

[mikio, 2012-08-21 12:48:14]

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

Please take another look.

[dave_cheney.net, 2012-08-21 13:00:04]

Thank you. I'm very happy with how this turned out.

[mikio, 2012-08-23 11:54:10]

*** Submitted as http://code.google.com/p/go/source/detail?r=2518eee18c4f ***

net: avoid nil pointer dereference when RemoteAddr.String method chain is called

Fixes issue 3721.

R=dave, rsc
CC=golang-dev
http://codereview.appspot.com/6395055

[rsc, 2012-08-31 11:09:52]

Sorry for the delayed review. I'm not sure about this.

http://codereview.appspot.com/6395055/diff/39001/src/pkg/net/fd.go
File src/pkg/net/fd.go (right):

http://codereview.appspot.com/6395055/diff/39001/src/pkg/net/fd.go#newcode618
src/pkg/net/fd.go:618: netfd.setAddr(localSockname(fd, toAddr), toAddr(rsa))
It may not matter, but a direct translation of the old code would have used
netfd, not fd, in the call to localSockname. netfd is the accepted connection,
while fd is the listener.

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

http://codereview.appspot.com/6395055/diff/39001/src/pkg/net/sock.go#newcode103
src/pkg/net/sock.go:103: return (*TCPAddr)(nil)
This is unusual. Normally we do not put typed nils inside interface values.
Instead we return interface nils instead. I understand that this may keep
.String() from crashing, but it doesn't help callers who expect a successful
.(*TCPAddr) to result in a real data structure.