Issue 6479053: code review 6479053: net: fix nil pointer defererence in selfConnect for TCP (Closed)
|
Created:
11 years, 8 months ago by mikio Modified:
11 years, 7 months ago CC:
golang-dev, aam, gobot Visibility:
Public. |
Descriptionnet: fix nil pointer defererence in selfConnect for TCP
localSockname and remoteSockname return (*TCPAddr)(nil)
when syscall getsockname, getpeername fail.
Fixes issue 4033.
Patch Set 1 : diff -r ca5e20f93081 https://code.google.com/p/go #Patch Set 2 : diff -r 9e610a78455c https://code.google.com/p/go #
Total comments: 6
Patch Set 3 : diff -r 9e610a78455c https://code.google.com/p/go #Patch Set 4 : diff -r 3181682928c9 https://code.google.com/p/go #
Total comments: 5
Patch Set 5 : diff -r 3181682928c9 https://code.google.com/p/go #MessagesTotal messages: 17
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
Sign in to reply to this message.
Thank you for fixing this so quickly. http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcp_test.go File src/pkg/net/tcp_test.go (right): http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcp_test.go#newco... src/pkg/net/tcp_test.go:121: func TestTCPSelfConenct(t *testing.T) { s/Conenct/Connect/ http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcp_test.go#newco... src/pkg/net/tcp_test.go:128: selfConnect(fd2, nil) I think you need an additional test for something that isn't nil and isn't a (*net.TCPAddr) fd3 := &netFD{} fd3.setAddr(nullProtocolAddr(syscall.AF_INET, syscall.SOCK_DGRAM), nullProtocolAddr(syscall.AF_INET, syscall.SOCK_DGRAM)) selfConnect(fd3, nil) http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcpsock_posix.go File src/pkg/net/tcpsock_posix.go (right): http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcpsock_posix.go#... src/pkg/net/tcpsock_posix.go:203: if fd.laddr == nil || fd.laddr == (*TCPAddr)(nil) || fd.raddr == nil || fd.raddr == (*TCPAddr)(nil) { We still have to check that {l,r}addr is (*TCPAddr). Maybe something like this: l, ok1 := fd.laddr.(*TCPAddr) r, ok2 := fd.raddr.(*TCPAddr) if !ok1 || !ok2 || l == nil || r == nil { return true } return l.Port == r.Port && l.IP.Equal(r.IP) But I think you could also make the argument that selfConnect expects to only be called with two tcp endpoints, so you can make the cast without a check.
Sign in to reply to this message.
http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcp_test.go File src/pkg/net/tcp_test.go (right): http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcp_test.go#newco... src/pkg/net/tcp_test.go:121: func TestTCPSelfConenct(t *testing.T) { On 2012/08/25 09:35:46, dfc wrote: > s/Conenct/Connect/ Done. http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcp_test.go#newco... src/pkg/net/tcp_test.go:128: selfConnect(fd2, nil) On 2012/08/25 09:35:46, dfc wrote: > I think you need an additional test for something that isn't nil and isn't a > (*net.TCPAddr) Not necessary for args that passed DialTCP gate. http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcpsock_posix.go File src/pkg/net/tcpsock_posix.go (right): http://codereview.appspot.com/6479053/diff/2002/src/pkg/net/tcpsock_posix.go#... src/pkg/net/tcpsock_posix.go:203: if fd.laddr == nil || fd.laddr == (*TCPAddr)(nil) || fd.raddr == nil || fd.raddr == (*TCPAddr)(nil) { ditto.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
thanks to Dave Chaney who alerted me to this CL. I had built a somewhat reproducible case which panic-ed pointing at this function. After the CL was applied I haven't seen the error in about 500 retries.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.go File src/pkg/net/tcp_posix_test.go (right): http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.g... src/pkg/net/tcp_posix_test.go:19: {&netFD{laddr: nil, raddr: nil}, nil, true}, Why do we need to test this? selfConnect is only called from DialTCP and the type of {l,r}addr is assured to be *TCPAddr, right? http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcpsock_posix.go File src/pkg/net/tcpsock_posix.go (right): http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcpsock_posix.go... src/pkg/net/tcpsock_posix.go:203: if fd.laddr == nil || fd.laddr == (*TCPAddr)(nil) || fd.raddr == nil || fd.raddr == (*TCPAddr)(nil) { See comment in other file
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.go File src/pkg/net/tcp_posix_test.go (right): http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.g... src/pkg/net/tcp_posix_test.go:19: {&netFD{laddr: nil, raddr: nil}, nil, true}, right, I said so. ;) thx. http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.g... src/pkg/net/tcp_posix_test.go:19: {&netFD{laddr: nil, raddr: nil}, nil, true}, On 2012/08/29 10:33:07, dfc wrote: > Why do we need to test this? selfConnect is only called from DialTCP and the > type of {l,r}addr is assured to be *TCPAddr, right? Done. http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcpsock_posix.go File src/pkg/net/tcpsock_posix.go (right): http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcpsock_posix.go... src/pkg/net/tcpsock_posix.go:203: if fd.laddr == nil || fd.laddr == (*TCPAddr)(nil) || fd.raddr == nil || fd.raddr == (*TCPAddr)(nil) { On 2012/08/29 10:33:07, dfc wrote: > See comment in other file Done.
Sign in to reply to this message.
On 2012/08/29 10:48:28, mikio wrote: > http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.go > File src/pkg/net/tcp_posix_test.go (right): > > http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.g... > src/pkg/net/tcp_posix_test.go:19: {&netFD{laddr: nil, raddr: nil}, nil, true}, > right, I said so. ;) thx. > > http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcp_posix_test.g... > src/pkg/net/tcp_posix_test.go:19: {&netFD{laddr: nil, raddr: nil}, nil, true}, > On 2012/08/29 10:33:07, dfc wrote: > > Why do we need to test this? selfConnect is only called from DialTCP and the > > type of {l,r}addr is assured to be *TCPAddr, right? > > Done. > > http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcpsock_posix.go > File src/pkg/net/tcpsock_posix.go (right): > > http://codereview.appspot.com/6479053/diff/16003/src/pkg/net/tcpsock_posix.go... > src/pkg/net/tcpsock_posix.go:203: if fd.laddr == nil || fd.laddr == > (*TCPAddr)(nil) || fd.raddr == nil || fd.raddr == (*TCPAddr)(nil) { > On 2012/08/29 10:33:07, dfc wrote: > > See comment in other file > > Done. LGTM, but maybe wait for rsc.
Sign in to reply to this message.
> LGTM, but maybe wait for rsc. as usual.
Sign in to reply to this message.
R=rsc (assigned by rsc)
Sign in to reply to this message.
On 2012/09/01 14:40:26, gobot wrote: > R=rsc (assigned by rsc) ping.
Sign in to reply to this message.
I don't believe this is the correct fix. This was created by returning the typed nil in Mikio's earlier CL. I think the correct fix is to undo that and accept that RemoteAddr can return nil. Russ
Sign in to reply to this message.
ping? On 2012/09/10 16:15:13, rsc wrote: > I don't believe this is the correct fix. This was created by returning the typed > nil in Mikio's earlier CL. I think the correct fix is to undo that and accept > that RemoteAddr can return nil. > > Russ
Sign in to reply to this message.
On 2012/09/10 16:15:13, rsc wrote: > I don't believe this is the correct fix. This was created by returning the typed > nil in Mikio's earlier CL. I think the correct fix is to undo that and accept > that RemoteAddr can return nil. sure, will do.
Sign in to reply to this message.
|
