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. 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 · Oct 26, 2016 · 9c7316b · 9c7316b
1 parent f717cf6
commit 9c7316b
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 1 deletion.
diff --git a/channel.go b/channel.go
@@ -13,6 +13,12 @@ import (
 	"golang.org/x/net/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
 // interface. Typically, message channels do no protocol processing except to
 // send and receive message frames.
@@ -154,6 +160,41 @@ func (ch *channel) ReadFcall(ctx context.Context, fcall *Fcall) error {
 	return nil
 }
 
+// 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
+}
+
+// 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 +213,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 +229,60 @@ 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]
+		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/client.go b/client.go
@@ -1,6 +1,7 @@
 package p9p
 
 import (
+	"io"
 	"net"
 
 	"golang.org/x/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 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/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)