ssh: initialize new channels fully before adding them to chanList

newChannel published the channel into the mux's chanList (via
chanList.add) before assigning its localId, and openChannel/Accept set
maxIncomingPayload only after newChannel returned. For an outbound
channel, whose newChannel runs on an application goroutine, the mux
loop could observe the channel through getChan before those fields
were initialized, reading a zero localId or maxIncomingPayload with no
happens-before relationship.

Assign localId inside add while the chanList lock is held, and set
maxIncomingPayload (always channelMaxPacket) in newChannel itself, so
a channel returned by getChan is always fully initialized.

Change-Id: Ib54a5d10dcb5c1ad8ed4dabe5d8d2790778be298
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/797061
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>

ssh/channel.go

@@ -499,19 +499,20 @@ func (ch *channel) handlePacket(packet []byte) error {
 
 func (m *mux) newChannel(chanType string, direction channelDirection, extraData []byte) *channel {
 	ch := &channel{
-		remoteWin:        window{Cond: newCond()},
-		myWindow:         channelWindowSize,
-		pending:          newBuffer(),
-		extPending:       newBuffer(),
-		direction:        direction,
-		incomingRequests: make(chan *Request, chanSize),
-		msg:              make(chan interface{}, chanSize),
-		chanType:         chanType,
-		extraData:        extraData,
-		mux:              m,
-		packetPool:       make(map[uint32][]byte),
+		remoteWin:          window{Cond: newCond()},
+		myWindow:           channelWindowSize,
+		maxIncomingPayload: channelMaxPacket,
+		pending:            newBuffer(),
+		extPending:         newBuffer(),
+		direction:          direction,
+		incomingRequests:   make(chan *Request, chanSize),
+		msg:                make(chan interface{}, chanSize),
+		chanType:           chanType,
+		extraData:          extraData,
+		mux:                m,
+		packetPool:         make(map[uint32][]byte),
 	}
-	ch.localId = m.chanList.add(ch)
+	m.chanList.add(ch)
 	return ch
 }
 
@@ -535,7 +536,6 @@ func (ch *channel) Accept() (Channel, <-chan *Request, error) {
 	if ch.decided {
 		return nil, nil, errDecidedAlready
 	}
-	ch.maxIncomingPayload = channelMaxPacket
 	confirm := channelOpenConfirmMsg{
 		PeersID:       ch.remoteId,
 		MyID:          ch.localId,

ssh/mux.go

@@ -32,18 +32,21 @@ type chanList struct {
 	offset uint32
 }
 
-// Assigns a channel ID to the given channel.
-func (c *chanList) add(ch *channel) uint32 {
+// add stores the given channel and assigns its localId while holding the
+// lock, so that getChan can never return a channel whose localId is not yet
+// initialized.
+func (c *chanList) add(ch *channel) {
 	c.Lock()
 	defer c.Unlock()
 	for i := range c.chans {
 		if c.chans[i] == nil {
 			c.chans[i] = ch
-			return uint32(i) + c.offset
+			ch.localId = uint32(i) + c.offset
+			return
 		}
 	}
 	c.chans = append(c.chans, ch)
-	return uint32(len(c.chans)-1) + c.offset
+	ch.localId = uint32(len(c.chans)-1) + c.offset
 }
 
 // getChan returns the channel for the given ID.
@@ -343,8 +346,6 @@ func (m *mux) OpenChannel(chanType string, extra []byte) (Channel, <-chan *Reque
 func (m *mux) openChannel(chanType string, extra []byte) (*channel, error) {
 	ch := m.newChannel(chanType, channelOutbound, extra)
 
-	ch.maxIncomingPayload = channelMaxPacket
-
 	open := channelOpenMsg{
 		ChanType:         chanType,
 		PeersWindow:      ch.myWindow,

ssh/mux_test.go

@@ -1225,6 +1225,53 @@ func TestChannelCloseIdempotent(t *testing.T) {
 	ch.close() // must be a no-op, not a panic
 }
 
+func TestNewChannelFullyInitializedWhenPublished(t *testing.T) {
+	a, b := memPipe()
+	defer a.Close()
+	defer b.Close()
+
+	m := newMux(a)
+
+	const channels = 100
+	stop := make(chan struct{})
+	readerDone := make(chan struct{})
+	go func() {
+		// Mimic the mux loop: fetch channels by id and read the fields it
+		// uses to route and validate packets.
+		defer close(readerDone)
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+			}
+			for id := uint32(0); id < channels; id++ {
+				ch := m.chanList.getChan(id)
+				if ch == nil {
+					continue
+				}
+				if got := ch.localId; got != id {
+					t.Errorf("channel registered under id %d has localId %d", id, got)
+					return
+				}
+				if got := ch.maxIncomingPayload; got != channelMaxPacket {
+					t.Errorf("channel %d: maxIncomingPayload = %d, want %d", id, got, channelMaxPacket)
+					return
+				}
+			}
+		}
+	}()
+
+	for range channels {
+		ch := m.newChannel("session", channelOutbound, nil)
+		if m.chanList.getChan(ch.localId) != ch {
+			t.Fatalf("channel %d not registered under its localId", ch.localId)
+		}
+	}
+	close(stop)
+	<-readerDone
+}
+
 func TestChannelConcurrentRequests(t *testing.T) {
 	writer, reader, mux := channelPair(t)
 	defer writer.Close()