code review 5433063: exp/ssh: messages now contain remote channel's id inste...

exp/ssh: messages now contain remote channel's id instead of local id

According to http://www.ietf.org/rfc/rfc4254.txt most channel messages contain the channel id of the recipient channel, not the sender id. This allows the recipient connection multiplexer to route the message to the correct channel.

This changeset fixes several messages that incorrectly send the local channel id instead of the remote channel's id.

While sessions were being created and closed in sequence channels in the channel pool were freed and reused on the server side of the connection at the same rate as was done on the client, so the channel local and remote channel ids always corresponded. As soon as I had concurrent sessions on the same clientConn the server started to complain of 'uknown channel id N' where N is the local channel id, which is actually paired with server channel id K.

[gpaul, 2011-11-24 07:37:12]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2011-11-24 09:26:58]

Hi Gustav,

Thank you for submitting this patch, I think you have identified a serious
issue. If I add a few 

conn.newChan(conn.transport) 

calls into client.go:line36 to seed the chanlist then I can recreate your error.


I'll reply to your CL when I get back in WiFi range. 


Sent from my iPhone

On 24/11/2011, at 18:37, gustav.paul@gmail.com wrote:

> Reviewers: golang-dev_googlegroups.com, dfc, rsc, agl1,
> 
> Message:
> Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org,
> agl@golang.org (cc: golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/
> 
> 
> Description:
> exp/ssh: messages now contain remote channel's id instead of local id
> 
> According to http://www.ietf.org/rfc/rfc4254.txt most channel messages
> contain the channel id of the recipient channel, not the sender id. This
> allows the recipient connection multiplexer to route the message to the
> correct channel.
> 
> This changeset fixes several messages that incorrectly send the local
> channel id instead of the remote channel's id.
> 
> While sessions were being created and closed in sequence channels in the
> channel pool were freed and reused on the server side of the connection
> at the same rate as was done on the client, so the channel local and
> remote channel ids always corresponded. As soon as I had concurrent
> sessions on the same clientConn the server started to complain of
> 'uknown channel id N' where N is the local channel id, which is actually
> paired with server channel id K.
> 
> Please review this at http://codereview.appspot.com/5433063/
> 
> Affected files:
>  M src/pkg/exp/ssh/client.go
>  M src/pkg/exp/ssh/session.go
>  M src/pkg/exp/ssh/tcpip.go
> 
> 
> Index: src/pkg/exp/ssh/client.go
> ===================================================================
> --- a/src/pkg/exp/ssh/client.go
> +++ b/src/pkg/exp/ssh/client.go
> @@ -338,7 +338,7 @@
> // Close closes the channel. This does not close the underlying connection.
> func (c *clientChan) Close() error {
>    return c.writePacket(marshal(msgChannelClose, channelCloseMsg{
> -        PeersId: c.id,
> +        PeersId: c.peersId,
>    }))
> }
> 
> @@ -385,6 +385,7 @@
> type chanWriter struct {
>    win          chan int // receives window adjustments
>    id           uint32   // this channel's id
> +    peersId      uint32   // the server channel's id
>    rwin         int      // current rwin size
>    packetWriter          // for sending channelDataMsg
> }
> @@ -403,7 +404,7 @@
>        n = len(data)
>        packet := make([]byte, 0, 9+n)
>        packet = append(packet, msgChannelData,
> -            byte(w.id)>>24, byte(w.id)>>16, byte(w.id)>>8, byte(w.id),
> +            byte(w.peersId)>>24, byte(w.peersId)>>16, byte(w.peersId)>>8,
byte(w.peersId),
>            byte(n)>>24, byte(n)>>16, byte(n)>>8, byte(n))
>        err = w.writePacket(append(packet, data...))
>        w.rwin -= n
> @@ -413,7 +414,7 @@
> }
> 
> func (w *chanWriter) Close() error {
> -    return w.writePacket(marshal(msgChannelEOF, channelEOFMsg{w.id}))
> +    return w.writePacket(marshal(msgChannelEOF, channelEOFMsg{w.peersId}))
> }
> 
> // A chanReader represents stdout or stderr of a remote process.
> @@ -422,8 +423,9 @@
>    // If writes to this channel block, they will block mainLoop, making
>    // it unable to receive new messages from the remote side.
>    data         chan []byte // receives data from remote
> -    id           uint32
> -    packetWriter // for sending windowAdjustMsg
> +    id           uint32      // this channel's id
> +    peersId      uint32      // the server channel's id
> +    packetWriter             // for sending windowAdjustMsg
>    buf          []byte
> }
> 
> @@ -435,7 +437,7 @@
>            n := copy(data, r.buf)
>            r.buf = r.buf[n:]
>            msg := windowAdjustMsg{
> -                PeersId:         r.id,
> +                PeersId:         r.peersId,
>                AdditionalBytes: uint32(n),
>            }
>            return n, r.writePacket(marshal(msgChannelWindowAdjust, msg))
> @@ -449,5 +451,5 @@
> }
> 
> func (r *chanReader) Close() error {
> -    return r.writePacket(marshal(msgChannelEOF, channelEOFMsg{r.id}))
> +    return r.writePacket(marshal(msgChannelEOF, channelEOFMsg{r.peersId}))
> }
> Index: src/pkg/exp/ssh/session.go
> ===================================================================
> --- a/src/pkg/exp/ssh/session.go
> +++ b/src/pkg/exp/ssh/session.go
> @@ -53,7 +53,7 @@
> // command executed by Shell or Exec.
> func (s *Session) Setenv(name, value string) error {
>    req := setenvRequest{
> -        PeersId:   s.id,
> +        PeersId:   s.peersId,
>        Request:   "env",
>        WantReply: true,
>        Name:      name,
> @@ -84,7 +84,7 @@
> // RequestPty requests the association of a pty with the session on the remote
host.
> func (s *Session) RequestPty(term string, h, w int) error {
>    req := ptyRequestMsg{
> -        PeersId:   s.id,
> +        PeersId:   s.peersId,
>        Request:   "pty-req",
>        WantReply: true,
>        Term:      term,
> @@ -116,7 +116,7 @@
>        return errors.New("ssh: session already started")
>    }
>    req := execMsg{
> -        PeersId:   s.id,
> +        PeersId:   s.peersId,
>        Request:   "exec",
>        WantReply: true,
>        Command:   cmd,
> @@ -140,7 +140,7 @@
>        return errors.New("ssh: session already started")
>    }
>    req := channelRequestMsg{
> -        PeersId:   s.id,
> +        PeersId:   s.peersId,
>        Request:   "shell",
>        WantReply: true,
>    }
> Index: src/pkg/exp/ssh/tcpip.go
> ===================================================================
> --- a/src/pkg/exp/ssh/tcpip.go
> +++ b/src/pkg/exp/ssh/tcpip.go
> @@ -87,11 +87,13 @@
>        Reader: &chanReader{
>            packetWriter: ch,
>            id:           ch.id,
> +            peersId:      ch.peersId,
>            data:         ch.data,
>        },
>        Writer: &chanWriter{
>            packetWriter: ch,
>            id:           ch.id,
> +            peersId:      ch.peersId,
>            win:          ch.win,
>        },
>    }, nil
> 
> 

[gpaul, 2011-11-24 09:40:11]

Hi Dave,

Great, glad its not just me.

Thanks for the quick responses,
Gustav

On Thu, Nov 24, 2011 at 11:26 AM, Dave Cheney <dave@cheney.net> wrote:

> Hi Gustav,
>
> Thank you for submitting this patch, I think you have identified a serious
> issue. If I add a few
>
> conn.newChan(conn.transport)
>
> calls into client.go:line36 to seed the chanlist then I can recreate your
> error.
>
> I'll reply to your CL when I get back in WiFi range.
>
>
> Sent from my iPhone
>
> On 24/11/2011, at 18:37, gustav.paul@gmail.com wrote:
>
> > Reviewers: golang-dev_googlegroups.com, dfc, rsc, agl1,
> >
> > Message:
> > Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org,
> > agl@golang.org (cc: golang-dev@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> >
> >
> > Description:
> > exp/ssh: messages now contain remote channel's id instead of local id
> >
> > According to http://www.ietf.org/rfc/rfc4254.txt most channel messages
> > contain the channel id of the recipient channel, not the sender id. This
> > allows the recipient connection multiplexer to route the message to the
> > correct channel.
> >
> > This changeset fixes several messages that incorrectly send the local
> > channel id instead of the remote channel's id.
> >
> > While sessions were being created and closed in sequence channels in the
> > channel pool were freed and reused on the server side of the connection
> > at the same rate as was done on the client, so the channel local and
> > remote channel ids always corresponded. As soon as I had concurrent
> > sessions on the same clientConn the server started to complain of
> > 'uknown channel id N' where N is the local channel id, which is actually
> > paired with server channel id K.
> >
> > Please review this at http://codereview.appspot.com/5433063/
> >
> > Affected files:
> >  M src/pkg/exp/ssh/client.go
> >  M src/pkg/exp/ssh/session.go
> >  M src/pkg/exp/ssh/tcpip.go
> >
> >
> > Index: src/pkg/exp/ssh/client.go
> > ===================================================================
> > --- a/src/pkg/exp/ssh/client.go
> > +++ b/src/pkg/exp/ssh/client.go
> > @@ -338,7 +338,7 @@
> > // Close closes the channel. This does not close the underlying
> connection.
> > func (c *clientChan) Close() error {
> >    return c.writePacket(marshal(msgChannelClose, channelCloseMsg{
> > -        PeersId: c.id,
> > +        PeersId: c.peersId,
> >    }))
> > }
> >
> > @@ -385,6 +385,7 @@
> > type chanWriter struct {
> >    win          chan int // receives window adjustments
> >    id           uint32   // this channel's id
> > +    peersId      uint32   // the server channel's id
> >    rwin         int      // current rwin size
> >    packetWriter          // for sending channelDataMsg
> > }
> > @@ -403,7 +404,7 @@
> >        n = len(data)
> >        packet := make([]byte, 0, 9+n)
> >        packet = append(packet, msgChannelData,
> > -            byte(w.id)>>24, byte(w.id)>>16, byte(w.id)>>8, byte(w.id),
> > +            byte(w.peersId)>>24, byte(w.peersId)>>16,
> byte(w.peersId)>>8, byte(w.peersId),
> >            byte(n)>>24, byte(n)>>16, byte(n)>>8, byte(n))
> >        err = w.writePacket(append(packet, data...))
> >        w.rwin -= n
> > @@ -413,7 +414,7 @@
> > }
> >
> > func (w *chanWriter) Close() error {
> > -    return w.writePacket(marshal(msgChannelEOF, channelEOFMsg{w.id}))
> > +    return w.writePacket(marshal(msgChannelEOF,
> channelEOFMsg{w.peersId}))
> > }
> >
> > // A chanReader represents stdout or stderr of a remote process.
> > @@ -422,8 +423,9 @@
> >    // If writes to this channel block, they will block mainLoop, making
> >    // it unable to receive new messages from the remote side.
> >    data         chan []byte // receives data from remote
> > -    id           uint32
> > -    packetWriter // for sending windowAdjustMsg
> > +    id           uint32      // this channel's id
> > +    peersId      uint32      // the server channel's id
> > +    packetWriter             // for sending windowAdjustMsg
> >    buf          []byte
> > }
> >
> > @@ -435,7 +437,7 @@
> >            n := copy(data, r.buf)
> >            r.buf = r.buf[n:]
> >            msg := windowAdjustMsg{
> > -                PeersId:         r.id,
> > +                PeersId:         r.peersId,
> >                AdditionalBytes: uint32(n),
> >            }
> >            return n, r.writePacket(marshal(msgChannelWindowAdjust, msg))
> > @@ -449,5 +451,5 @@
> > }
> >
> > func (r *chanReader) Close() error {
> > -    return r.writePacket(marshal(msgChannelEOF, channelEOFMsg{r.id}))
> > +    return r.writePacket(marshal(msgChannelEOF,
> channelEOFMsg{r.peersId}))
> > }
> > Index: src/pkg/exp/ssh/session.go
> > ===================================================================
> > --- a/src/pkg/exp/ssh/session.go
> > +++ b/src/pkg/exp/ssh/session.go
> > @@ -53,7 +53,7 @@
> > // command executed by Shell or Exec.
> > func (s *Session) Setenv(name, value string) error {
> >    req := setenvRequest{
> > -        PeersId:   s.id,
> > +        PeersId:   s.peersId,
> >        Request:   "env",
> >        WantReply: true,
> >        Name:      name,
> > @@ -84,7 +84,7 @@
> > // RequestPty requests the association of a pty with the session on the
> remote host.
> > func (s *Session) RequestPty(term string, h, w int) error {
> >    req := ptyRequestMsg{
> > -        PeersId:   s.id,
> > +        PeersId:   s.peersId,
> >        Request:   "pty-req",
> >        WantReply: true,
> >        Term:      term,
> > @@ -116,7 +116,7 @@
> >        return errors.New("ssh: session already started")
> >    }
> >    req := execMsg{
> > -        PeersId:   s.id,
> > +        PeersId:   s.peersId,
> >        Request:   "exec",
> >        WantReply: true,
> >        Command:   cmd,
> > @@ -140,7 +140,7 @@
> >        return errors.New("ssh: session already started")
> >    }
> >    req := channelRequestMsg{
> > -        PeersId:   s.id,
> > +        PeersId:   s.peersId,
> >        Request:   "shell",
> >        WantReply: true,
> >    }
> > Index: src/pkg/exp/ssh/tcpip.go
> > ===================================================================
> > --- a/src/pkg/exp/ssh/tcpip.go
> > +++ b/src/pkg/exp/ssh/tcpip.go
> > @@ -87,11 +87,13 @@
> >        Reader: &chanReader{
> >            packetWriter: ch,
> >            id:           ch.id,
> > +            peersId:      ch.peersId,
> >            data:         ch.data,
> >        },
> >        Writer: &chanWriter{
> >            packetWriter: ch,
> >            id:           ch.id,
> > +            peersId:      ch.peersId,
> >            win:          ch.win,
> >        },
> >    }, nil
> >
> >
>

[dave_cheney.net, 2011-11-24 15:18:27]

LGTM (with comments), leaving for rsc and agl.

If you haven't already done so, you should complete the Google CLA agreement.

Have a think about some possibly changes you could make to the test cases the
catch this. I've simulated the error by throwing a few 

        conn.newChan(conn.transport)

lines into client.go:Client, to throw the id's between the local and remote out
of sync. However, please don't delay this CL, you can submit a later one with
the test cases.

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go
File src/pkg/exp/ssh/client.go (right):

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:341: PeersId: c.peersId,
boy is my face red.

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:387: id           uint32   // this channel's id
Drop id

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:388: peersId      uint32   // the server channel's id
the peers id.

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:426: id           uint32      // this channel's id
drop id

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:427: peersId      uint32      // the server channel's
id
see above

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:455: }
nit: you can delete this method if you like. It is not currently used.

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/tcpip.go
File src/pkg/exp/ssh/tcpip.go (right):

http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/tcpip.go#newc...
src/pkg/exp/ssh/tcpip.go:89: id:           ch.id,
drop id, and again below.

[gpaul, 2011-11-24 15:21:32]

Will do, I'm about to leave for home but I'll modify it first thing in the
morning.

Gustav

On Thu, Nov 24, 2011 at 5:18 PM, <dave@cheney.net> wrote:

> LGTM (with comments), leaving for rsc and agl.
>
> If you haven't already done so, you should complete the Google CLA
> agreement.
>
> Have a think about some possibly changes you could make to the test
> cases the catch this. I've simulated the error by throwing a few
>
>        conn.newChan(conn.transport)
>
> lines into client.go:Client, to throw the id's between the local and
> remote out of sync. However, please don't delay this CL, you can submit
> a later one with the test cases.
>
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go>
> File src/pkg/exp/ssh/client.go (right):
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go#newcode341<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#newcode341>
> src/pkg/exp/ssh/client.go:341: PeersId: c.peersId,
> boy is my face red.
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go#newcode387<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#newcode387>
> src/pkg/exp/ssh/client.go:387: id           uint32   // this channel's
> id
> Drop id
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go#newcode388<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#newcode388>
> src/pkg/exp/ssh/client.go:388: peersId      uint32   // the server
> channel's id
> the peers id.
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go#newcode426<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#newcode426>
> src/pkg/exp/ssh/client.go:426: id           uint32      // this
> channel's id
> drop id
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go#newcode427<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#newcode427>
> src/pkg/exp/ssh/client.go:427: peersId      uint32      // the server
> channel's id
> see above
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/client.go#newcode455<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/client.go#newcode455>
> src/pkg/exp/ssh/client.go:455: }
> nit: you can delete this method if you like. It is not currently used.
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/tcpip.go<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/tcpip.go>
> File src/pkg/exp/ssh/tcpip.go (right):
>
> http://codereview.appspot.com/**5433063/diff/2002/src/pkg/exp/**
>
ssh/tcpip.go#newcode89<http://codereview.appspot.com/5433063/diff/2002/src/pkg/exp/ssh/tcpip.go#newcode89>
> src/pkg/exp/ssh/tcpip.go:89: id:           ch.id,
> drop id, and again below.
>
>
http://codereview.appspot.com/**5433063/<http://codereview.appspot.com/5433063/>
>

[agl1, 2011-11-24 19:33:02]

On Thu, Nov 24, 2011 at 10:21 AM, Gustav Paul <gustav.paul@gmail.com> wrote:
> Will do, I'm about to leave for home but I'll modify it first thing in the
> morning.

(Waiting for contributor agreement before landing.)


AGL

[gpaul, 2011-11-25 08:55:46]

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

Please take another look.

[gpaul, 2011-11-25 09:00:29]

I just signed the Google CLA. It said 'processing...' which I'm assuming really
means 'done.'

[bradfitz, 2011-11-27 14:59:17]

*** Submitted as http://code.google.com/p/go/source/detail?r=0a947e1cd2e2 ***

exp/ssh: messages now contain remote channel's id instead of local id

According to http://www.ietf.org/rfc/rfc4254.txt most channel messages contain
the channel id of the recipient channel, not the sender id. This allows the
recipient connection multiplexer to route the message to the correct channel.

This changeset fixes several messages that incorrectly send the local channel id
instead of the remote channel's id.

While sessions were being created and closed in sequence channels in the channel
pool were freed and reused on the server side of the connection at the same rate
as was done on the client, so the channel local and remote channel ids always
corresponded. As soon as I had concurrent sessions on the same clientConn the
server started to complain of 'uknown channel id N' where N is the local channel
id, which is actually paired with server channel id K.

R=golang-dev, dave, rsc, agl
CC=golang-dev
http://codereview.appspot.com/5433063

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[rsc, 2011-11-28 15:24:30]

It seems like there is a crucial apostrophe missing
in "the peers id" unless this is an id denoting a set
of peers.

[gpaul, 2011-11-28 15:26:12]

Indeed! It refers to a single peer. What is the process for ammending the
CL?

On Mon, Nov 28, 2011 at 5:24 PM, Russ Cox <rsc@golang.org> wrote:

> It seems like there is a crucial apostrophe missing
> in "the peers id" unless this is an id denoting a set
> of peers.
>

[bradfitz, 2011-11-28 15:31:34]

On Mon, Nov 28, 2011 at 10:26 AM, Gustav Paul <gustav.paul@gmail.com> wrote:

> Indeed! It refers to a single peer. What is the process for ammending the
> CL?


Sending another CL.

There's no "amending", per se.