|
|
Created:
11 years, 12 months ago by dfc Modified:
11 years, 11 months ago Reviewers:
CC:
golang-dev, agl1, gpaul, kardia, dvyukov Visibility:
Public. |
Descriptiongo.crypto/ssh: replace window channel with an atomic variable and condition
Fixes issue 3479.
Using a channel to model window size was a mistake. Unlike stdin and
stdout, which are streams of data, window size is an variable and
should be modeled as such.
Patch Set 1 #Patch Set 2 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #Patch Set 3 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #Patch Set 4 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #Patch Set 5 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #Patch Set 6 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #
Total comments: 4
Patch Set 7 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #
Total comments: 6
Patch Set 8 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #Patch Set 9 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #
Total comments: 6
Patch Set 10 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #Patch Set 11 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #Patch Set 12 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #MessagesTotal messages: 18
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.crypto
Sign in to reply to this message.
LGTM. I haven't been able to reproduce this issue, even when writing >65MB over stdin. Thanks Dave http://codereview.appspot.com/5986053/diff/2002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/2002/ssh/client.go#newcode191 ssh/client.go:191: // The msg, win, data and dataExt channels of a clientChan can s/, win//
Sign in to reply to this message.
LGTM. In another CL, I think in chanReader, we should replace the chan []byte buffer with circular byte buffer, as found in CL 5970056. I'll move the circular byte buffer out of channel.go so the client can also use it. http://codereview.appspot.com/5986053/diff/2002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/2002/ssh/client.go#newcode501 ssh/client.go:501: win int RFC 4254 5.2: Implementations MUST correctly handle window sizes of up to 2^32 - 1 bytes. Make this uint32.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for your comments. I've converted the window size to a uint32, there are probably other cases in the package where this should be done as well. http://codereview.appspot.com/5986053/diff/2002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/2002/ssh/client.go#newcode191 ssh/client.go:191: // The msg, win, data and dataExt channels of a clientChan can On 2012/04/08 15:00:28, gpaul wrote: > s/, win// Done. http://codereview.appspot.com/5986053/diff/2002/ssh/client.go#newcode501 ssh/client.go:501: win int Done. I think there are other places where uint32 should replace int.
Sign in to reply to this message.
LGTM with a couple of updates. http://codereview.appspot.com/5986053/diff/7003/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode257 ssh/client.go:257: c.getChan(msg.PeersId).stdin.win.add(msg.AdditionalBytes) if !c.getChan(msg.PeersId).stdin.win.add(...) { // Invalid window update. break } http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode507 ssh/client.go:507: win uint32 // RFC 4254 5.2 says the window size can grow to 2 ^ 32 -1 spacing: "2^32 - 1" or "2^32-1" http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode514 ssh/client.go:514: w.win += win if (w.win + win < w.win) { return false }
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for your comments agl. PTAL. Gustav, would you be able to reconfirm that this fixes your issue after adding a check for wrap around in the window size. http://codereview.appspot.com/5986053/diff/7003/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode257 ssh/client.go:257: c.getChan(msg.PeersId).stdin.win.add(msg.AdditionalBytes) On 2012/04/09 14:40:06, agl1 wrote: > if !c.getChan(msg.PeersId).stdin.win.add(...) { > // Invalid window update. > break > } Done. http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode507 ssh/client.go:507: win uint32 // RFC 4254 5.2 says the window size can grow to 2 ^ 32 -1 On 2012/04/09 14:40:06, agl1 wrote: > spacing: "2^32 - 1" or "2^32-1" Done. http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode514 ssh/client.go:514: w.win += win On 2012/04/09 14:40:06, agl1 wrote: > if (w.win + win < w.win) { > return false > } Done.
Sign in to reply to this message.
Certainly. I'll let you know by tomorrow. On Sat, Apr 14, 2012 at 5:40 AM, <dave@cheney.net> wrote: > Thank you for your comments agl. PTAL. > > Gustav, would you be able to reconfirm that this fixes your issue after > adding a check for wrap around in the window size. > > > > http://codereview.appspot.com/**5986053/diff/7003/ssh/client.**go<http://code... > File ssh/client.go (right): > > http://codereview.appspot.com/**5986053/diff/7003/ssh/client.** > go#newcode257<http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode257> > ssh/client.go:257: > c.getChan(msg.PeersId).stdin.**win.add(msg.AdditionalBytes) > On 2012/04/09 14:40:06, agl1 wrote: > >> if !c.getChan(msg.PeersId).stdin.**win.add(...) { >> // Invalid window update. >> break >> } >> > > Done. > > > http://codereview.appspot.com/**5986053/diff/7003/ssh/client.** > go#newcode507<http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode507> > ssh/client.go:507: win uint32 // RFC 4254 5.2 says the window size can > grow to 2 ^ 32 -1 > On 2012/04/09 14:40:06, agl1 wrote: > >> spacing: "2^32 - 1" or "2^32-1" >> > > Done. > > > http://codereview.appspot.com/**5986053/diff/7003/ssh/client.** > go#newcode514<http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode514> > ssh/client.go:514: w.win += win > On 2012/04/09 14:40:06, agl1 wrote: > >> if (w.win + win < w.win) { >> return false >> } >> > > Done. > > http://codereview.appspot.com/**5986053/<http://codereview.appspot.com/5986053/> >
Sign in to reply to this message.
http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() Can't reserve() be called concurrently? http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode529 ssh/client.go:529: w.Wait() Can't w.win == 0 after Wait()? I do not see where code checks that add is not called with 0 arg.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com, dvyukov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for your comments Dmity. PTAL. http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() Not quite sure what you mean here. the value of the window should appear atomic. There is typically one consumer of the window, whatever is connected to the stdin of the channel. Should the order of the Signal and Unlock be reversed ? On 2012/04/15 06:51:11, dvyukov wrote: > Can't reserve() be called concurrently? http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode529 ssh/client.go:529: w.Wait() Hmm. Good point I've added a check to window#add to reject 0 sized window updates. On 2012/04/15 06:51:11, dvyukov wrote: > Can't w.win == 0 after Wait()? I do not see where code checks that add is not > called with 0 arg.
Sign in to reply to this message.
http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() On 2012/04/15 07:01:02, dfc wrote: > Not quite sure what you mean here. the value of the window should appear atomic. > There is typically one consumer of the window, whatever is connected to the > stdin of the channel. There is *typically* one consumer? Or just one consumer? :) I just meant if there can possibly be more than 1 consumer, then w.Broadcast() must be used. However, I understand that it's somewhat weird to write concurrently to ssh channel. > Should the order of the Signal and Unlock be reversed ? I think it may be either way here. Signaling under mutex has some preference towards correctness, while signaling outside of mutex has some preference towards performance.
Sign in to reply to this message.
The latest update still fixes the issue: I am able to correctly transfer >64MB. I'm testing against ssh server: 22/tcp open ssh OpenSSH 5.8 (protocol 2.0) On Sun, Apr 15, 2012 at 9:41 AM, <dvyukov@google.com> wrote: > > http://codereview.appspot.com/**5986053/diff/9002/ssh/client.**go<http://code... > File ssh/client.go (right): > > http://codereview.appspot.com/**5986053/diff/9002/ssh/client.** > go#newcode518<http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518> > ssh/client.go:518: w.Signal() > On 2012/04/15 07:01:02, dfc wrote: > >> Not quite sure what you mean here. the value of the window should >> > appear atomic. > >> There is typically one consumer of the window, whatever is connected >> > to the > >> stdin of the channel. >> > > There is *typically* one consumer? Or just one consumer? :) > I just meant if there can possibly be more than 1 consumer, then > w.Broadcast() must be used. However, I understand that it's somewhat > weird to write concurrently to ssh channel. > > > Should the order of the Signal and Unlock be reversed ? >> > > I think it may be either way here. Signaling under mutex has some > preference towards correctness, while signaling outside of mutex has > some preference towards performance. > > http://codereview.appspot.com/**5986053/<http://codereview.appspot.com/5986053/> >
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com, dvyukov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for your review. Please take another (final?) look. http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() There should only be one goroutine calling Write, but the write lock isn't enforced until the request gets to the transport. I'll change it to Broadcast to be safe.
Sign in to reply to this message.
FWIW LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=b42815235d16&repo=crypto *** go.crypto/ssh: replace window channel with an atomic variable and condition Fixes issue 3479. Using a channel to model window size was a mistake. Unlike stdin and stdout, which are streams of data, window size is an variable and should be modeled as such. R=golang-dev, agl, gustav.paul, kardianos, dvyukov CC=golang-dev http://codereview.appspot.com/5986053
Sign in to reply to this message.
|