code review 6210063: go.crypto/ssh: fix panic unmarshalling channelOpenFailureMsg

go.crypto/ssh: fix panic unmarshalling channelOpenFailureMsg

I introduced this problem in change set 33 when I redefined
RejectionReason from a uint32 to an int. I have added a test
to verify the message can be marshaled correctly.

This was a bit hard to track down as the recover in clientConn
mainLoop would catch the panic and shutdown the connection. We
probably shouldn't be using panic inside marshal/unmarshal, at
least not without a way to let the user know why the recover
logic tripped.

[dave_cheney.net, 2012-05-17 02:19:00]

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

[gpaul, 2012-05-17 05:17:24]

LGTM.

I stumbled across the panic -> drop connection logic recently when my code
intermittently complained that I was closing an already closed connection.

I'm not sure an error in mainLoop should close the underlying tcp connection, at
least not if there is no way to interrogate the ClientConn to ask whether the
connection is still active.

[dave_cheney.net, 2012-05-17 05:26:06]

Let's wait for agl. If we can find another way to avoid panic'ing on a bad
id supplied from the server, then the recover logic might be retired. Feel
free to propose a CL that does this.


On Thu, May 17, 2012 at 3:17 PM, <gustav.paul@gmail.com> wrote:

> LGTM.
>
> I stumbled across the panic -> drop connection logic recently when my
> code intermittently complained that I was closing an already closed
> connection.
>
> I'm not sure an error in mainLoop should close the underlying tcp
> connection, at least not if there is no way to interrogate the
> ClientConn to ask whether the connection is still active.
>
>
http://codereview.appspot.com/**6210063/<http://codereview.appspot.com/6210063/>
>

[agl1, 2012-05-17 14:25:24]

LGTM.

I added the recover because I felt bad adding lots of error checking to the
client code. However, if it's causing problems then it can certainly be
replaced.

[kardia, 2012-05-17 18:31:17]

LGTM

[dave_cheney.net, 2012-05-17 23:11:39]

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

go.crypto/ssh: fix panic unmarshalling channelOpenFailureMsg

I introduced this problem in change set 33 when I redefined
RejectionReason from a uint32 to an int. I have added a test
to verify the message can be marshaled correctly.

This was a bit hard to track down as the recover in clientConn
mainLoop would catch the panic and shutdown the connection. We
probably shouldn't be using panic inside marshal/unmarshal, at
least not without a way to let the user know why the recover
logic tripped.

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