From 7f524f24686b7f453e0fd7852319c23198dbb162 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Sat, 18 Feb 2012 13:49:50 -0500 Subject: [PATCH] ssh: fix locking in channel.Write Since a lock is retaken before sync.Cond.Wait returns, this could deadlock when the for loop attempts to take the lock again. (Reported by sanjay.m.) theirWindow was used outside of the lock, therefore concurrent writers could overrun the window. theirWindow was never updated to reflect the data written. R=dave, balasanjay CC=golang-dev https://golang.org/cl/5671084 --- ssh/channel.go | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/ssh/channel.go b/ssh/channel.go index 9d75f37..dacaaec 100644 --- a/ssh/channel.go +++ b/ssh/channel.go @@ -224,22 +224,43 @@ func (c *channel) Read(data []byte) (n int, err error) { panic("unreachable") } -func (c *channel) Write(data []byte) (n int, err error) { - for len(data) > 0 { - c.lock.Lock() +// getWindowSpace takes, at most, max bytes of space from the peer's window. It +// returns the number of bytes actually reserved. +func (c *channel) getWindowSpace(max uint32) (uint32, error) { + c.lock.Lock() + defer c.lock.Unlock() + + for { if c.dead || c.weClosed { return 0, io.EOF } - if c.theirWindow == 0 { - c.cond.Wait() - continue + if c.theirWindow > 0 { + break + } + + c.cond.Wait() + } + + taken := c.theirWindow + if taken > max { + taken = max + } + + c.theirWindow -= taken + return taken, nil +} + +func (c *channel) Write(data []byte) (n int, err error) { + for len(data) > 0 { + var space uint32 + if space, err = c.getWindowSpace(uint32(len(data))); err != nil { + return 0, err } - c.lock.Unlock() todo := data - if uint32(len(todo)) > c.theirWindow { - todo = todo[:c.theirWindow] + if uint32(len(todo)) > space { + todo = todo[:space] } packet := make([]byte, 1+4+4+len(todo))