channel: truncate twrite messages based on msize

While there are a few problems around handling of msize, the easiest to address and, arguably, the most problematic is that of Twrite. We now truncate Twrite.Data to the correct length if it will overflow the msize limit negotiated on the session. ErrShortWrite is returned by the `Session.Write` method if written data is truncated. In addition, we now reject incoming messages from `ReadFcall` that overflow the msize. Such messages are probably terminal in practice, but can be detected with the `Overflow` function. Other problems with Twrite/Rread are documented in TODOs here, along with possible solutions. Signed-off-by: Stephen J Day <stephen.day@docker.com>
docker-archive · Nov 15, 2016 · a8d27f2 · a8d27f2
1 parent 529e2b2
commit a8d27f2
Show file tree

Hide file tree

Showing 6 changed files with 279 additions and 6 deletions.
diff --git a/channel.go b/channel.go
@@ -2,15 +2,19 @@ package p9p
 
 import (
 	"bufio"
+	"context"
 	"encoding/binary"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"log"
 	"net"
 	"time"
+)
 
-	"context"
+const (
+	// channelMessageHeaderSize is the overhead for sending the size of a
+	// message on the wire.
+	channelMessageHeaderSize = 4
 )
 
 // Channel defines the operations necessary to implement a 9p message channel
@@ -114,6 +118,9 @@ func (ch *channel) SetMSize(msize int) {
 }
 
 // ReadFcall reads the next message from the channel into fcall.
+//
+// If the incoming message overflows the msize, Overflow(err) will return
+// nonzero with the number of bytes overflowed.
 func (ch *channel) ReadFcall(ctx context.Context, fcall *Fcall) error {
 	select {
 	case <-ctx.Done():
@@ -140,9 +147,7 @@ func (ch *channel) ReadFcall(ctx context.Context, fcall *Fcall) error {
 	}
 
 	if n > len(ch.rdbuf) {
-		// TODO(stevvooe): Make this error detectable and respond with error
-		// message.
-		return fmt.Errorf("message too large for buffer: %v > %v ", n, len(ch.rdbuf))
+		return overflowErr{size: n - len(ch.rdbuf)}
 	}
 
 	// clear out the fcall
@@ -154,6 +159,12 @@ func (ch *channel) ReadFcall(ctx context.Context, fcall *Fcall) error {
 	return nil
 }
 
+// WriteFcall writes the message to the connection.
+//
+// If a message destined for the wire will overflow MSize, an Overflow error
+// may be returned. For Twrite calls, the buffer will simply be truncated to
+// the optimal msize, with the caller detecting this condition with
+// Rwrite.Count.
 func (ch *channel) WriteFcall(ctx context.Context, fcall *Fcall) error {
 	select {
 	case <-ctx.Done():
@@ -172,6 +183,10 @@ func (ch *channel) WriteFcall(ctx context.Context, fcall *Fcall) error {
 		log.Printf("transport: error setting read deadline on %v: %v", ch.conn.RemoteAddr(), err)
 	}
 
+	if err := ch.maybeTruncate(fcall); err != nil {
+		return err
+	}
+
 	p, err := ch.codec.Marshal(fcall)
 	if err != nil {
 		return err
@@ -184,6 +199,62 @@ func (ch *channel) WriteFcall(ctx context.Context, fcall *Fcall) error {
 	return ch.bwr.Flush()
 }
 
+// maybeTruncate will truncate the message to fit into msize on the wire, if
+// possible. If the message cannot be truncated, an error will be returned and
+// the message should not be sent.
+//
+// A nil return value means the message can be sent without
+func (ch *channel) maybeTruncate(fcall *Fcall) error {
+	// If we are going to overflow the msize, we need to truncate the write to
+	// appropriate size or throw an error in all other conditions.
+	size := ch.msgmsize(fcall)
+	if size <= ch.msize {
+		return nil
+	}
+
+	// overflow the msize, including the channel message size fields.
+	overflow := size - ch.msize
+
+	// for certain message types, just remove the extra bytes from the data portion.
+	switch msg := fcall.Message.(type) {
+	// TODO(stevvooe): There are two more problematic message types:
+	//
+	// Tread: We can rewrite msg.Count so that a return message will be
+	// under msize. This is more defensive than anything but will ensure
+	// that calls don't fail on sloppy servers.
+	//
+	// Rread: while we can employ the same truncation fix as Twrite, we
+	// need to make it observable to upstream handlers. On the server side,
+	// this can likely be done in server.go or in dispatch by intercepting
+	// Tread.
+
+	case MessageTwrite:
+		if len(msg.Data) < overflow {
+			// paranoid: if msg.Data is not big enough to handle the
+			// overflow, we should get an overflow error. MSize would have
+			// to be way too small to be realistic.
+			break
+		}
+
+		// The truncation is reflected in the return message (Rwrite) by
+		// the server, so we don't need a return value or error condition
+		// to communicate it.
+		msg.Data = msg.Data[:len(msg.Data)-overflow]
+		fcall.Message = msg // since we have a local copy
+
+		return nil
+	}
+
+	return overflowErr{size: overflow}
+}
+
+// msgmsize returns the on-wire msize of the Fcall, including the size header.
+// Typically, this can be used to detect whether or not the message overflows
+// the msize buffer.
+func (ch *channel) msgmsize(fcall *Fcall) int {
+	return channelMessageHeaderSize + ch.codec.Size(fcall)
+}
+
 // readmsg reads a 9p message into p from rd, ensuring that all bytes are
 // consumed from the size header. If the size header indicates the message is
 // larger than p, the entire message will be discarded, leaving a truncated

diff --git a/channel_test.go b/channel_test.go
@@ -0,0 +1,134 @@
+package p9p
+
+import (
+	"bytes"
+	"context"
+	"encoding/binary"
+	"net"
+	"testing"
+	"time"
+)
+
+// TestWriteOverflow ensures that a Twrite message will have the data field
+// truncated if the msize would be exceeded.
+func TestWriteOverflow(t *testing.T) {
+	const (
+		msize         = 512
+		overflowMSize = msize * 3 / 2
+	)
+
+	var (
+		ctx   = context.Background()
+		conn  = &mockConn{}
+		ch    = NewChannel(conn, msize)
+		data  = bytes.Repeat([]byte{'A'}, overflowMSize)
+		fcall = newFcall(1, MessageTwrite{
+			Data: data,
+		})
+		messageSize uint32
+	)
+
+	if err := ch.WriteFcall(ctx, fcall); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := binary.Read(bytes.NewReader(conn.buf.Bytes()), binary.LittleEndian, &messageSize); err != nil {
+		t.Fatal(err)
+	}
+
+	if messageSize != msize {
+		t.Fatalf("should have truncated size header: %d != %d", messageSize, msize)
+	}
+
+	if conn.buf.Len() != msize {
+		t.Fatalf("should have truncated message: conn.buf.Len(%v) != msize(%v)", conn.buf.Len(), msize)
+	}
+}
+
+// TestWriteOverflowError ensures that we return an error in cases when there
+// will certainly be an overflow and it cannot be resolved.
+func TestWriteOverflowError(t *testing.T) {
+	const (
+		msize         = 4
+		overflowMSize = msize + 1
+	)
+
+	var (
+		ctx   = context.Background()
+		conn  = &mockConn{}
+		ch    = NewChannel(conn, msize)
+		data  = bytes.Repeat([]byte{'A'}, 4)
+		fcall = newFcall(1, MessageTwrite{
+			Data: data,
+		})
+		messageSize = 4 + ch.(*channel).codec.Size(fcall)
+	)
+
+	err := ch.WriteFcall(ctx, fcall)
+	if err == nil {
+		t.Fatal("error expected when overflowing message")
+	}
+
+	if Overflow(err) != messageSize-msize {
+		t.Fatalf("overflow should reflect messageSize and msize, %d != %d", Overflow(err), messageSize-msize)
+	}
+}
+
+// TestReadOverflow ensures that messages coming over a network connection do
+// not overflow the msize. Invalid messages will cause `ReadFcall` to return an
+// Overflow error.
+func TestReadOverflow(t *testing.T) {
+	const (
+		msize         = 256
+		overflowMSize = msize + 1
+	)
+
+	var (
+		ctx   = context.Background()
+		conn  = &mockConn{}
+		ch    = NewChannel(conn, msize)
+		data  = bytes.Repeat([]byte{'A'}, overflowMSize)
+		fcall = newFcall(1, MessageTwrite{
+			Data: data,
+		})
+		messageSize = 4 + ch.(*channel).codec.Size(fcall)
+	)
+
+	// prepare the raw message
+	p, err := ch.(*channel).codec.Marshal(fcall)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// "send" the message into the buffer
+	// this message is crafted to overflow the read buffer.
+	if err := sendmsg(&conn.buf, p); err != nil {
+		t.Fatal(err)
+	}
+
+	var incoming Fcall
+	err = ch.ReadFcall(ctx, &incoming)
+	if err == nil {
+		t.Fatal("expected error on fcall")
+	}
+
+	if Overflow(err) != messageSize-msize {
+		t.Fatalf("unexpected overflow on error: %v !=%v", Overflow(err), messageSize-msize)
+	}
+}
+
+type mockConn struct {
+	net.Conn
+	buf bytes.Buffer
+}
+
+func (m mockConn) SetWriteDeadline(t time.Time) error { return nil }
+func (m mockConn) SetReadDeadline(t time.Time) error  { return nil }
+
+func (m *mockConn) Write(p []byte) (int, error) {
+	return m.buf.Write(p)
+}
+
+func (m *mockConn) Read(p []byte) (int, error) {
+	return m.buf.Read(p)
+}
diff --git a/client.go b/client.go
@@ -1,6 +1,7 @@
 package p9p
 
 import (
+	"io"
 	"net"
 
 	"context"
@@ -167,7 +168,11 @@ func (c *client) Write(ctx context.Context, fid Fid, p []byte, offset int64) (n
 		return 0, ErrUnexpectedMsg
 	}
 
-	return int(rwrite.Count), nil
+	if int(rwrite.Count) < len(p) {
+		err = io.ErrShortWrite
+	}
+
+	return int(rwrite.Count), err
 }
 
 func (c *client) Open(ctx context.Context, fid Fid, mode Flag) (Qid, uint32, error) {

diff --git a/overflow.go b/overflow.go
@@ -0,0 +1,49 @@
+package p9p
+
+import "fmt"
+
+// Overflow will return a positive number, indicating there was an overflow for
+// the error.
+func Overflow(err error) int {
+	if of, ok := err.(overflow); ok {
+		return of.Size()
+	}
+
+	// traverse cause, if above fails.
+	if causal, ok := err.(interface {
+		Cause() error
+	}); ok {
+		return Overflow(causal.Cause())
+	}
+
+	return 0
+}
+
+// overflow is a resolvable error type that can help callers negotiate
+// session msize. If this error is encountered, no message was sent.
+//
+// The return value of `Size()` represents the number of bytes that would have
+// been truncated if the message were sent. This IS NOT the optimal buffer size
+// for operations like read and write.
+//
+// In the case of `Twrite`, the caller can Size() from the local size to get an
+// optimally size buffer or the write can simply be truncated to `len(buf) -
+// err.Size()`.
+//
+// For the most part, no users of this package should see this error in
+// practice. If this escapes the Session interface, it is a bug.
+type overflow interface {
+	Size() int // number of bytes overflowed.
+}
+
+type overflowErr struct {
+	size int // number of bytes overflowed
+}
+
+func (o overflowErr) Error() string {
+	return fmt.Sprintf("message overflowed %d bytes", o.size)
+}
+
+func (o overflowErr) Size() int {
+	return o.size
+}
diff --git a/server.go b/server.go
@@ -132,6 +132,9 @@ func (c *conn) serve() error {
 				}
 
 				go func(ctx context.Context, req *Fcall) {
+					// TODO(stevvooe): Re-write incoming Treads so that handler
+					// can always respond with a message of the correct msize.
+
 					var resp *Fcall
 					msg, err := c.handler.Handle(ctx, req.Message)
 					if err != nil {
@@ -208,6 +211,11 @@ func (c *conn) write(responses chan *Fcall) {
 	for {
 		select {
 		case resp := <-responses:
+			// TODO(stevvooe): Correctly protect againt overflowing msize from
+			// handler. This can be done above, in the main message handler
+			// loop, by adjusting incoming Tread calls to have a Count that
+			// won't overflow the msize.
+
 			if err := c.ch.WriteFcall(c.ctx, resp); err != nil {
 				if err, ok := err.(net.Error); ok {
 					if err.Timeout() || err.Temporary() {

diff --git a/session.go b/session.go
@@ -20,7 +20,13 @@ type Session interface {
 	Remove(ctx context.Context, fid Fid) error
 	Walk(ctx context.Context, fid Fid, newfid Fid, names ...string) ([]Qid, error)
 	Read(ctx context.Context, fid Fid, p []byte, offset int64) (n int, err error)
+
+	// Write follows the semantics of io.Writer except takes a context and an Fid.
+	//
+	// If n == len(p), no error is returned.
+	// If n < len(p), io.ErrShortWrite will be returned.
 	Write(ctx context.Context, fid Fid, p []byte, offset int64) (n int, err error)
+
 	Open(ctx context.Context, fid Fid, mode Flag) (Qid, uint32, error)
 	Create(ctx context.Context, parent Fid, name string, perm uint32, mode Flag) (Qid, uint32, error)
 	Stat(ctx context.Context, fid Fid) (Dir, error)