code review 7220047: go.net/proxy: fix desired destination address in SOCKS5...

go.net/proxy: fix desired destination address in SOCKS5 CONNECT

Both types IPv6 IPv4-mapped address and IPv4-compatible
address are not allowed to be used in wire protocols.

Fixes issue 4709.

Thank you raptium for original CL 6922050.

[mikio, 2013-01-26 12:22:27]

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

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

[mikio, 2013-01-26 13:20:46]

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

Please take another look.

[agl1, 2013-01-26 15:23:43]

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go
File proxy/proxy_test.go (right):

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode55
proxy/proxy_test.go:55: type testSOCKS5Dialer struct{}
This is just a Direct.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode62
proxy/proxy_test.go:62: es, err := net.Listen("tcp", "127.0.0.1:0")
es -> ?

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode67
proxy/proxy_test.go:67: gw, err := net.Listen("tcp", "127.0.0.1:0")
gw -> gateway?

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode79
proxy/proxy_test.go:79: t.Errorf("SOCKS5 failed: %v", err)
t.Fatalf rather than return.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode83
proxy/proxy_test.go:83: t.Errorf("SOCKS5.Dial failed: %v", err)
ditto (and throughout).

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode93
proxy/proxy_test.go:93: defer func() { wg.Done() }()
I don't think that you need to wrap this in a closure.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode95
proxy/proxy_test.go:95: gw.(*net.TCPListener).SetDeadline(time.Now().Add(100 *
time.Millisecond))
I'm wary of making this flakey on slower hardware. Can we do without the
timeout?

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode103
proxy/proxy_test.go:103: c.SetDeadline(time.Now().Add(100 * time.Millisecond))
(same here)

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode106
proxy/proxy_test.go:106: // at your service
comment seems superfluous.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode107
proxy/proxy_test.go:107: if _, err := c.Read(b); err != nil {
I think this should be a ReadFull to make sure that all the expected bytes are
read.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode117
proxy/proxy_test.go:117: n, err := c.Read(b)
also a ReadFull?

https://codereview.appspot.com/7220047/diff/8001/proxy/socks5.go
File proxy/socks5.go (left):

https://codereview.appspot.com/7220047/diff/8001/proxy/socks5.go#oldcode67
proxy/socks5.go:67: break
Doesn't this break everything?

[mikio, 2013-01-28 12:27:53]

Hello golang-dev@googlegroups.com, agl@golang.org (cc:
golang-dev@googlegroups.com, raptium@gmail.com),

Please take another look.

[mikio, 2013-01-28 12:28:51]

PTAL

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go
File proxy/proxy_test.go (right):

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode55
proxy/proxy_test.go:55: type testSOCKS5Dialer struct{}
On 2013/01/26 15:23:43, agl1 wrote:
> This is just a Direct.

aha, thanks.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode62
proxy/proxy_test.go:62: es, err := net.Listen("tcp", "127.0.0.1:0")
On 2013/01/26 15:23:43, agl1 wrote:
> es -> ?

changed to endSystem

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode67
proxy/proxy_test.go:67: gw, err := net.Listen("tcp", "127.0.0.1:0")
On 2013/01/26 15:23:43, agl1 wrote:
> gw -> gateway?

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode79
proxy/proxy_test.go:79: t.Errorf("SOCKS5 failed: %v", err)
On 2013/01/26 15:23:43, agl1 wrote:
> t.Fatalf rather than return.

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode83
proxy/proxy_test.go:83: t.Errorf("SOCKS5.Dial failed: %v", err)
On 2013/01/26 15:23:43, agl1 wrote:
> ditto (and throughout).

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode93
proxy/proxy_test.go:93: defer func() { wg.Done() }()
On 2013/01/26 15:23:43, agl1 wrote:
> I don't think that you need to wrap this in a closure.

yup, thx.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode95
proxy/proxy_test.go:95: gw.(*net.TCPListener).SetDeadline(time.Now().Add(100 *
time.Millisecond))
On 2013/01/26 15:23:43, agl1 wrote:
> I'm wary of making this flakey on slower hardware. Can we do without the
> timeout?

okay, will drop.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode103
proxy/proxy_test.go:103: c.SetDeadline(time.Now().Add(100 * time.Millisecond))
On 2013/01/26 15:23:43, agl1 wrote:
> (same here)

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode106
proxy/proxy_test.go:106: // at your service
On 2013/01/26 15:23:43, agl1 wrote:
> comment seems superfluous.

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode107
proxy/proxy_test.go:107: if _, err := c.Read(b); err != nil {
On 2013/01/26 15:23:43, agl1 wrote:
> I think this should be a ReadFull to make sure that all the expected bytes are
> read.

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/proxy_test.go#newcode117
proxy/proxy_test.go:117: n, err := c.Read(b)
On 2013/01/26 15:23:43, agl1 wrote:
> also a ReadFull?

Done.

https://codereview.appspot.com/7220047/diff/8001/proxy/socks5.go
File proxy/socks5.go (left):

https://codereview.appspot.com/7220047/diff/8001/proxy/socks5.go#oldcode67
proxy/socks5.go:67: break
nope.

[agl1, 2013-01-28 15:57:30]

LGTM

https://codereview.appspot.com/7220047/diff/12002/proxy/socks5.go
File proxy/socks5.go (left):

https://codereview.appspot.com/7220047/diff/12002/proxy/socks5.go#oldcode67
proxy/socks5.go:67: break
ah, indeed. I didn't have my Go hat on firmly. None the less, I worry that this
is a little too magical. I would prefer to waste an extra line of code to be
clear.

[mikio, 2013-01-28 18:23:46]

On Tue, Jan 29, 2013 at 12:57 AM,  <agl@golang.org> wrote:

> ah, indeed. I didn't have my Go hat on firmly. None the less, I worry
> that this is a little too magical.

When in Rome, we'll do as Romans do.

> I would prefer to waste an extra line of code to be clear.

Oh please. Flip-flopping btw Go and C/C++ becomes really hard
to me recently. No break in switch statements at Go land (and
playing with break statements at C/C++ lands) is the only way to
make me happy. ;)

[mikio, 2013-01-28 18:24:50]

*** Submitted as
https://code.google.com/p/go/source/detail?r=944169afe891&repo=net ***

go.net/proxy: fix desired destination address in SOCKS5 CONNECT

Both types IPv6 IPv4-mapped address and IPv4-compatible
address are not allowed to be used in wire protocols.

Fixes issue 4709.

Thank you raptium for original CL 6922050.

R=golang-dev, agl
CC=golang-dev, raptium
https://codereview.appspot.com/7220047

[agl1, 2013-01-28 18:28:18]

On Mon, Jan 28, 2013 at 1:23 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> Oh please. Flip-flopping btw Go and C/C++ becomes really hard
> to me recently. No break in switch statements at Go land (and
> playing with break statements at C/C++ lands) is the only way to
> make me happy. ;)

I do try to be polite in code reviews but that was not intended to be
an optional request.


Cheers

AGL

[rsc, 2013-01-28 18:31:09]

I've found it useful in those cases to write:

  // ok

instead of break. Then it's clear that you're marking an empty body
explicitly.

Russ

[mikio, 2013-01-28 18:46:33]

On Tue, Jan 29, 2013 at 3:28 AM, Adam Langley <agl@golang.org> wrote:

> I do try to be polite in code reviews but that was not intended to be
> an optional request.

Yes, thank you for the review.

On Tue, Jan 29, 2013 at 3:31 AM, Russ Cox <rsc@golang.org> wrote:

> I've found it useful in those cases to write:
>
>   // ok
>
> instead of break. Then it's clear that you're marking an empty body
> explicitly.

Will try, thanks.