code review 7038045: net: SplitHostPort: adjust error message for missing po...

net: SplitHostPort: adjust error message for missing port in IPv6 addresses

An hostport of "[::1]" now results in the same error message
"missing port in address" as the hostport value "127.0.0.1",
so SplitHostPort won't complain about "too many colons
in address" anymore for an IPv6 address missing a port.

Added tests checking the error values.

Fixes issue 4526.

[knieriem, 2012-12-31 20:41:07]

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

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

[dave_cheney.net, 2012-12-31 23:34:48]

What happens with these test cases

0.0.0.0:0
:0
127.0.0.1:
www.google.com:
On 1 Jan 2013 10:27, <mt4swm@googlemail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: mikioh.mikioh@gmail.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net: SplitHostPort: adjust error message for missing port in IPv6
> addresses
>
> An hostport of "[::1]" now results in the same error message
> "missing port in address" as the hostport value "127.0.0.1",
> so SplitHostPort won't complain about "too many colons
> in address" anymore for an IPv6 address missing a port.
>
> Added tests checking the error values.
>
> Fixes issue 4526.
>
> Please review this at
https://codereview.appspot.**com/7038045/<https://codereview.appspot.com/7038...
>
> Affected files:
>   M src/pkg/net/ip_test.go
>   M src/pkg/net/ipsock.go
>
>
> Index: src/pkg/net/ip_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/ip_test.go
> +++ b/src/pkg/net/ip_test.go
> @@ -271,12 +271,32 @@
>         {"google.com", "https%foo", "google.com:https%foo"}, // Go 1.0
> behavior
>  }
>
> +var splitfailuretests = []struct {
> +       HostPort string
> +       Err      string
> +}{
> +       {"www.google.com", "missing port in address"},
> +       {"127.0.0.1", "missing port in address"},
> +       {"[::1]", "missing port in address"},
> +       {"::1", "too many colons in address"},
> +}
> +
>  func TestSplitHostPort(t *testing.T) {
>         for _, tt := range splitjointests {
>                 if host, port, err := SplitHostPort(tt.Join); host !=
> tt.Host || port != tt.Port || err != nil {
>                         t.Errorf("SplitHostPort(%q) = %q, %q, %v; want %q,
> %q, nil", tt.Join, host, port, err, tt.Host, tt.Port)
>                 }
>         }
> +       for _, tt := range splitfailuretests {
> +               if _, _, err := SplitHostPort(tt.HostPort); err == nil {
> +                       t.Errorf("SplitHostPort(%q) should have failed",
> tt.HostPort)
> +               } else {
> +                       e := err.(*AddrError)
> +                       if e.Err != tt.Err {
> +                               t.Errorf("SplitHostPort(%q) = _, _, %q;
> want %q", tt.HostPort, e.Err, tt.Err)
> +                       }
> +               }
> +       }
>  }
>
>  func TestJoinHostPort(t *testing.T) {
> Index: src/pkg/net/ipsock.go
> ==============================**==============================**=======
> --- a/src/pkg/net/ipsock.go
> +++ b/src/pkg/net/ipsock.go
> @@ -85,9 +85,12 @@
>         }
>         host, port = hostport[:i], hostport[i+1:]
>         // Can put brackets around host ...
> -       if len(host) > 0 && host[0] == '[' && host[len(host)-1] == ']' {
> +       switch {
> +       case insideBrackets(host):
>                 host = host[1 : len(host)-1]
> -       } else {
> +       case insideBrackets(hostport):
> +               err = &AddrError{"missing port in address", hostport}
> +       default:
>                 // ... but if there are no brackets, no colons.
>                 if byteIndex(host, ':') >= 0 {
>                         err = &AddrError{"too many colons in address",
> hostport}
> @@ -97,6 +100,10 @@
>         return
>  }
>
> +func insideBrackets(s string) bool {
> +       return len(s) > 0 && s[0] == '[' && s[len(s)-1] == ']'
> +}
> +
>  // JoinHostPort combines host and port into a network address
>  // of the form "host:port" or, if host contains a colon, "[host]:port".
>  func JoinHostPort(host, port string) string {
>
>
>

[knieriem, 2013-01-02 02:06:07]

On Tue, 1 Jan 2013 10:34:47 +1100
Dave Cheney <dave@cheney.net> wrote:

> What happens with these test cases
> 
> 0.0.0.0:0
> :0
> 127.0.0.1:
> www.google.com:

These hostport values all pass SplitHostPort without
an error (same as in the past).

The last two values (those with the colon at the end)
should probably also yield a "missing port in address"
error. I can adjust SplitHostPort accordingly if you like.

The first value is matched by the test case for
127.0.0.1:1234, I suppose.

For the second value -- ":0" with a missing host part --
I could add a test to splitjointests that checks that empty
host parts actually are regarded as valid (which is
SplitHostPort's current behaviour, and has a usage in
address arguments to Listen calls).
	+	{"", "0", ":0"},

[rsc, 2013-01-07 04:55:41]

https://codereview.appspot.com/7038045/diff/2002/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

https://codereview.appspot.com/7038045/diff/2002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:271: {"google.com", "https%foo", "google.com:https%foo"},
// Go 1.0 behavior
Please add Dave's test cases here. You can put // Go 1.0 behavior
for the empty string ports if you like. The goal is to avoid regressions.
Please also add

{"[foo]:[bar]baz", "foo", "[bar]baz"}

https://codereview.appspot.com/7038045/diff/2002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:281: {"::1", "too many colons in address"},
Please add

{"[foo:bar]", "missing port in address"}
{"[foo:bar]baz", "missing port in address"}
{"[foo]:[bar]:baz", "too many colons in address"},

here. I think you will find that the last two cases need work.
We should probably reject them, and that will be a Go 1.0 regression,
but one that I am willing to treat as a bug fix.

The docs are clear that the allowed forms are host:port and [host]:port.
I think that means that if the string begins with [, then scanning forward 
we have to find a ], and the ] must be followed by a :, which is the separator.
There must be no colons after the separator.
If the string does not begin with [, then the separator is the last :.

[knieriem, 2013-01-08 13:58:40]

I've prepared a program in the Go Playground that
contains all the changes:
	http://play.golang.org/p/9Im0PcQf-F

It tells how the behaviour of the new SplitHostPort function
will differ from the one in the Playground's Go version (1.0.3).
Test cases are prefixed by "==" and "!=" accordingly.

I've added another test case, "[foo]bar:baz", which didn't
fail in Go 1.0, but now would fail with the "missing port" error.

I wonder how the following cases should be handled:
	foo[bar]:baz
	fo[bar]o:baz

With the present patch, and in Go 1.0, these values wouldn't yield
an error.

I'll push the new patch to codereview a few hours later.

[knieriem, 2013-01-08 15:59:05]

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

Please take another look.

[rsc, 2013-01-09 19:19:12]

Let's reject [] anywhere other than in the [host]:port form.

[knieriem, 2013-01-09 22:38:18]

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

Please take another look.

[knieriem, 2013-01-09 22:40:15]

On Wed, 9 Jan 2013 14:19:10 -0500
Russ Cox <rsc@golang.org> wrote:

> Let's reject [] anywhere other than in the [host]:port form.
> 

I've adjusted the patch so that SplitHostPort works that way now.

The (updated) program showing the differences between go1.0.3
and the patched version can be found at:
	http://play.golang.org/p/eZ1H5z-F10

[rsc, 2013-01-18 20:15:35]

LGTM

Perhaps mikio would like to look too.

[mikio, 2013-01-19 00:33:58]

LGTM

leave for rsc

[rsc, 2013-01-22 22:03:40]

LGTM

Hi Michael,

It looks like you still need to complete a CLA as described at
http://golang.org/doc/contribute.html#copyright.

Thanks.
Russ

[rsc, 2013-01-30 17:25:21]

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

net: SplitHostPort: adjust error message for missing port in IPv6 addresses

An hostport of "[::1]" now results in the same error message
"missing port in address" as the hostport value "127.0.0.1",
so SplitHostPort won't complain about "too many colons
in address" anymore for an IPv6 address missing a port.

Added tests checking the error values.

Fixes issue 4526.

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

Committer: Russ Cox <rsc@golang.org>