code review 6119043: net: fix crash of Listen with "" or nil laddr

net: fix crash of Listen with "" or nil laddr

Fixes issue 3584.

[mikio, 2012-04-24 06:29:21]

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

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

[mikio, 2012-04-24 06:34:30]

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

Please take another look.

[dfc, 2012-04-24 09:15:57]

Hi Mikio, thank you for fixing this issue, please see my comments below.

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go
File src/pkg/net/unicast_test.go (right):

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go#n...
src/pkg/net/unicast_test.go:549: t.Fatalf("panicked")
Should you print the value of p ?

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go#n...
src/pkg/net/unicast_test.go:555: }
Do you need to test Listen("udp", "") etc ?

[mikio, 2012-04-24 09:36:22]

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go
File src/pkg/net/unicast_test.go (right):

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go#n...
src/pkg/net/unicast_test.go:549: t.Fatalf("panicked")
On 2012/04/24 09:15:57, dfc wrote:
> Should you print the value of p ?

nope, thx.

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go#n...
src/pkg/net/unicast_test.go:549: t.Fatalf("panicked")
On 2012/04/24 09:15:57, dfc wrote:
> Should you print the value of p ?

Done.

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go#n...
src/pkg/net/unicast_test.go:555: }
On 2012/04/24 09:15:57, dfc wrote:
> Do you need to test Listen("udp", "") etc ?

good catch, I forgot ListenPacket.

http://codereview.appspot.com/6119043/diff/5003/src/pkg/net/unicast_test.go#n...
src/pkg/net/unicast_test.go:555: }
On 2012/04/24 09:15:57, dfc wrote:
> Do you need to test Listen("udp", "") etc ?

Done.

[mikio, 2012-04-24 09:37:07]

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

Please take another look.

[dfc, 2012-04-24 09:53:05]

LGTM. 

I was uncomfortable that a nil laddr value got down so deep in the package
internals, but looking at the rest of favoriteAddrFamily() it already expects
laddr to be nil in other cases. It's good there are tests to validate this
behaviour now.

[mikio, 2012-04-25 00:47:56]

Thank you for your review.
I'll submit this CL after Go 1.0.1 release.

On Tue, Apr 24, 2012 at 6:53 PM,  <dave@cheney.net> wrote:
> LGTM.
>
> I was uncomfortable that a nil laddr value got down so deep in the
> package internals, but looking at the rest of favoriteAddrFamily() it
> already expects laddr to be nil in other cases. It's good there are
> tests to validate this behaviour now.
>
> http://codereview.appspot.com/6119043/

[dsymonds, 2012-04-25 00:49:00]

On Wed, Apr 25, 2012 at 10:47 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> I'll submit this CL after Go 1.0.1 release.

Don't bother waiting. Go 1.0.1 is going to be a set of cherrypicks,
not a fresh cut on the main branch.

[rsc, 2012-04-25 02:01:11]

I would have expected the laddr == nil to happen after the mapping check.

http://codereview.appspot.com/6119043/diff/12001/src/pkg/net/ipsock_posix.go
File src/pkg/net/ipsock_posix.go (right):

http://codereview.appspot.com/6119043/diff/12001/src/pkg/net/ipsock_posix.go#...
src/pkg/net/ipsock_posix.go:101: if laddr == nil {
This seems wrong to me.  Why should "" be tcp4-only?

[mikio, 2012-04-25 03:21:13]

http://codereview.appspot.com/6119043/diff/12001/src/pkg/net/ipsock_posix.go
File src/pkg/net/ipsock_posix.go (right):

http://codereview.appspot.com/6119043/diff/12001/src/pkg/net/ipsock_posix.go#...
src/pkg/net/ipsock_posix.go:101: if laddr == nil {
On 2012/04/25 02:01:11, rsc wrote:
> This seems wrong to me.  Why should "" be tcp4-only?

oops, you are right. thx.

[mikio, 2012-04-25 03:23:13]

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

Please take another look.

[rsc, 2012-04-25 03:28:35]

LGTM

[mikio, 2012-04-25 03:29:24]

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

net: fix crash of Listen with "" or nil laddr

Fixes issue 3584.

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

[mikio, 2012-04-25 04:24:29]

Sorry this CL fixes issue 3548, not 3584.

On Wed, Apr 25, 2012 at 12:29 PM,  <mikioh.mikioh@gmail.com> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=b07591eb8077 ***
>
>
> net: fix crash of Listen with "" or nil laddr
>
> Fixes issue 3584.
>
> R=dave, dsymonds, rsc
> CC=golang-dev
> http://codereview.appspot.com/6119043
>
>
> http://codereview.appspot.com/6119043/