go.crypto/ssh: close channel feeding tcpListener.

go.crypto/ssh: close channel feeding tcpListener.

Close both on closing the listener, and on closing the
connection. Test the former case.

[hanwen-google, 2013-07-16 14:03:56]

https://codereview.appspot.com/11349043/diff/2001/ssh/test/forward_unix_test.go
File ssh/test/forward_unix_test.go (right):

https://codereview.appspot.com/11349043/diff/2001/ssh/test/forward_unix_test....
ssh/test/forward_unix_test.go:153: // TODO(hanwen): test that closing the
connection also
this is hard to test, since killing the server doesn't make the client read
exit. 

How about substituting a unix socket for the client connection instead? That
would let us throw out all the code to simulate a net.Conn by using a real one
instead.

[dave_cheney.net, 2013-07-16 14:06:49]

On 2013/07/16 14:03:56, hanwen wrote:
>
https://codereview.appspot.com/11349043/diff/2001/ssh/test/forward_unix_test.go
> File ssh/test/forward_unix_test.go (right):
> 
>
https://codereview.appspot.com/11349043/diff/2001/ssh/test/forward_unix_test....
> ssh/test/forward_unix_test.go:153: // TODO(hanwen): test that closing the
> connection also
> this is hard to test, since killing the server doesn't make the client read
> exit. 
> 
> How about substituting a unix socket for the client connection instead? That
> would let us throw out all the code to simulate a net.Conn by using a real one
> instead.

I think I decided not to do that. I don't remember the reason now, but I
wouldn't be surprised if it turned out that sshd won't listen on a unix socket.

[hanwen-google, 2013-07-16 14:14:01]

You don't have to make sshd listen to the socket? You could use the
socket to create the 2 sides of a net.Conn, and then pass the fd of
the net.Conn as stdout/stdin to sshd

On Tue, Jul 16, 2013 at 4:06 PM,  <dave@cheney.net> wrote:
> On 2013/07/16 14:03:56, hanwen wrote:
>
>
https://codereview.appspot.com/11349043/diff/2001/ssh/test/forward_unix_test.go
>>
>> File ssh/test/forward_unix_test.go (right):
>
>
>
>
https://codereview.appspot.com/11349043/diff/2001/ssh/test/forward_unix_test....
>>
>> ssh/test/forward_unix_test.go:153: // TODO(hanwen): test that closing
>
> the
>>
>> connection also
>> this is hard to test, since killing the server doesn't make the client
>
> read
>>
>> exit.
>
>
>> How about substituting a unix socket for the client connection
>
> instead? That
>>
>> would let us throw out all the code to simulate a net.Conn by using a
>
> real one
>>
>> instead.
>
>
> I think I decided not to do that. I don't remember the reason now, but I
> wouldn't be surprised if it turned out that sshd won't listen on a unix
> socket.
>
> https://codereview.appspot.com/11349043/



-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[hanwen-google, 2013-07-19 08:41:00]

I mistakenly deleted the repo that held this change. If it LGTMs, I would be
grateful if you could apply it directly. If not, let me know, and I'll try to
resurrect it.

https://codereview.appspot.com/11349043/diff/2001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/11349043/diff/2001/ssh/tcpip.go#newcode112
ssh/tcpip.go:112: close(f.c)
oops; the removal of return here is unintended.

[dave_cheney.net, 2013-07-19 08:48:05]

You can fix it by checking out again, setting up the code review extension then
running hg clpatch.

Sorry I haven't had a chance to review it yet.



On 19/07/2013, at 18:41, hanwen@google.com wrote:

> I mistakenly deleted the repo that held this change. If it LGTMs, I
> would be grateful if you could apply it directly. If not, let me know,
> and I'll try to resurrect it.
> 
> 
> https://codereview.appspot.com/11349043/diff/2001/ssh/tcpip.go
> File ssh/tcpip.go (right):
> 
> https://codereview.appspot.com/11349043/diff/2001/ssh/tcpip.go#newcode112
> ssh/tcpip.go:112: close(f.c)
> oops; the removal of return here is unintended.
> 
> https://codereview.appspot.com/11349043/

[hanwen-google, 2013-07-20 11:27:34]

https://codereview.appspot.com/11349043/diff/2001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/11349043/diff/2001/ssh/tcpip.go#newcode112
ssh/tcpip.go:112: close(f.c)
On 2013/07/19 08:41:00, hanwen wrote:
> oops; the removal of return here is unintended.

Done.

[dave_cheney.net, 2013-07-22 11:49:39]

LGTM. Thank you. Sorry for the delay.

[dave_cheney.net, 2013-07-22 11:50:21]

*** Submitted as
https://code.google.com/p/go/source/detail?r=6ad91f44b054&repo=crypto ***

go.crypto/ssh: close channel feeding tcpListener.

Close both on closing the listener, and on closing the
connection. Test the former case.

R=dave
CC=golang-dev
https://codereview.appspot.com/11349043

Committer: Dave Cheney <dave@cheney.net>

https://codereview.appspot.com/11349043/diff/12001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/11349043/diff/12001/ssh/client.go#newcode214
ssh/client.go:214: c.forwardList.closeAll()
// TODO, all these closes should probably be merged into one case.

[hanwen-google, 2013-07-24 20:20:09]

*** Abandoned ***