code review 5970056: go.crypto/ssh: fix locking and corruption issues with s...

go.crypto/ssh: fix locking and corruption issues with server.
Fixes issue 3204.

[kardia, 2012-03-31 21:52:12]

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

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

[kardia, 2012-03-31 22:10:01]

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

Please take another look.

[kardia, 2012-03-31 22:10:17]

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

Please take another look.

[dave_cheney.net, 2012-04-01 07:34:10]

Hello,

Thank you for tackling this. The CL you have submitted is very large and it
looks like it combines some source cleanups with some additions which is hard
for me to follow. Could I encourage you to submit a cleanup CL first (reordering
functions, theirId -> remoteId), which will make this more complicated change
easier to grok.

Cheers

Dave

[kardia, 2012-04-01 15:30:08]

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

Please take another look.

[kardia, 2012-04-01 15:39:51]

I would normally be ready to back you on this.  However, in this case I wasn't
adjusting the existing code; I rewrote the interface function by function, on
occasion taking snippets from the existing code.  I couldn't get the existing
code to fully work by just changing it.  So please ignore the delta and treat
this as new code (which it is).

A few known issues here:
* We are adjusting the window every time we read.  For large batch sessions,
this could ok be but not optimal. For terminal sessions this is less ideal.
Attempts to fix this have not been met with success so far. This should be
addressed in a followup CL.

* The defaultWindowSize const can be adjusted up to ~200000, but values greater
then that causes it to stop functioning correctly.  This is less of a critical
issue, but I think it should still be addressed.  This leads me to believe
something is still not quite right.

On 2012/04/01 07:34:10, dfc wrote:
> Hello,
> 
> Thank you for tackling this. The CL you have submitted is very large and it
> looks like it combines some source cleanups with some additions which is hard
> for me to follow. Could I encourage you to submit a cleanup CL first
(reordering
> functions, theirId -> remoteId), which will make this more complicated change
> easier to grok.
> 
> Cheers
> 
> Dave

[kardia, 2012-04-01 19:32:32]

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

Please take another look.

[kardia, 2012-04-01 19:32:53]

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

Please take another look.

[kardia, 2012-04-01 19:40:38]

I should talk to more people before mailing out a change set.  After writing out
what didn't work, I realized what I could do to make it behave correctly.

As far as I know, this should take care of all of the known issues, as well as
the two I listed below.  Sorry for all the noise in the meantime.

Regarding the defaultWindowSize: currently set at 32768 byte, I noticed the
openssh client has their's at ~2M byte.  Later maybe want a server option to set
the window size?

On 2012/04/01 15:39:51, kardia wrote:
> I would normally be ready to back you on this.  However, in this case I wasn't
> adjusting the existing code; I rewrote the interface function by function, on
> occasion taking snippets from the existing code.  I couldn't get the existing
> code to fully work by just changing it.  So please ignore the delta and treat
> this as new code (which it is).
> 
> A few known issues here:
> * We are adjusting the window every time we read.  For large batch sessions,
> this could ok be but not optimal. For terminal sessions this is less ideal.
> Attempts to fix this have not been met with success so far. This should be
> addressed in a followup CL.
> 
> * The defaultWindowSize const can be adjusted up to ~200000, but values
greater
> then that causes it to stop functioning correctly.  This is less of a critical
> issue, but I think it should still be addressed.  This leads me to believe
> something is still not quite right.
> 
> On 2012/04/01 07:34:10, dfc wrote:
> > Hello,
> > 
> > Thank you for tackling this. The CL you have submitted is very large and it
> > looks like it combines some source cleanups with some additions which is
hard
> > for me to follow. Could I encourage you to submit a cleanup CL first
> (reordering
> > functions, theirId -> remoteId), which will make this more complicated
change
> > easier to grok.
> > 
> > Cheers
> > 
> > Dave

[dave_cheney.net, 2012-04-04 09:52:43]

Hello again,

Sorry to be difficult. Could you please hg mail your CL again against the latest
checkout.

Cheers

Dave

[kardia, 2012-04-04 15:03:12]

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

Please take another look.

[kardia, 2012-04-08 18:05:02]

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

Please take another look.

[kardia, 2012-04-08 18:05:44]

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

Please take another look.

[dave_cheney.net, 2012-04-09 01:32:14]

Hi, thank you for tackling this. I have a few comments below but I'd like to
raise these two points.

1. The circular buffer does a lot of copying, and ends up holding a large
internal buffer while equally large buffers are passed in and out. I believe (I
will confirm) that the buffer returned by transport.ReadPacket are not shared by
anyone else, so it should be safe to construct a linked list of buffers rather
than copying. Do you think this approach is worth investigating ?

2. Referring to the comment on the other ssh CL, I would love to see more code
shared between the client and server. It irks me that they are essentially
separate implementations that share the same package.

http://codereview.appspot.com/5970056/diff/4006/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/5970056/diff/4006/ssh/channel.go#newcode257
ssh/channel.go:257: err :=
c.serverConn.writePacket(marshal(msgChannelOpenConfirm, confirm))
return c.serverConn.writePacket(marshal(msgChannelOpenConfirm, confirm))

http://codereview.appspot.com/5970056/diff/9002/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5970056/diff/9002/ssh/common.go#newcode281
ssh/common.go:281: 
Maybe just rename this buffer, go style encourages shorter names for unexported
types.

http://codereview.appspot.com/5970056/diff/9002/ssh/common.go#newcode284
ssh/common.go:284: lock         *sync.Mutex
lock sync.Mutex, the zero value is usable without initialisation

http://codereview.appspot.com/5970056/diff/9002/ssh/common.go#newcode291
ssh/common.go:291: lock:   new(sync.Mutex),
Drop, see above

[dave_cheney.net, 2012-04-09 01:51:23]

Re: myId, theirId, peersId, localId, remoteId, etc, etc. You make a good case
that this needs to be rationalised. The original myId / peersId came from the
RFC message format. Would anyone like to take a crack at submitting a CL that
standardises on one pair throughout the package? I'm happy to tackle it if there
is interest.

[kardia, 2012-04-09 02:59:28]

On 2012/04/09 01:32:14, dfc wrote:
> Hi, thank you for tackling this. I have a few comments below but I'd like to
> raise these two points.
> 
> 1. The circular buffer does a lot of copying, and ends up holding a large
> internal buffer while equally large buffers are passed in and out. I believe
(I
> will confirm) that the buffer returned by transport.ReadPacket are not shared
by
> anyone else, so it should be safe to construct a linked list of buffers rather
> than copying. Do you think this approach is worth investigating ?

Yes, I think this is worth investigating.  From initial look, it doesn't appear
to modify the packet.  But it might be worth documenting this reliance in
transport and server.

In prep for this (possible) change, I removed buffer.Length and buffer.Free as
they were not being used externally and made less sense in that context.

> 2. Referring to the comment on the other ssh CL, I would love to see more code
> shared between the client and server. It irks me that they are essentially
> separate implementations that share the same package.
> 
> http://codereview.appspot.com/5970056/diff/4006/ssh/channel.go
> File ssh/channel.go (right):
> 
> http://codereview.appspot.com/5970056/diff/4006/ssh/channel.go#newcode257
> ssh/channel.go:257: err :=
> c.serverConn.writePacket(marshal(msgChannelOpenConfirm, confirm))
> return c.serverConn.writePacket(marshal(msgChannelOpenConfirm, confirm))

Done.

> http://codereview.appspot.com/5970056/diff/9002/ssh/common.go
> File ssh/common.go (right):
> 
> http://codereview.appspot.com/5970056/diff/9002/ssh/common.go#newcode281
> ssh/common.go:281: 
> Maybe just rename this buffer, go style encourages shorter names for
unexported
> types.

Done.

> http://codereview.appspot.com/5970056/diff/9002/ssh/common.go#newcode284
> ssh/common.go:284: lock         *sync.Mutex
> lock sync.Mutex, the zero value is usable without initialisation
> 
> http://codereview.appspot.com/5970056/diff/9002/ssh/common.go#newcode291
> ssh/common.go:291: lock:   new(sync.Mutex),
> Drop, see above

Done.

[kardia, 2012-04-09 03:00:00]

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

Please take another look.

[kardia, 2012-04-09 03:20:56]

On 2012/04/09 01:51:23, dfc wrote:
> Re: myId, theirId, peersId, localId, remoteId, etc, etc. You make a good case
> that this needs to be rationalised. The original myId / peersId came from the
> RFC message format. Would anyone like to take a crack at submitting a CL that
> standardises on one pair throughout the package? I'm happy to tackle it if
there
> is interest.

I thinks standardizing on verbs would be nice.  I skimmed through the RFCs,
didn't see my/peer verbs, I think it used recipient/sender, but I don't think
that's general enough.  I like local/peer or local/remote myself. Regardless of
what verbs are decided on, I think it's a good idea.

[kardia, 2012-04-11 07:21:21]

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

Please take another look.

[dave_cheney.net, 2012-04-11 07:29:53]

Sorry for the delay, I have some time this evening to review this CL. 

On 11/04/2012, at 17:21, kardianos@gmail.com wrote:

> Hello golang-dev@googlegroups.com, dave@cheney.net (cc: agl@golang.org,
> golang-dev@googlegroups.com),
> 
> Please take another look.
> 
> 
> http://codereview.appspot.com/5970056/

[kardia, 2012-04-14 15:07:59]

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

Please take another look.

[dave_cheney.net, 2012-04-15 11:31:29]

Hello,

I'm sorry it has taken so long to review this change. Could you please sync it
against the latest commit and mail again.

Cheers

Dave

[kardia, 2012-04-15 15:38:41]

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

Please take another look.

[agl1, 2012-04-15 15:50:59]

This throws a lock of locking into the code without comments about what is
protecting what, nor the locking order.

I can well believe that we have races if the server is reading and writing
concurrently. But I think it would be much more helpful if you point point out
the races (i.e. theirWindow looks like a real one), and they could be addressed
more cleanly.

[kardia, 2012-04-15 20:07:58]

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

Please take another look.

[kardia, 2012-04-15 20:23:59]

I don't think there is any locking order in the code. I just modified the CL to
comment on what each lock protects.

Regarding the current ssh channel code. In some situations it would lock and in
some situations it would corrupt the data, so it may be more then just locking
issues, unless the locking issues were corrupting the data. I did spend several
weeks attempting to fix these issues, but each time not quite fixing it all the
way.

I'm not convinced all the current locking and waiting mechanisms are needed in
this CL, but it appears to work and fixed my immediate need in a production
system (OpenSSH and PUTTY clients).

I don't think I will be able to fix the existing code, as I have already tried
that without success. But let me know if this CL is useful or could be improved.
If someone does work on the existing code, make sure you use the server_test.go
in this CL as a test, also varying the defautlWindowSize const, as that can also
help bring out problems.


On 2012/04/15 15:50:59, agl1 wrote:
> This throws a lock of locking into the code without comments about what is
> protecting what, nor the locking order.
> 
> I can well believe that we have races if the server is reading and writing
> concurrently. But I think it would be much more helpful if you point point out
> the races (i.e. theirWindow looks like a real one), and they could be
addressed
> more cleanly.

[dave_cheney.net, 2012-04-18 06:12:58]

Hello,

Thank you for this CL, sorry it has taken so long to get through it. Some
general comments.

* window size, there is a scary amount of locking going on inside channel.write.
Is it possible to borrow the window type from client.go (move it into common.go)
to encapsulate some of that locking.

* incomingDataBuffer. Last year I write this,
http://codereview.appspot.com/5306079/, which is essentially a buffered
io.Reader, io.Writer pair. Something like this might encapsulate the locking
going on in read and write.

I don't think I have a good understanding on what is going on inside
channel.{read,write} yet.

http://codereview.appspot.com/5970056/diff/31002/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/channel.go#newcode245
ssh/channel.go:245: c.windowLock.RLock()
c.windowLock was unlocked the line above, is this to give other writers a chance
to run ?

http://codereview.appspot.com/5970056/diff/31002/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/common.go#newcode294
ssh/common.go:294: func (c *buffer) Write(data []byte) (n int, err error) {
Does Write need to match the io.Writer contract ? It looks like n is either
len(data) or and 0 and an error. The only place that calls
incomingDataBuffer.Write discards the value of n anyway.

http://codereview.appspot.com/5970056/diff/31002/ssh/common.go#newcode343
ssh/common.go:343: }
Fill only appears to be used as a check to see if there is data available, ie,
the only check is 

        bufferFill := c.incomingDataBuffer.Fill()
        if bufferFill == 0 { ... }

Could this be rewritten as a boolean, something like
 
        if c.incomingDataBuffer.IsEmpty()

http://codereview.appspot.com/5970056/diff/31002/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/server.go#newcode557
ssh/server.go:557: c.incomingDataBuffer = newBuffer(int(defaultWindowSize))
defaultWindowSize is an ideal constant, the int conversion is unnecessary here.

http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go
File ssh/server_test.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go#newcode188
ssh/server_test.go:188: //config.PublicKeyCallback = keyAuth
drop

http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go#newcode245
ssh/server_test.go:245: for {
I took this bit out and the test appeared to pass anyway, I'm not sure what it
is trying to do.

[kardia, 2012-04-18 07:09:10]

Thanks for your comments. For now I've updated with comments and reposes to
yours. Added comments to write and Read functions.

I'll look into the window struct in client. Looks reasonable but would need to
carefully test.


On 2012/04/18 06:12:58, dfc wrote:
> Hello,
> 
> Thank you for this CL, sorry it has taken so long to get through it. Some
> general comments.
> 
> * window size, there is a scary amount of locking going on inside
channel.write.
> Is it possible to borrow the window type from client.go (move it into
common.go)
> to encapsulate some of that locking.
> 
> * incomingDataBuffer. Last year I write this,
> http://codereview.appspot.com/5306079/, which is essentially a buffered
> io.Reader, io.Writer pair. Something like this might encapsulate the locking
> going on in read and write.
> 
> I don't think I have a good understanding on what is going on inside
> channel.{read,write} yet.
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/channel.go
> File ssh/channel.go (right):
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/channel.go#newcode245
> ssh/channel.go:245: c.windowLock.RLock()
> c.windowLock was unlocked the line above, is this to give other writers a
chance
> to run ?
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/common.go
> File ssh/common.go (right):
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/common.go#newcode294
> ssh/common.go:294: func (c *buffer) Write(data []byte) (n int, err error) {
> Does Write need to match the io.Writer contract ? It looks like n is either
> len(data) or and 0 and an error. The only place that calls
> incomingDataBuffer.Write discards the value of n anyway.
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/common.go#newcode343
> ssh/common.go:343: }
> Fill only appears to be used as a check to see if there is data available, ie,
> the only check is 
> 
>         bufferFill := c.incomingDataBuffer.Fill()
>         if bufferFill == 0 { ... }
> 
> Could this be rewritten as a boolean, something like
>  
>         if c.incomingDataBuffer.IsEmpty()
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/server.go
> File ssh/server.go (right):
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/server.go#newcode557
> ssh/server.go:557: c.incomingDataBuffer = newBuffer(int(defaultWindowSize))
> defaultWindowSize is an ideal constant, the int conversion is unnecessary
here.
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go
> File ssh/server_test.go (right):
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go#newcode188
> ssh/server_test.go:188: //config.PublicKeyCallback = keyAuth
> drop
> 
> http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go#newcode245
> ssh/server_test.go:245: for {
> I took this bit out and the test appeared to pass anyway, I'm not sure what it
> is trying to do.

[kardia, 2012-04-18 07:10:16]

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

Please take another look.

[kardia, 2012-04-18 07:10:26]

http://codereview.appspot.com/5970056/diff/31002/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/channel.go#newcode245
ssh/channel.go:245: c.windowLock.RLock()
No, just wasn't paying attention. Thanks.

On 2012/04/18 06:12:58, dfc wrote:
> c.windowLock was unlocked the line above, is this to give other writers a
chance
> to run ?

http://codereview.appspot.com/5970056/diff/31002/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/common.go#newcode294
ssh/common.go:294: func (c *buffer) Write(data []byte) (n int, err error) {
No, it doesn't need to. I matched it because I wanted to verify the amount
written anyway (see bottom if). Removed it for now.

On 2012/04/18 06:12:58, dfc wrote:
> Does Write need to match the io.Writer contract ? It looks like n is either
> len(data) or and 0 and an error. The only place that calls
> incomingDataBuffer.Write discards the value of n anyway.

http://codereview.appspot.com/5970056/diff/31002/ssh/common.go#newcode343
ssh/common.go:343: }
Done.

On 2012/04/18 06:12:58, dfc wrote:
> Fill only appears to be used as a check to see if there is data available, ie,
> the only check is 
> 
>         bufferFill := c.incomingDataBuffer.Fill()
>         if bufferFill == 0 { ... }
> 
> Could this be rewritten as a boolean, something like
>  
>         if c.incomingDataBuffer.IsEmpty()

http://codereview.appspot.com/5970056/diff/31002/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/server.go#newcode557
ssh/server.go:557: c.incomingDataBuffer = newBuffer(int(defaultWindowSize))
Done.

On 2012/04/18 06:12:58, dfc wrote:
> defaultWindowSize is an ideal constant, the int conversion is unnecessary
here.

http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go
File ssh/server_test.go (right):

http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go#newcode188
ssh/server_test.go:188: //config.PublicKeyCallback = keyAuth
Done.

On 2012/04/18 06:12:58, dfc wrote:
> drop

http://codereview.appspot.com/5970056/diff/31002/ssh/server_test.go#newcode245
ssh/server_test.go:245: for {
Was there to handle "exec" or "shell" msgs on other clients, but as we are using
internal client, not needed. Removed.

On 2012/04/18 06:12:58, dfc wrote:
> I took this bit out and the test appeared to pass anyway, I'm not sure what it
> is trying to do.

[agl1, 2012-04-18 22:15:30]

I'm afraid that this is basically a NACK from me. I don't feel that this patch
is identifying the problems. Rather it's rewriting stuff until it works.

I'm going to review the ssh code and find the bugs manually. Hopefully making
much smaller changes to fix them.

Having said that, submitting the test might be very worthwhile.

[agl1, 2012-04-19 00:19:15]

After reading the code I did http://codereview.appspot.com/6049050/

I can import the test from this CL and it passes with that change.

[kardia, 2012-04-19 17:18:15]

*** Abandoned ***