code review 6405064: go.crypto/ssh: prevent channel writes after Close

go.crypto/ssh: prevent channel writes after Close

Fixes issue 3810.

This change introduces an atomic boolean to guard the close
of the clientChan. Previously the client code was quite
lax with the ordering of the close messages and could allow
window adjustment or EOF messages to leak after Close had
been signaled.

Consolidating the changes to the serverChan will be handled
in a following CL.

[dave_cheney.net, 2012-07-21 07:05:52]

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

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

[agl1, 2012-07-21 14:04:06]

It's tough to dive into this change and understand whether it's correct, but I
don't see any major red flags.

http://codereview.appspot.com/6405064/diff/10003/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6405064/diff/10003/ssh/channel.go#newcode93
ssh/channel.go:93: if c.setWeClosed() {
I'd turn this condition inside out so that the bulk of the function isn't
indented.

[dave_cheney.net, 2012-07-22 01:57:39]

Thank you for your comments. I can make this diff slightly smaller by not
touching the serverChan side (it will retain weClosed as a bool, and closed will
never be non zero).

http://codereview.appspot.com/6405064/diff/10003/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6405064/diff/10003/ssh/channel.go#newcode93
ssh/channel.go:93: if c.setWeClosed() {
On 2012/07/21 14:04:06, agl1 wrote:
> I'd turn this condition inside out so that the bulk of the function isn't
> indented.

Will address. In a future CL I want to make the atomic bool a type, and use it
for serverChan.dead as well, so this will possibly become c.weClosed.set().

[kardia, 2012-07-23 05:43:02]

Moving the msg chan into the channel struct rubs me the wrong way, though I see
why you did. Server code changes LGTM, the client changes appear benign (I have
less experience with that code).

[dave_cheney.net, 2012-07-23 16:05:27]

> Moving the msg chan into the channel struct rubs me the wrong way,
> though I see why you did. Server code changes LGTM, the client changes
> appear benign (I have less experience with that code).

Yes, moving the msg chan up a level could be handled another way, but
it would need a sendClose method on both *clientChan and *serverChan.
To me, the cost of the duplication was more than the check for a nil
chan during the close cycle.

>
> http://codereview.appspot.com/6405064/

[dave_cheney.net, 2012-08-22 10:46:41]

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

Please take another look.

[dave_cheney.net, 2012-08-22 10:47:31]

Please take another look. I have refactored the CL to be only the minimum change
required to pass the sample code in issue 3810.

[kardia, 2012-08-22 15:25:43]

I like how this removes redundant code in the client.

http://codereview.appspot.com/6405064/diff/25001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6405064/diff/25001/ssh/channel.go#newcode122
ssh/channel.go:122: // TODO(dfc) should this be io.EOF ?
Yes, I think this should be io.EOF. I would want to know if I'm not writing
everything I'm trying to.

http://codereview.appspot.com/6405064/diff/25001/ssh/channel.go#newcode149
ssh/channel.go:149: theySentEOF bool // used by serverChan
Can remove these comments, as they are now in serverChan.

[dave_cheney.net, 2012-08-23 00:15:18]

Thank you for your comments. I have restored the Close() -> io.EOF behaviour,
which came at the cost of a larger diff.

The serverChan and clientChan Close paths have now diverged again, so my next CL
will attempt to unify them again.

http://codereview.appspot.com/6405064/diff/25001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6405064/diff/25001/ssh/channel.go#newcode122
ssh/channel.go:122: // TODO(dfc) should this be io.EOF ?
On 2012/08/22 15:25:43, kardia wrote:
> Yes, I think this should be io.EOF. I would want to know if I'm not writing
> everything I'm trying to.

Done.

[dave_cheney.net, 2012-08-23 00:15:57]

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

Please take another look.

[kardia, 2012-08-23 00:21:34]

LGTM

[agl1, 2012-08-23 16:17:29]

LGTM

https://codereview.appspot.com/6405064/diff/25005/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/6405064/diff/25005/ssh/channel.go#newcode85
ssh/channel.go:85: isclosed          uint32 // atomic bool, non zero if true
isClosed?

https://codereview.appspot.com/6405064/diff/25005/ssh/session_test.go
File ssh/session_test.go (right):

https://codereview.appspot.com/6405064/diff/25005/ssh/session_test.go#newcode383
ssh/session_test.go:383: // TODO(dfc) currently writes succeed after Close()
you have this TODO, but the test Fatalf's is it's actually the case.

[dave_cheney.net, 2012-08-23 23:46:50]

*** Submitted as
http://code.google.com/p/go/source/detail?r=951fbbb0a477&repo=crypto ***

go.crypto/ssh: prevent channel writes after Close

Fixes issue 3810.

This change introduces an atomic boolean to guard the close
of the clientChan. Previously the client code was quite
lax with the ordering of the close messages and could allow
window adjustment or EOF messages to leak after Close had
been signaled.

Consolidating the changes to the serverChan will be handled
in a following CL.

R=agl, kardianos, gustav.paul
CC=golang-dev
http://codereview.appspot.com/6405064

[dave_cheney.net, 2012-08-23 23:47:52]

@agl - sorry I failed to address your two comments. I will address
them in the follow up CL.

On Fri, Aug 24, 2012 at 9:46 AM,  <dave@cheney.net> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=951fbbb0a477&repo=crypto ***
>
> go.crypto/ssh: prevent channel writes after Close
>
> Fixes issue 3810.
>
> This change introduces an atomic boolean to guard the close
> of the clientChan. Previously the client code was quite
> lax with the ordering of the close messages and could allow
> window adjustment or EOF messages to leak after Close had
> been signaled.
>
> Consolidating the changes to the serverChan will be handled
> in a following CL.
>
> R=agl, kardianos, gustav.paul
> CC=golang-dev
> http://codereview.appspot.com/6405064
>
>
> http://codereview.appspot.com/6405064/