diff --git a/pkg/abi/linux/file.go b/pkg/abi/linux/file.go index 67646f837..691b16db4 100644 --- a/pkg/abi/linux/file.go +++ b/pkg/abi/linux/file.go @@ -242,7 +242,7 @@ const ( // Statx represents struct statx. // -// +marshal slice:StatxSlice +// +marshal boundCheck slice:StatxSlice type Statx struct { Mask uint32 Blksize uint32 diff --git a/pkg/lisafs/README.md b/pkg/lisafs/README.md index 6b857321a..51d0d40e5 100644 --- a/pkg/lisafs/README.md +++ b/pkg/lisafs/README.md @@ -1,8 +1,5 @@ # Replacing 9P -NOTE: LISAFS is **NOT** production ready. There are still some security concerns -that must be resolved first. - ## Background The Linux filesystem model consists of the following key aspects (modulo mounts, diff --git a/pkg/lisafs/client.go b/pkg/lisafs/client.go index e0f278b5c..933e7a0bb 100644 --- a/pkg/lisafs/client.go +++ b/pkg/lisafs/client.go @@ -101,7 +101,7 @@ func NewClient(sock *unet.Socket, mountPath string) (*Client, *Inode, error) { MountPath: SizedString(mountPath), } var mountResp MountResp - if err := c.SndRcvMessage(Mount, uint32(mountMsg.SizeBytes()), mountMsg.MarshalBytes, mountResp.UnmarshalBytes, nil); err != nil { + if err := c.SndRcvMessage(Mount, uint32(mountMsg.SizeBytes()), mountMsg.MarshalBytes, mountResp.CheckedUnmarshal, nil); err != nil { return nil, nil, err } @@ -223,7 +223,7 @@ func (c *Client) Close() { func (c *Client) createChannel() (*channel, error) { var chanResp ChannelResp var fds [2]int - if err := c.SndRcvMessage(Channel, 0, NoopMarshal, chanResp.UnmarshalUnsafe, fds[:]); err != nil { + if err := c.SndRcvMessage(Channel, 0, NoopMarshal, chanResp.CheckedUnmarshal, fds[:]); err != nil { return nil, err } if fds[0] < 0 || fds[1] < 0 { @@ -313,12 +313,12 @@ func (c *Client) SyncFDs(ctx context.Context, fds []FDID) error { // implicit conversion to an interface leads to an allocation. // // Precondition: reqMarshal and respUnmarshal must be non-nil. -func (c *Client) SndRcvMessage(m MID, payloadLen uint32, reqMarshal func(dst []byte) []byte, respUnmarshal func(src []byte) []byte, respFDs []int) error { +func (c *Client) SndRcvMessage(m MID, payloadLen uint32, reqMarshal func(dst []byte) []byte, respUnmarshal func(src []byte) ([]byte, bool), respFDs []int) error { if !c.IsSupported(m) { return unix.EOPNOTSUPP } if payloadLen > c.maxMessageSize { - log.Warningf("message %d has message size = %d which is larger than client.maxMessageSize = %d", m, payloadLen, c.maxMessageSize) + log.Warningf("message %d has payload which is too large: %d bytes", m, payloadLen) return unix.EIO } wantFDs := len(respFDs) @@ -357,6 +357,11 @@ func (c *Client) SndRcvMessage(m MID, payloadLen uint32, reqMarshal func(dst []b closeFDs(respFDs) return err } + if respPayloadLen > c.maxMessageSize { + log.Warningf("server response for message %d is too large: %d bytes", respM, respPayloadLen) + closeFDs(respFDs) + return unix.EIO + } if respM == Error { closeFDs(respFDs) var resp ErrorResp @@ -370,7 +375,10 @@ func (c *Client) SndRcvMessage(m MID, payloadLen uint32, reqMarshal func(dst []b } // Success. The payload must be unmarshalled *before* comm is released. - respUnmarshal(comm.PayloadBuf(respPayloadLen)) + if _, ok := respUnmarshal(comm.PayloadBuf(respPayloadLen)); !ok { + log.Warningf("server response unmarshalling for %d message failed", respM) + return unix.EIO + } return nil } diff --git a/pkg/lisafs/client_file.go b/pkg/lisafs/client_file.go index 170c15705..7024f0a6c 100644 --- a/pkg/lisafs/client_file.go +++ b/pkg/lisafs/client_file.go @@ -82,7 +82,7 @@ func (f *ClientFD) OpenAt(ctx context.Context, flags uint32) (FDID, int, error) var respFD [1]int var resp OpenAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(OpenAt, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.UnmarshalUnsafe, respFD[:]) + err := f.client.SndRcvMessage(OpenAt, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, respFD[:]) ctx.UninterruptibleSleepFinish(false) return resp.NewFD, respFD[0], err } @@ -100,7 +100,7 @@ func (f *ClientFD) OpenCreateAt(ctx context.Context, name string, flags uint32, var respFD [1]int var resp OpenCreateAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(OpenCreateAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalUnsafe, respFD[:]) + err := f.client.SndRcvMessage(OpenCreateAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, respFD[:]) ctx.UninterruptibleSleepFinish(false) return resp.Child, resp.NewFD, respFD[0], err } @@ -109,7 +109,7 @@ func (f *ClientFD) OpenCreateAt(ctx context.Context, name string, flags uint32, func (f *ClientFD) StatTo(ctx context.Context, stat *linux.Statx) error { req := StatReq{FD: f.fd} ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(FStat, uint32(req.SizeBytes()), req.MarshalUnsafe, stat.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(FStat, uint32(req.SizeBytes()), req.MarshalUnsafe, stat.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return err } @@ -178,10 +178,10 @@ func (f *ClientFD) Read(ctx context.Context, dst []byte, offset uint64) (uint64, // This will be unmarshalled into. Already set Buf so that we don't need to // allocate a temporary buffer during unmarshalling. - // PReadResp.UnmarshalBytes expects this to be set. + // PReadResp.CheckedUnmarshal expects this to be set. resp.Buf = buf ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(PRead, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(PRead, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return uint64(resp.NumBytes), err }) @@ -205,7 +205,7 @@ func (f *ClientFD) Write(ctx context.Context, src []byte, offset uint64) (uint64 var resp PWriteResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(PWrite, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(PWrite, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return resp.Count, err }) @@ -222,7 +222,7 @@ func (f *ClientFD) MkdirAt(ctx context.Context, name string, mode linux.FileMode var resp MkdirAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(MkdirAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(MkdirAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return &resp.ChildDir, err } @@ -239,7 +239,7 @@ func (f *ClientFD) SymlinkAt(ctx context.Context, name, target string, uid UID, var resp SymlinkAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(SymlinkAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(SymlinkAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return &resp.Symlink, err } @@ -254,7 +254,7 @@ func (f *ClientFD) LinkAt(ctx context.Context, targetFD FDID, name string) (*Ino var resp LinkAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(LinkAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(LinkAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return &resp.Link, err } @@ -272,7 +272,7 @@ func (f *ClientFD) MknodAt(ctx context.Context, name string, mode linux.FileMode var resp MknodAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(MknodAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(MknodAt, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return &resp.Child, err } @@ -298,7 +298,7 @@ func (f *ClientFD) SetStat(ctx context.Context, stat *linux.Statx) (uint32, erro var resp SetStatResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(SetStat, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(SetStat, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return resp.FailureMask, unix.Errno(resp.FailureErrNo), err } @@ -312,7 +312,7 @@ func (f *ClientFD) WalkMultiple(ctx context.Context, names []string) (WalkStatus var resp WalkResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(Walk, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(Walk, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return resp.Status, resp.Inodes, err } @@ -327,7 +327,7 @@ func (f *ClientFD) Walk(ctx context.Context, name string) (*Inode, error) { var inode [1]Inode resp := WalkResp{Inodes: inode[:]} ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(Walk, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(Walk, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) if err != nil { return nil, err @@ -363,7 +363,7 @@ func (f *ClientFD) WalkStat(ctx context.Context, names []string) ([]linux.Statx, var resp WalkStatResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(WalkStat, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(WalkStat, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return resp.Stats, err } @@ -372,7 +372,7 @@ func (f *ClientFD) WalkStat(ctx context.Context, names []string) ([]linux.Statx, func (f *ClientFD) StatFSTo(ctx context.Context, statFS *StatFS) error { req := FStatFSReq{FD: f.fd} ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(FStatFS, uint32(req.SizeBytes()), req.MarshalUnsafe, statFS.UnmarshalUnsafe, nil) + err := f.client.SndRcvMessage(FStatFS, uint32(req.SizeBytes()), req.MarshalUnsafe, statFS.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return err } @@ -396,7 +396,7 @@ func (f *ClientFD) ReadLinkAt(ctx context.Context) (string, error) { req := ReadLinkAtReq{FD: f.fd} var resp ReadLinkAtResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(ReadLinkAt, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(ReadLinkAt, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return string(resp.Target), err } @@ -465,7 +465,7 @@ func (f *ClientFD) Getdents64(ctx context.Context, count int32) ([]Dirent64, err var resp Getdents64Resp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(Getdents64, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(Getdents64, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return resp.Dirents, err } @@ -479,7 +479,7 @@ func (f *ClientFD) ListXattr(ctx context.Context, size uint64) ([]string, error) var resp FListXattrResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(FListXattr, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(FListXattr, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return resp.Xattrs, err } @@ -494,7 +494,7 @@ func (f *ClientFD) GetXattr(ctx context.Context, name string, size uint64) (stri var resp FGetXattrResp ctx.UninterruptibleSleepStart(false) - err := f.client.SndRcvMessage(FGetXattr, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalBytes, nil) + err := f.client.SndRcvMessage(FGetXattr, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil) ctx.UninterruptibleSleepFinish(false) return string(resp.Value), err } diff --git a/pkg/lisafs/connection.go b/pkg/lisafs/connection.go index f6e5ecb4f..861a0ab9a 100644 --- a/pkg/lisafs/connection.go +++ b/pkg/lisafs/connection.go @@ -36,6 +36,9 @@ type Connection struct { // associated with it for its entire lifetime. server *Server + // maxMessageSize is the cached value of server.impl.MaxMessageSize(). + maxMessageSize uint32 + // mounted is a one way flag indicating whether this connection has been // mounted correctly and the server is initialized properly. mounted bool @@ -72,12 +75,13 @@ type Connection struct { // required. The connection must be started separately. func (s *Server) CreateConnection(sock *unet.Socket, readonly bool) (*Connection, error) { c := &Connection{ - sockComm: newSockComm(sock), - server: s, - readonly: readonly, - channels: make([]*channel, 0, maxChannels()), - fds: make(map[FDID]genericFD), - nextFDID: InvalidFDID + 1, + sockComm: newSockComm(sock), + server: s, + maxMessageSize: s.impl.MaxMessageSize(), + readonly: readonly, + channels: make([]*channel, 0, maxChannels()), + fds: make(map[FDID]genericFD), + nextFDID: InvalidFDID + 1, } alloc, err := flipcall.NewPacketWindowAllocator() @@ -153,6 +157,10 @@ func (c *Connection) respondError(comm Communicator, err unix.Errno) (MID, uint3 } func (c *Connection) handleMsg(comm Communicator, m MID, payloadLen uint32) (MID, uint32, []int) { + if payloadLen > c.maxMessageSize { + log.Warningf("received payload is too large: %d bytes", payloadLen) + return c.respondError(comm, unix.EIO) + } if !c.reqGate.Enter() { // c.close() has been called; the connection is shutting down. return c.respondError(comm, unix.ECONNRESET) @@ -177,6 +185,11 @@ func (c *Connection) handleMsg(comm Communicator, m MID, payloadLen uint32) (MID closeFDs(fds) return c.respondError(comm, p9.ExtractErrno(err)) } + if respPayloadLen > c.maxMessageSize { + log.Warningf("handler for message %d responded with payload which is too large: %d bytes", m, respPayloadLen) + closeFDs(fds) + return c.respondError(comm, unix.EIO) + } return m, respPayloadLen, fds } diff --git a/pkg/lisafs/connection_test.go b/pkg/lisafs/connection_test.go index 28ba47112..c127db44e 100644 --- a/pkg/lisafs/connection_test.go +++ b/pkg/lisafs/connection_test.go @@ -119,7 +119,9 @@ func TestUnsupportedMessage(t *testing.T) { func dynamicMsgHandler(c *lisafs.Connection, comm lisafs.Communicator, payloadLen uint32) (uint32, error) { var req lisafs.MsgDynamic - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } // Just echo back the message. respPayloadLen := uint32(req.SizeBytes()) @@ -144,7 +146,7 @@ func TestStress(t *testing.T) { req.Randomize(100) var resp lisafs.MsgDynamic - if err := c.SndRcvMessage(dynamicMsgID, uint32(req.SizeBytes()), req.MarshalBytes, resp.UnmarshalBytes, nil); err != nil { + if err := c.SndRcvMessage(dynamicMsgID, uint32(req.SizeBytes()), req.MarshalBytes, resp.CheckedUnmarshal, nil); err != nil { t.Errorf("SndRcvMessage: received unexpected error %v", err) return } @@ -163,7 +165,9 @@ func versionHandler(c *lisafs.Connection, comm lisafs.Communicator, payloadLen u // To be fair, usually handlers will create their own objects and return a // pointer to those. Might be tempting to reuse above variables, but don't. var rv lisafs.P9Version - rv.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := rv.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } // Create a new response. sv := lisafs.P9Version{ @@ -186,7 +190,7 @@ func BenchmarkSendRecv(b *testing.B) { var recvV lisafs.P9Version runServerClient(b, func(c *lisafs.Client) { for i := 0; i < b.N; i++ { - if err := c.SndRcvMessage(versionMsgID, uint32(sendV.SizeBytes()), sendV.MarshalBytes, recvV.UnmarshalBytes, nil); err != nil { + if err := c.SndRcvMessage(versionMsgID, uint32(sendV.SizeBytes()), sendV.MarshalBytes, recvV.CheckedUnmarshal, nil); err != nil { b.Fatalf("unexpected error occurred: %v", err) } } diff --git a/pkg/lisafs/fd.go b/pkg/lisafs/fd.go index cc6919a1b..eab21de04 100644 --- a/pkg/lisafs/fd.go +++ b/pkg/lisafs/fd.go @@ -26,7 +26,7 @@ import ( // FDID (file descriptor identifier) is used to identify FDs on a connection. // Each connection has its own FDID namespace. // -// +marshal slice:FDIDSlice +// +marshal boundCheck slice:FDIDSlice type FDID uint32 // InvalidFDID represents an invalid FDID. diff --git a/pkg/lisafs/handlers.go b/pkg/lisafs/handlers.go index 82807734d..4fa316fe5 100644 --- a/pkg/lisafs/handlers.go +++ b/pkg/lisafs/handlers.go @@ -85,7 +85,9 @@ func ErrorHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, // has been successfully mounted can other channels be created. func MountHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req MountReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } mountPath := path.Clean(string(req.MountPath)) if !filepath.IsAbs(mountPath) { @@ -160,7 +162,9 @@ func ChannelHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32 // FStatHandler handles the FStat RPC. func FStatHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req StatReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.lookupFD(req.FD) if err != nil { @@ -185,7 +189,9 @@ func SetStatHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32 } var req SetStatReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -203,7 +209,9 @@ func SetStatHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32 // WalkHandler handles the Walk RPC. func WalkHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req WalkReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.DirFD) if err != nil { @@ -225,7 +233,9 @@ func WalkHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, e // WalkStatHandler handles the WalkStat RPC. func WalkStatHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req WalkReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.DirFD) if err != nil { @@ -256,7 +266,9 @@ func WalkStatHandler(c *Connection, comm Communicator, payloadLen uint32) (uint3 // OpenAtHandler handles the OpenAt RPC. func OpenAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req OpenAtReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } // Only keep allowed open flags. if allowedFlags := req.Flags & allowedOpenFlags; allowedFlags != req.Flags { @@ -291,7 +303,9 @@ func OpenCreateAtHandler(c *Connection, comm Communicator, payloadLen uint32) (u return 0, unix.EROFS } var req OpenCreateAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } // Only keep allowed open flags. if allowedFlags := req.Flags & allowedOpenFlags; allowedFlags != req.Flags { @@ -319,7 +333,9 @@ func OpenCreateAtHandler(c *Connection, comm Communicator, payloadLen uint32) (u // CloseHandler handles the Close RPC. func CloseHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req CloseReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } for _, fd := range req.FDs { c.RemoveFD(fd) } @@ -331,7 +347,9 @@ func CloseHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, // FSyncHandler handles the FSync RPC. func FSyncHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req FsyncReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } // Return the first error we encounter, but sync everything we can // regardless. @@ -363,7 +381,10 @@ func PWriteHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, // Note that it is an optimized Unmarshal operation which avoids any buffer // allocation and copying. req.Buf just points to payload. This is safe to do // as the handler owns payload and req's lifetime is limited to the handler. - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } + fd, err := c.LookupOpenFD(req.FD) if err != nil { return 0, err @@ -377,7 +398,9 @@ func PWriteHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, // PReadHandler handles the PRead RPC. func PReadHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req PReadReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupOpenFD(req.FD) if err != nil { @@ -396,7 +419,9 @@ func MkdirAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32 return 0, unix.EROFS } var req MkdirAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } name := string(req.Name) if err := checkSafeName(name); err != nil { @@ -420,7 +445,9 @@ func MknodAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32 return 0, unix.EROFS } var req MknodAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } name := string(req.Name) if err := checkSafeName(name); err != nil { @@ -444,7 +471,9 @@ func SymlinkAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint return 0, unix.EROFS } var req SymlinkAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } name := string(req.Name) if err := checkSafeName(name); err != nil { @@ -468,7 +497,9 @@ func LinkAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, return 0, unix.EROFS } var req LinkAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } name := string(req.Name) if err := checkSafeName(name); err != nil { @@ -494,7 +525,9 @@ func LinkAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, // FStatFSHandler handles the FStatFS RPC. func FStatFSHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req FStatFSReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -510,7 +543,9 @@ func FAllocateHandler(c *Connection, comm Communicator, payloadLen uint32) (uint return 0, unix.EROFS } var req FAllocateReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupOpenFD(req.FD) if err != nil { @@ -526,7 +561,9 @@ func FAllocateHandler(c *Connection, comm Communicator, payloadLen uint32) (uint // ReadLinkAtHandler handles the ReadLinkAt RPC. func ReadLinkAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req ReadLinkAtReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -542,7 +579,9 @@ func ReadLinkAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uin // FlushHandler handles the Flush RPC. func FlushHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req FlushReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupOpenFD(req.FD) if err != nil { @@ -556,7 +595,9 @@ func FlushHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, // ConnectHandler handles the Connect RPC. func ConnectHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req ConnectReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -575,7 +616,9 @@ func UnlinkAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint3 return 0, unix.EROFS } var req UnlinkAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } name := string(req.Name) if err := checkSafeName(name); err != nil { @@ -599,7 +642,9 @@ func RenameAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint3 return 0, unix.EROFS } var req RenameAtReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } newName := string(req.NewName) if err := checkSafeName(newName); err != nil { @@ -682,7 +727,9 @@ func (fd *ControlFD) renameRecursiveLocked(newDir *ControlFD, newName string, pi // Getdents64Handler handles the Getdents64 RPC. func Getdents64Handler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req Getdents64Req - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupOpenFD(req.DirFD) if err != nil { @@ -704,7 +751,9 @@ func Getdents64Handler(c *Connection, comm Communicator, payloadLen uint32) (uin // FGetXattrHandler handles the FGetXattr RPC. func FGetXattrHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req FGetXattrReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -720,7 +769,9 @@ func FSetXattrHandler(c *Connection, comm Communicator, payloadLen uint32) (uint return 0, unix.EROFS } var req FSetXattrReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -733,7 +784,9 @@ func FSetXattrHandler(c *Connection, comm Communicator, payloadLen uint32) (uint // FListXattrHandler handles the FListXattr RPC. func FListXattrHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { var req FListXattrReq - req.UnmarshalUnsafe(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { @@ -749,7 +802,9 @@ func FRemoveXattrHandler(c *Connection, comm Communicator, payloadLen uint32) (u return 0, unix.EROFS } var req FRemoveXattrReq - req.UnmarshalBytes(comm.PayloadBuf(payloadLen)) + if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { + return 0, unix.EIO + } fd, err := c.LookupControlFD(req.FD) if err != nil { diff --git a/pkg/lisafs/message.go b/pkg/lisafs/message.go index c5474d804..bfcb9426a 100644 --- a/pkg/lisafs/message.go +++ b/pkg/lisafs/message.go @@ -154,10 +154,6 @@ func MaxMessageSize() uint32 { return uint32(hostarch.HugePageSize - os.Getpagesize()) } -// TODO(gvisor.dev/issue/6450): Once this is resolved: -// * Update manual implementations and function signatures. -// * Update RPC handlers and appropriate callers to handle errors correctly. - // UID represents a user ID. // // +marshal @@ -182,7 +178,7 @@ func (gid GID) Ok() bool { func NoopMarshal(b []byte) []byte { return b } // NoopUnmarshal is a noop implementation of marshal.Marshallable.UnmarshalBytes. -func NoopUnmarshal(b []byte) []byte { return b } +func NoopUnmarshal(b []byte) ([]byte, bool) { return b, true } // SizedString represents a string in memory. The marshalled string bytes are // preceded by a uint32 signifying the string length. @@ -201,13 +197,16 @@ func (s *SizedString) MarshalBytes(dst []byte) []byte { return dst[copy(dst[:strLen], *s):] } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (s *SizedString) UnmarshalBytes(src []byte) []byte { +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (s *SizedString) CheckedUnmarshal(src []byte) ([]byte, bool) { var strLen primitive.Uint32 - src = strLen.UnmarshalUnsafe(src) + srcRemain, ok := strLen.CheckedUnmarshal(src) + if !ok || len(srcRemain) < int(strLen) { + return src, false + } // Take the hit, this leads to an allocation + memcpy. No way around it. - *s = SizedString(src[:strLen]) - return src[strLen:] + *s = SizedString(srcRemain[:strLen]) + return srcRemain[strLen:], true } // StringArray represents an array of SizedStrings in memory. The marshalled @@ -235,10 +234,13 @@ func (s *StringArray) MarshalBytes(dst []byte) []byte { return dst } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (s *StringArray) UnmarshalBytes(src []byte) []byte { +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (s *StringArray) CheckedUnmarshal(src []byte) ([]byte, bool) { var arrLen primitive.Uint32 - src = arrLen.UnmarshalUnsafe(src) + srcRemain, ok := arrLen.CheckedUnmarshal(src) + if !ok { + return src, false + } if cap(*s) < int(arrLen) { *s = make([]string, arrLen) @@ -248,10 +250,13 @@ func (s *StringArray) UnmarshalBytes(src []byte) []byte { for i := primitive.Uint32(0); i < arrLen; i++ { var sstr SizedString - src = sstr.UnmarshalBytes(src) + srcRemain, ok = sstr.CheckedUnmarshal(srcRemain) + if !ok { + return src, false + } (*s)[i] = string(sstr) } - return src + return srcRemain, true } // Inode represents an inode on the remote filesystem. @@ -278,9 +283,9 @@ func (m *MountReq) MarshalBytes(dst []byte) []byte { return m.MountPath.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (m *MountReq) UnmarshalBytes(src []byte) []byte { - return m.MountPath.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (m *MountReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + return m.MountPath.CheckedUnmarshal(src) } // MountResp represents a Mount response. @@ -310,23 +315,30 @@ func (m *MountResp) MarshalBytes(dst []byte) []byte { return MarshalUnsafeMIDSlice(m.SupportedMs, dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (m *MountResp) UnmarshalBytes(src []byte) []byte { - src = m.Root.UnmarshalUnsafe(src) - src = m.MaxMessageSize.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (m *MountResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + m.SupportedMs = m.SupportedMs[:0] + if m.SizeBytes() > len(src) { + return src, false + } + srcRemain := m.Root.UnmarshalUnsafe(src) + srcRemain = m.MaxMessageSize.UnmarshalUnsafe(srcRemain) var numSupported primitive.Uint16 - src = numSupported.UnmarshalBytes(src) + srcRemain = numSupported.UnmarshalBytes(srcRemain) + if int(numSupported)*(*MID)(nil).SizeBytes() > len(srcRemain) { + return src, false + } if cap(m.SupportedMs) < int(numSupported) { m.SupportedMs = make([]MID, numSupported) } else { m.SupportedMs = m.SupportedMs[:numSupported] } - return UnmarshalUnsafeMIDSlice(m.SupportedMs, src) + return UnmarshalUnsafeMIDSlice(m.SupportedMs, srcRemain), true } // ChannelResp is the response to the create channel request. // -// +marshal +// +marshal boundCheck type ChannelResp struct { dataOffset int64 dataLength uint64 @@ -341,14 +353,14 @@ type ErrorResp struct { // StatReq requests the stat results for the specified FD. // -// +marshal +// +marshal boundCheck type StatReq struct { FD FDID } // SetStatReq is used to set attributeds on FDs. // -// +marshal +// +marshal boundCheck type SetStatReq struct { FD FDID _ uint32 @@ -366,7 +378,7 @@ type SetStatReq struct { // set attribute operation. If multiple operations failed then any of those // errnos can be returned. // -// +marshal +// +marshal boundCheck type SetStatResp struct { FailureMask uint32 FailureErrNo uint32 @@ -390,10 +402,17 @@ func (w *WalkReq) MarshalBytes(dst []byte) []byte { return w.Path.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (w *WalkReq) UnmarshalBytes(src []byte) []byte { - src = w.DirFD.UnmarshalUnsafe(src) - return w.Path.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (w *WalkReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + w.Path = w.Path[:0] + if w.SizeBytes() > len(src) { + return src, false + } + srcRemain := w.DirFD.UnmarshalUnsafe(src) + if srcRemain, ok := w.Path.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // WalkStatus is used to indicate the reason for partial/unsuccessful server @@ -439,19 +458,25 @@ func (w *WalkResp) MarshalBytes(dst []byte) []byte { return MarshalUnsafeInodeSlice(w.Inodes, dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (w *WalkResp) UnmarshalBytes(src []byte) []byte { - src = w.Status.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (w *WalkResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + w.Inodes = w.Inodes[:0] + if w.SizeBytes() > len(src) { + return src, false + } + srcRemain := w.Status.UnmarshalUnsafe(src) var numInodes primitive.Uint32 - src = numInodes.UnmarshalUnsafe(src) - + srcRemain = numInodes.UnmarshalUnsafe(srcRemain) + if int(numInodes)*(*Inode)(nil).SizeBytes() > len(srcRemain) { + return src, false + } if cap(w.Inodes) < int(numInodes) { w.Inodes = make([]Inode, numInodes) } else { w.Inodes = w.Inodes[:numInodes] } - return UnmarshalUnsafeInodeSlice(w.Inodes, src) + return UnmarshalUnsafeInodeSlice(w.Inodes, srcRemain), true } // WalkStatResp is used to communicate stat results for WalkStat. @@ -472,22 +497,29 @@ func (w *WalkStatResp) MarshalBytes(dst []byte) []byte { return linux.MarshalUnsafeStatxSlice(w.Stats, dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (w *WalkStatResp) UnmarshalBytes(src []byte) []byte { +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (w *WalkStatResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + w.Stats = w.Stats[:0] + if w.SizeBytes() > len(src) { + return src, false + } var numStats primitive.Uint32 - src = numStats.UnmarshalUnsafe(src) + srcRemain := numStats.UnmarshalUnsafe(src) + if int(numStats)*linux.SizeOfStatx > len(srcRemain) { + return src, false + } if cap(w.Stats) < int(numStats) { w.Stats = make([]linux.Statx, numStats) } else { w.Stats = w.Stats[:numStats] } - return linux.UnmarshalUnsafeStatxSlice(w.Stats, src) + return linux.UnmarshalUnsafeStatxSlice(w.Stats, srcRemain), true } // OpenAtReq is used to open existing FDs with the specified flags. // -// +marshal +// +marshal boundCheck type OpenAtReq struct { FD FDID Flags uint32 @@ -495,7 +527,7 @@ type OpenAtReq struct { // OpenAtResp is used to communicate the newly created FD. // -// +marshal +// +marshal boundCheck type OpenAtResp struct { NewFD FDID } @@ -512,32 +544,39 @@ type createCommon struct { // OpenCreateAtReq is used to make OpenCreateAt requests. type OpenCreateAtReq struct { createCommon - Name SizedString Flags primitive.Uint32 + Name SizedString } // SizeBytes implements marshal.Marshallable.SizeBytes. func (o *OpenCreateAtReq) SizeBytes() int { - return o.createCommon.SizeBytes() + o.Name.SizeBytes() + o.Flags.SizeBytes() + return o.createCommon.SizeBytes() + o.Flags.SizeBytes() + o.Name.SizeBytes() } // MarshalBytes implements marshal.Marshallable.MarshalBytes. func (o *OpenCreateAtReq) MarshalBytes(dst []byte) []byte { dst = o.createCommon.MarshalUnsafe(dst) - dst = o.Name.MarshalBytes(dst) - return o.Flags.MarshalUnsafe(dst) + dst = o.Flags.MarshalUnsafe(dst) + return o.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (o *OpenCreateAtReq) UnmarshalBytes(src []byte) []byte { - src = o.createCommon.UnmarshalUnsafe(src) - src = o.Name.UnmarshalBytes(src) - return o.Flags.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (o *OpenCreateAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + o.Name = "" + if o.SizeBytes() > len(src) { + return src, false + } + srcRemain := o.createCommon.UnmarshalUnsafe(src) + srcRemain = o.Flags.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := o.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // OpenCreateAtResp is used to communicate successful OpenCreateAt results. // -// +marshal +// +marshal boundCheck type OpenCreateAtResp struct { Child Inode NewFD FDID @@ -561,16 +600,23 @@ func (f *FdArray) MarshalBytes(dst []byte) []byte { return MarshalUnsafeFDIDSlice(*f, dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (f *FdArray) UnmarshalBytes(src []byte) []byte { +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (f *FdArray) CheckedUnmarshal(src []byte) ([]byte, bool) { + *f = (*f)[:0] + if f.SizeBytes() > len(src) { + return src, false + } var arrLen primitive.Uint32 - src = arrLen.UnmarshalUnsafe(src) + srcRemain := arrLen.UnmarshalUnsafe(src) + if int(arrLen)*(*FDID)(nil).SizeBytes() > len(srcRemain) { + return src, false + } if cap(*f) < int(arrLen) { *f = make(FdArray, arrLen) } else { *f = (*f)[:arrLen] } - return UnmarshalUnsafeFDIDSlice(*f, src) + return UnmarshalUnsafeFDIDSlice(*f, srcRemain), true } // CloseReq is used to close(2) FDs. @@ -588,9 +634,9 @@ func (c *CloseReq) MarshalBytes(dst []byte) []byte { return c.FDs.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (c *CloseReq) UnmarshalBytes(src []byte) []byte { - return c.FDs.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (c *CloseReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + return c.FDs.CheckedUnmarshal(src) } // FsyncReq is used to fsync(2) FDs. @@ -608,14 +654,14 @@ func (f *FsyncReq) MarshalBytes(dst []byte) []byte { return f.FDs.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (f *FsyncReq) UnmarshalBytes(src []byte) []byte { - return f.FDs.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (f *FsyncReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + return f.FDs.CheckedUnmarshal(src) } // PReadReq is used to pread(2) on an FD. // -// +marshal +// +marshal boundCheck type PReadReq struct { Offset uint64 FD FDID @@ -639,13 +685,16 @@ func (r *PReadResp) MarshalBytes(dst []byte) []byte { return dst[copy(dst[:r.NumBytes], r.Buf[:r.NumBytes]):] } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (r *PReadResp) UnmarshalBytes(src []byte) []byte { - src = r.NumBytes.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (r *PReadResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + srcRemain, ok := r.NumBytes.CheckedUnmarshal(src) + if !ok || int(r.NumBytes) > len(srcRemain) || int(r.NumBytes) > len(r.Buf) { + return src, false + } // We expect the client to have already allocated r.Buf. r.Buf probably // (optimally) points to usermem. Directly copy into that. - return src[copy(r.Buf[:r.NumBytes], src[:r.NumBytes]):] + return srcRemain[copy(r.Buf[:r.NumBytes], srcRemain[:r.NumBytes]):], true } // PWriteReq is used to pwrite(2) on an FD. @@ -669,21 +718,28 @@ func (w *PWriteReq) MarshalBytes(dst []byte) []byte { return dst[copy(dst[:w.NumBytes], w.Buf[:w.NumBytes]):] } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (w *PWriteReq) UnmarshalBytes(src []byte) []byte { - src = w.Offset.UnmarshalUnsafe(src) - src = w.FD.UnmarshalUnsafe(src) - src = w.NumBytes.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (w *PWriteReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + w.NumBytes = 0 + if w.SizeBytes() > len(src) { + return src, false + } + srcRemain := w.Offset.UnmarshalUnsafe(src) + srcRemain = w.FD.UnmarshalUnsafe(srcRemain) + srcRemain = w.NumBytes.UnmarshalUnsafe(srcRemain) // This is an optimization. Assuming that the server is making this call, it // is safe to just point to src rather than allocating and copying. - w.Buf = src[:w.NumBytes] - return src[w.NumBytes:] + if int(w.NumBytes) > len(srcRemain) { + return src, false + } + w.Buf = srcRemain[:w.NumBytes] + return srcRemain[w.NumBytes:], true } // PWriteResp is used to return the result of pwrite(2). // -// +marshal +// +marshal boundCheck type PWriteResp struct { Count uint64 } @@ -705,15 +761,22 @@ func (m *MkdirAtReq) MarshalBytes(dst []byte) []byte { return m.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (m *MkdirAtReq) UnmarshalBytes(src []byte) []byte { - src = m.createCommon.UnmarshalUnsafe(src) - return m.Name.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (m *MkdirAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + m.Name = "" + if m.SizeBytes() > len(src) { + return src, false + } + srcRemain := m.createCommon.UnmarshalUnsafe(src) + if srcRemain, ok := m.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // MkdirAtResp is the response to a successful MkdirAt request. // -// +marshal +// +marshal boundCheck type MkdirAtResp struct { ChildDir Inode } @@ -721,35 +784,42 @@ type MkdirAtResp struct { // MknodAtReq is used to make MknodAt requests. type MknodAtReq struct { createCommon - Name SizedString Minor primitive.Uint32 Major primitive.Uint32 + Name SizedString } // SizeBytes implements marshal.Marshallable.SizeBytes. func (m *MknodAtReq) SizeBytes() int { - return m.createCommon.SizeBytes() + m.Name.SizeBytes() + m.Minor.SizeBytes() + m.Major.SizeBytes() + return m.createCommon.SizeBytes() + m.Minor.SizeBytes() + m.Major.SizeBytes() + m.Name.SizeBytes() } // MarshalBytes implements marshal.Marshallable.MarshalBytes. func (m *MknodAtReq) MarshalBytes(dst []byte) []byte { dst = m.createCommon.MarshalUnsafe(dst) - dst = m.Name.MarshalBytes(dst) dst = m.Minor.MarshalUnsafe(dst) - return m.Major.MarshalUnsafe(dst) + dst = m.Major.MarshalUnsafe(dst) + return m.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (m *MknodAtReq) UnmarshalBytes(src []byte) []byte { - src = m.createCommon.UnmarshalUnsafe(src) - src = m.Name.UnmarshalBytes(src) - src = m.Minor.UnmarshalUnsafe(src) - return m.Major.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (m *MknodAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + m.Name = "" + if m.SizeBytes() > len(src) { + return src, false + } + srcRemain := m.createCommon.UnmarshalUnsafe(src) + srcRemain = m.Minor.UnmarshalUnsafe(srcRemain) + srcRemain = m.Major.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := m.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // MknodAtResp is the response to a successful MknodAt request. // -// +marshal +// +marshal boundCheck type MknodAtResp struct { Child Inode } @@ -757,38 +827,49 @@ type MknodAtResp struct { // SymlinkAtReq is used to make SymlinkAt request. type SymlinkAtReq struct { DirFD FDID - Name SizedString - Target SizedString UID UID GID GID + Name SizedString + Target SizedString } // SizeBytes implements marshal.Marshallable.SizeBytes. func (s *SymlinkAtReq) SizeBytes() int { - return s.DirFD.SizeBytes() + s.Name.SizeBytes() + s.Target.SizeBytes() + s.UID.SizeBytes() + s.GID.SizeBytes() + return s.DirFD.SizeBytes() + s.UID.SizeBytes() + s.GID.SizeBytes() + s.Name.SizeBytes() + s.Target.SizeBytes() } // MarshalBytes implements marshal.Marshallable.MarshalBytes. func (s *SymlinkAtReq) MarshalBytes(dst []byte) []byte { dst = s.DirFD.MarshalUnsafe(dst) - dst = s.Name.MarshalBytes(dst) - dst = s.Target.MarshalBytes(dst) dst = s.UID.MarshalUnsafe(dst) - return s.GID.MarshalUnsafe(dst) + dst = s.GID.MarshalUnsafe(dst) + dst = s.Name.MarshalBytes(dst) + return s.Target.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (s *SymlinkAtReq) UnmarshalBytes(src []byte) []byte { - src = s.DirFD.UnmarshalUnsafe(src) - src = s.Name.UnmarshalBytes(src) - src = s.Target.UnmarshalBytes(src) - src = s.UID.UnmarshalUnsafe(src) - return s.GID.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (s *SymlinkAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + s.Name = "" + s.Target = "" + if s.SizeBytes() > len(src) { + return src, false + } + srcRemain := s.DirFD.UnmarshalUnsafe(src) + srcRemain = s.UID.UnmarshalUnsafe(srcRemain) + srcRemain = s.GID.UnmarshalUnsafe(srcRemain) + var ok bool + if srcRemain, ok = s.Name.CheckedUnmarshal(srcRemain); !ok { + return src, false + } + if srcRemain, ok = s.Target.CheckedUnmarshal(srcRemain); !ok { + return src, false + } + return srcRemain, true } // SymlinkAtResp is the response to a successful SymlinkAt request. // -// +marshal +// +marshal boundCheck type SymlinkAtResp struct { Symlink Inode } @@ -812,30 +893,37 @@ func (l *LinkAtReq) MarshalBytes(dst []byte) []byte { return l.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (l *LinkAtReq) UnmarshalBytes(src []byte) []byte { - src = l.DirFD.UnmarshalUnsafe(src) - src = l.Target.UnmarshalUnsafe(src) - return l.Name.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (l *LinkAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + l.Name = "" + if l.SizeBytes() > len(src) { + return src, false + } + srcRemain := l.DirFD.UnmarshalUnsafe(src) + srcRemain = l.Target.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := l.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // LinkAtResp is used to respond to a successful LinkAt request. // -// +marshal +// +marshal boundCheck type LinkAtResp struct { Link Inode } // FStatFSReq is used to request StatFS results for the specified FD. // -// +marshal +// +marshal boundCheck type FStatFSReq struct { FD FDID } // StatFS is responded to a successful FStatFS request. // -// +marshal +// +marshal boundCheck type StatFS struct { Type uint64 BlockSize int64 @@ -849,7 +937,7 @@ type StatFS struct { // FAllocateReq is used to request to fallocate(2) an FD. This has no response. // -// +marshal +// +marshal boundCheck type FAllocateReq struct { FD FDID _ uint32 @@ -860,7 +948,7 @@ type FAllocateReq struct { // ReadLinkAtReq is used to readlinkat(2) at the specified FD. // -// +marshal +// +marshal boundCheck type ReadLinkAtReq struct { FD FDID } @@ -880,21 +968,21 @@ func (r *ReadLinkAtResp) MarshalBytes(dst []byte) []byte { return r.Target.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (r *ReadLinkAtResp) UnmarshalBytes(src []byte) []byte { - return r.Target.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (r *ReadLinkAtResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + return r.Target.CheckedUnmarshal(src) } // FlushReq is used to make Flush requests. // -// +marshal +// +marshal boundCheck type FlushReq struct { FD FDID } // ConnectReq is used to make a Connect request. // -// +marshal +// +marshal boundCheck type ConnectReq struct { FD FDID // SockType is used to specify the socket type to connect to. As a special @@ -906,27 +994,34 @@ type ConnectReq struct { // UnlinkAtReq is used to make UnlinkAt request. type UnlinkAtReq struct { DirFD FDID - Name SizedString Flags primitive.Uint32 + Name SizedString } // SizeBytes implements marshal.Marshallable.SizeBytes. func (u *UnlinkAtReq) SizeBytes() int { - return u.DirFD.SizeBytes() + u.Name.SizeBytes() + u.Flags.SizeBytes() + return u.DirFD.SizeBytes() + u.Flags.SizeBytes() + u.Name.SizeBytes() } // MarshalBytes implements marshal.Marshallable.MarshalBytes. func (u *UnlinkAtReq) MarshalBytes(dst []byte) []byte { dst = u.DirFD.MarshalUnsafe(dst) - dst = u.Name.MarshalBytes(dst) - return u.Flags.MarshalUnsafe(dst) + dst = u.Flags.MarshalUnsafe(dst) + return u.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (u *UnlinkAtReq) UnmarshalBytes(src []byte) []byte { - src = u.DirFD.UnmarshalUnsafe(src) - src = u.Name.UnmarshalBytes(src) - return u.Flags.UnmarshalUnsafe(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (u *UnlinkAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + u.Name = "" + if u.SizeBytes() > len(src) { + return src, false + } + srcRemain := u.DirFD.UnmarshalUnsafe(src) + srcRemain = u.Flags.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := u.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // RenameAtReq is used to make Rename requests. Note that the request takes in @@ -949,16 +1044,23 @@ func (r *RenameAtReq) MarshalBytes(dst []byte) []byte { return r.NewName.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (r *RenameAtReq) UnmarshalBytes(src []byte) []byte { - src = r.Renamed.UnmarshalUnsafe(src) - src = r.NewDir.UnmarshalUnsafe(src) - return r.NewName.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (r *RenameAtReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + r.NewName = "" + if r.SizeBytes() > len(src) { + return src, false + } + srcRemain := r.Renamed.UnmarshalUnsafe(src) + srcRemain = r.NewDir.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := r.NewName.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // Getdents64Req is used to make Getdents64 requests. // -// +marshal +// +marshal boundCheck type Getdents64Req struct { DirFD FDID // Count is the number of bytes to read. A negative value of Count is used to @@ -993,14 +1095,21 @@ func (d *Dirent64) MarshalBytes(dst []byte) []byte { return d.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (d *Dirent64) UnmarshalBytes(src []byte) []byte { - src = d.Ino.UnmarshalUnsafe(src) - src = d.DevMinor.UnmarshalUnsafe(src) - src = d.DevMajor.UnmarshalUnsafe(src) - src = d.Off.UnmarshalUnsafe(src) - src = d.Type.UnmarshalUnsafe(src) - return d.Name.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (d *Dirent64) CheckedUnmarshal(src []byte) ([]byte, bool) { + d.Name = "" + if d.SizeBytes() > len(src) { + return src, false + } + srcRemain := d.Ino.UnmarshalUnsafe(src) + srcRemain = d.DevMinor.UnmarshalUnsafe(srcRemain) + srcRemain = d.DevMajor.UnmarshalUnsafe(srcRemain) + srcRemain = d.Off.UnmarshalUnsafe(srcRemain) + srcRemain = d.Type.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := d.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // Getdents64Resp is used to communicate getdents64 results. @@ -1027,20 +1136,27 @@ func (g *Getdents64Resp) MarshalBytes(dst []byte) []byte { return dst } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (g *Getdents64Resp) UnmarshalBytes(src []byte) []byte { +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (g *Getdents64Resp) CheckedUnmarshal(src []byte) ([]byte, bool) { + g.Dirents = g.Dirents[:0] + if g.SizeBytes() > len(src) { + return src, false + } var numDirents primitive.Uint32 - src = numDirents.UnmarshalUnsafe(src) + srcRemain := numDirents.UnmarshalUnsafe(src) if cap(g.Dirents) < int(numDirents) { g.Dirents = make([]Dirent64, numDirents) } else { g.Dirents = g.Dirents[:numDirents] } + var ok bool for i := range g.Dirents { - src = g.Dirents[i].UnmarshalBytes(src) + if srcRemain, ok = g.Dirents[i].CheckedUnmarshal(srcRemain); !ok { + return src, false + } } - return src + return srcRemain, true } // FGetXattrReq is used to make FGetXattr requests. The response to this is @@ -1063,11 +1179,18 @@ func (g *FGetXattrReq) MarshalBytes(dst []byte) []byte { return g.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (g *FGetXattrReq) UnmarshalBytes(src []byte) []byte { - src = g.FD.UnmarshalUnsafe(src) - src = g.BufSize.UnmarshalUnsafe(src) - return g.Name.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (g *FGetXattrReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + g.Name = "" + if g.SizeBytes() > len(src) { + return src, false + } + srcRemain := g.FD.UnmarshalUnsafe(src) + srcRemain = g.BufSize.UnmarshalUnsafe(srcRemain) + if srcRemain, ok := g.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // FGetXattrResp is used to respond to FGetXattr request. @@ -1085,9 +1208,9 @@ func (g *FGetXattrResp) MarshalBytes(dst []byte) []byte { return g.Value.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (g *FGetXattrResp) UnmarshalBytes(src []byte) []byte { - return g.Value.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (g *FGetXattrResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + return g.Value.CheckedUnmarshal(src) } // FSetXattrReq is used to make FSetXattr requests. It has no response. @@ -1111,12 +1234,23 @@ func (s *FSetXattrReq) MarshalBytes(dst []byte) []byte { return s.Value.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (s *FSetXattrReq) UnmarshalBytes(src []byte) []byte { - src = s.FD.UnmarshalUnsafe(src) - src = s.Flags.UnmarshalUnsafe(src) - src = s.Name.UnmarshalBytes(src) - return s.Value.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (s *FSetXattrReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + s.Name = "" + s.Value = "" + if s.SizeBytes() > len(src) { + return src, false + } + srcRemain := s.FD.UnmarshalUnsafe(src) + srcRemain = s.Flags.UnmarshalUnsafe(srcRemain) + var ok bool + if srcRemain, ok = s.Name.CheckedUnmarshal(srcRemain); !ok { + return src, false + } + if srcRemain, ok = s.Value.CheckedUnmarshal(srcRemain); !ok { + return src, false + } + return srcRemain, true } // FRemoveXattrReq is used to make FRemoveXattr requests. It has no response. @@ -1136,15 +1270,22 @@ func (r *FRemoveXattrReq) MarshalBytes(dst []byte) []byte { return r.Name.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (r *FRemoveXattrReq) UnmarshalBytes(src []byte) []byte { - src = r.FD.UnmarshalUnsafe(src) - return r.Name.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (r *FRemoveXattrReq) CheckedUnmarshal(src []byte) ([]byte, bool) { + r.Name = "" + if r.SizeBytes() > len(src) { + return src, false + } + srcRemain := r.FD.UnmarshalUnsafe(src) + if srcRemain, ok := r.Name.CheckedUnmarshal(srcRemain); ok { + return srcRemain, true + } + return src, false } // FListXattrReq is used to make FListXattr requests. // -// +marshal +// +marshal boundCheck type FListXattrReq struct { FD FDID _ uint32 @@ -1166,7 +1307,7 @@ func (l *FListXattrResp) MarshalBytes(dst []byte) []byte { return l.Xattrs.MarshalBytes(dst) } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (l *FListXattrResp) UnmarshalBytes(src []byte) []byte { - return l.Xattrs.UnmarshalBytes(src) +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (l *FListXattrResp) CheckedUnmarshal(src []byte) ([]byte, bool) { + return l.Xattrs.CheckedUnmarshal(src) } diff --git a/pkg/lisafs/sample_message.go b/pkg/lisafs/sample_message.go index 3d4c090e4..25ee031cf 100644 --- a/pkg/lisafs/sample_message.go +++ b/pkg/lisafs/sample_message.go @@ -65,6 +65,24 @@ func (m *MsgDynamic) UnmarshalBytes(src []byte) []byte { return UnmarshalUnsafeMsg1Slice(m.Arr, src) } +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (m *MsgDynamic) CheckedUnmarshal(src []byte) ([]byte, bool) { + m.Arr = m.Arr[:0] + if m.SizeBytes() > len(src) { + return nil, false + } + src = m.N.UnmarshalUnsafe(src) + if int(m.N) > cap(m.Arr) { + m.Arr = make([]MsgSimple, m.N) + } else { + m.Arr = m.Arr[:m.N] + } + if int(m.N)*(*MsgSimple)(nil).SizeBytes() > len(src) { + return nil, false + } + return UnmarshalUnsafeMsg1Slice(m.Arr, src), true +} + // Randomize randomizes the contents of m. func (m *MsgDynamic) Randomize(arrLen int) { m.N = primitive.Uint32(arrLen) @@ -75,8 +93,6 @@ func (m *MsgDynamic) Randomize(arrLen int) { } // P9Version mimics p9.TVersion and p9.Rversion. -// -// +marshal dynamic type P9Version struct { MSize primitive.Uint32 Version string @@ -95,11 +111,18 @@ func (v *P9Version) MarshalBytes(dst []byte) []byte { return dst[copy(dst, v.Version):] } -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (v *P9Version) UnmarshalBytes(src []byte) []byte { +// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal. +func (v *P9Version) CheckedUnmarshal(src []byte) ([]byte, bool) { + v.Version = "" + if v.SizeBytes() > len(src) { + return nil, false + } src = v.MSize.UnmarshalUnsafe(src) var versionLen primitive.Uint16 src = versionLen.UnmarshalUnsafe(src) + if int(versionLen) > len(src) { + return nil, false + } v.Version = string(src[:versionLen]) - return src[versionLen:] + return src[versionLen:], true } diff --git a/pkg/lisafs/sock_test.go b/pkg/lisafs/sock_test.go index 387f4b7a8..2819a72b0 100644 --- a/pkg/lisafs/sock_test.go +++ b/pkg/lisafs/sock_test.go @@ -195,7 +195,7 @@ func TestSndRcvMessageNoPayload(t *testing.T) { }) } -func checkMessageReceive(t *testing.T, comm *sockCommunicator, wantM MID, wantMsg marshal.Marshallable) { +func checkMessageReceive(t *testing.T, comm *sockCommunicator, wantM MID, wantMsg interface{}) { gotM, payloadLen, err := comm.rcvMsg(0) if err != nil { t.Fatalf("readMessageFrom failed: %v", err) diff --git a/pkg/marshal/marshal.go b/pkg/marshal/marshal.go index a48a5835d..e60eacb3a 100644 --- a/pkg/marshal/marshal.go +++ b/pkg/marshal/marshal.go @@ -132,6 +132,26 @@ type Marshallable interface { CopyOutN(cc CopyContext, addr hostarch.Addr, limit int) (int, error) } +// CheckedMarshallable represents operations on a type that can be marshalled +// to and from memory and additionally does bound checking. +type CheckedMarshallable interface { + // CheckedMarshal is the same as Marshallable.MarshalUnsafe but without the + // precondition that dst must at least have some appropriate length. Similar + // to Marshallable.MarshalBytes, it returns a shifted slice according to how + // much data is consumed. Additionally it returns a bool indicating whether + // marshalling was successful. Unsuccessful marshalling doesn't consume any + // data. + CheckedMarshal(dst []byte) ([]byte, bool) + + // CheckedUnmarshal is the same as Marshallable.UmarshalUnsafe but without + // the precondition that src must at least have some appropriate length. + // Similar to Marshallable.UnmarshalBytes, it returns a shifted slice + // according to how much data is consumed. Additionally it returns a bool + // indicating whether marshalling was successful. Unsuccessful marshalling + // doesn't consume any data. + CheckedUnmarshal(src []byte) ([]byte, bool) +} + // go-marshal generates additional functions for a type based on additional // clauses to the +marshal directive. They are documented below. // @@ -187,3 +207,12 @@ type Marshallable interface { // func CopyInt32SliceIn(cc marshal.CopyContext, addr hostarch.Addr, dst []Int32) (int, error) { ... } // // This may help avoid a cast depending on how the generated functions are used. +// +// Bound Checking +// ============== +// +// Some users might want to do bound checking on marshal and unmarshal. This is +// is useful when the user does not control the buffer size. To prevent +// repeated bound checking code around Marshallable, users can add a +// "boundCheck" clause to the +marshal directive. go_marshal will generate the +// CheckedMarshallable interface methods on the type. diff --git a/pkg/marshal/primitive/primitive.go b/pkg/marshal/primitive/primitive.go index 7ece26933..bea5e0b89 100644 --- a/pkg/marshal/primitive/primitive.go +++ b/pkg/marshal/primitive/primitive.go @@ -25,42 +25,42 @@ import ( // Int8 is a marshal.Marshallable implementation for int8. // -// +marshal slice:Int8Slice:inner +// +marshal boundCheck slice:Int8Slice:inner type Int8 int8 // Uint8 is a marshal.Marshallable implementation for uint8. // -// +marshal slice:Uint8Slice:inner +// +marshal boundCheck slice:Uint8Slice:inner type Uint8 uint8 // Int16 is a marshal.Marshallable implementation for int16. // -// +marshal slice:Int16Slice:inner +// +marshal boundCheck slice:Int16Slice:inner type Int16 int16 // Uint16 is a marshal.Marshallable implementation for uint16. // -// +marshal slice:Uint16Slice:inner +// +marshal boundCheck slice:Uint16Slice:inner type Uint16 uint16 // Int32 is a marshal.Marshallable implementation for int32. // -// +marshal slice:Int32Slice:inner +// +marshal boundCheck slice:Int32Slice:inner type Int32 int32 // Uint32 is a marshal.Marshallable implementation for uint32. // -// +marshal slice:Uint32Slice:inner +// +marshal boundCheck slice:Uint32Slice:inner type Uint32 uint32 // Int64 is a marshal.Marshallable implementation for int64. // -// +marshal slice:Int64Slice:inner +// +marshal boundCheck slice:Int64Slice:inner type Int64 int64 // Uint64 is a marshal.Marshallable implementation for uint64. // -// +marshal slice:Uint64Slice:inner +// +marshal boundCheck slice:Uint64Slice:inner type Uint64 uint64 // ByteSlice is a marshal.Marshallable implementation for []byte. diff --git a/tools/go_marshal/gomarshal/generator.go b/tools/go_marshal/gomarshal/generator.go index 4c23637c0..e290de030 100644 --- a/tools/go_marshal/gomarshal/generator.go +++ b/tools/go_marshal/gomarshal/generator.go @@ -217,10 +217,11 @@ type sliceAPI struct { // marshallableType carries information about a type marked with the '+marshal' // directive. type marshallableType struct { - spec *ast.TypeSpec - slice *sliceAPI - recv string - dynamic bool + spec *ast.TypeSpec + slice *sliceAPI + recv string + dynamic bool + boundCheck bool } func newMarshallableType(fset *token.FileSet, tagLine *ast.Comment, spec *ast.TypeSpec) *marshallableType { @@ -258,6 +259,9 @@ func newMarshallableType(fset *token.FileSet, tagLine *ast.Comment, spec *ast.Ty } else if tag == "dynamic" { mt.dynamic = true continue + } else if tag == "boundCheck" { + mt.boundCheck = true + continue } unhandledTags = append(unhandledTags, tag) @@ -391,6 +395,9 @@ func (g *Generator) generateOne(t *marshallableType, fset *token.FileSet) *inter if t.slice != nil { abortAt(fset.Position(t.slice.comment.Slash), "Slice API is not supported for dynamic types because it assumes that each slice element is statically sized.") } + if t.boundCheck { + abortAt(fset.Position(t.slice.comment.Slash), "Can not generate Checked methods for dynamic types. Has to be implemented manually.") + } // No validation needed, assume the user knows what they are doing. i.emitMarshallableForDynamicType() return i @@ -399,12 +406,18 @@ func (g *Generator) generateOne(t *marshallableType, fset *token.FileSet) *inter case *ast.StructType: i.validateStruct(t.spec, ty) i.emitMarshallableForStruct(ty) + if t.boundCheck { + i.emitCheckedMarshallableForStruct() + } if t.slice != nil { i.emitMarshallableSliceForStruct(ty, t.slice) } case *ast.Ident: i.validatePrimitiveNewtype(ty) i.emitMarshallableForPrimitiveNewtype(ty) + if t.boundCheck { + i.emitCheckedMarshallableForPrimitiveNewtype() + } if t.slice != nil { i.emitMarshallableSliceForPrimitiveNewtype(ty, t.slice) } @@ -412,6 +425,9 @@ func (g *Generator) generateOne(t *marshallableType, fset *token.FileSet) *inter i.validateArrayNewtype(t.spec.Name, ty) // After validate, we can safely call arrayLen. i.emitMarshallableForArrayNewtype(t.spec.Name, ty, ty.Elt.(*ast.Ident)) + if t.boundCheck { + i.emitCheckedMarshallableForArrayNewtype() + } if t.slice != nil { abortAt(fset.Position(t.slice.comment.Slash), "Array type marked as '+marshal slice:...', but this is not supported. Perhaps fold one of the dimensions?") } @@ -426,7 +442,7 @@ func (g *Generator) generateOne(t *marshallableType, fset *token.FileSet) *inter // implementations type t. func (g *Generator) generateOneTestSuite(t *marshallableType) *testGenerator { i := newTestGenerator(t.spec, t.recv) - i.emitTests(t.slice) + i.emitTests(t.slice, t.boundCheck) return i } diff --git a/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go b/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go index 4290bc959..ec59ac548 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go @@ -150,3 +150,33 @@ func (g *interfaceGenerator) emitMarshallableForArrayNewtype(n *ast.Ident, a *as }) g.emit("}\n\n") } + +func (g *interfaceGenerator) emitCheckedMarshallableForArrayNewtype() { + g.emit("// CheckedMarshal implements marshal.CheckedMarshallable.CheckedMarshal.\n") + g.emit("func (%s *%s) CheckedMarshal(dst []byte) ([]byte, bool) {\n", g.r, g.typeName()) + g.inIndent(func() { + g.emit("size := %s.SizeBytes()\n", g.r) + g.emit("if size > len(dst) {\n") + g.inIndent(func() { + g.emit("return dst, false\n") + }) + g.emit("}\n") + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(&%s[0]), uintptr(size))\n", g.r) + g.emit("return dst[size:], true\n") + }) + g.emit("}\n\n") + + g.emit("// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal.\n") + g.emit("func (%s *%s) CheckedUnmarshal(src []byte) ([]byte, bool) {\n", g.r, g.typeName()) + g.inIndent(func() { + g.emit("size := %s.SizeBytes()\n", g.r) + g.emit("if size > len(src) {\n") + g.inIndent(func() { + g.emit("return src, false\n") + }) + g.emit("}\n") + g.emit("gohacks.Memmove(unsafe.Pointer(%s), unsafe.Pointer(&src[0]), uintptr(size))\n", g.r) + g.emit("return src[size:], true\n") + }) + g.emit("}\n\n") +} diff --git a/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go b/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go index 0f9ef3e02..2edca399a 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go @@ -211,6 +211,36 @@ func (g *interfaceGenerator) emitMarshallableForPrimitiveNewtype(nt *ast.Ident) g.emit("}\n\n") } +func (g *interfaceGenerator) emitCheckedMarshallableForPrimitiveNewtype() { + g.emit("// CheckedMarshal implements marshal.CheckedMarshallable.CheckedMarshal.\n") + g.emit("func (%s *%s) CheckedMarshal(dst []byte) ([]byte, bool) {\n", g.r, g.typeName()) + g.inIndent(func() { + g.emit("size := %s.SizeBytes()\n", g.r) + g.emit("if size > len(dst) {\n") + g.inIndent(func() { + g.emit("return dst, false\n") + }) + g.emit("}\n") + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(%s), uintptr(size))\n", g.r) + g.emit("return dst[size:], true\n") + }) + g.emit("}\n\n") + + g.emit("// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal.\n") + g.emit("func (%s *%s) CheckedUnmarshal(src []byte) ([]byte, bool) {\n", g.r, g.typeName()) + g.inIndent(func() { + g.emit("size := %s.SizeBytes()\n", g.r) + g.emit("if size > len(src) {\n") + g.inIndent(func() { + g.emit("return src, false\n") + }) + g.emit("}\n") + g.emit("gohacks.Memmove(unsafe.Pointer(%s), unsafe.Pointer(&src[0]), uintptr(size))\n", g.r) + g.emit("return src[size:], true\n") + }) + g.emit("}\n\n") +} + func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Ident, slice *sliceAPI) { g.recordUsedImport("marshal") g.recordUsedImport("hostarch") diff --git a/tools/go_marshal/gomarshal/generator_interfaces_struct.go b/tools/go_marshal/gomarshal/generator_interfaces_struct.go index e7e4aef76..c254403bb 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_struct.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_struct.go @@ -435,6 +435,32 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("}\n\n") } +func (g *interfaceGenerator) emitCheckedMarshallableForStruct() { + g.emit("// CheckedMarshal implements marshal.CheckedMarshallable.CheckedMarshal.\n") + g.emit("func (%s *%s) CheckedMarshal(dst []byte) ([]byte, bool) {\n", g.r, g.typeName()) + g.inIndent(func() { + g.emit("if %s.SizeBytes() > len(dst) {\n", g.r) + g.inIndent(func() { + g.emit("return dst, false\n") + }) + g.emit("}\n") + g.emit("return %s.MarshalUnsafe(dst), true\n", g.r) + }) + g.emit("}\n\n") + + g.emit("// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal.\n") + g.emit("func (%s *%s) CheckedUnmarshal(src []byte) ([]byte, bool) {\n", g.r, g.typeName()) + g.inIndent(func() { + g.emit("if %s.SizeBytes() > len(src) {\n", g.r) + g.inIndent(func() { + g.emit("return src, false\n") + }) + g.emit("}\n") + g.emit("return %s.UnmarshalUnsafe(src), true\n", g.r) + }) + g.emit("}\n\n") +} + func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, slice *sliceAPI) { thisPacked := g.isStructPacked(st) diff --git a/tools/go_marshal/gomarshal/generator_tests.go b/tools/go_marshal/gomarshal/generator_tests.go index 8f93a1de5..a925f593f 100644 --- a/tools/go_marshal/gomarshal/generator_tests.go +++ b/tools/go_marshal/gomarshal/generator_tests.go @@ -216,7 +216,37 @@ func (g *testGenerator) emitTestSizeBytesOnTypedNilPtr() { }) } -func (g *testGenerator) emitTests(slice *sliceAPI) { +func (g *testGenerator) emitTestBoundCheck() { + g.inTestFunction("TestCheckedMethods", func() { + g.emit("var x %s\n", g.typeName()) + g.emit("size := x.SizeBytes()\n") + g.emit("b := make([]byte, size)\n\n") + + g.emit("if _, ok := x.CheckedMarshal(b[:size-1]); ok {\n") + g.inIndent(func() { + g.emit("t.Errorf(\"CheckedMarshal should have failed because buffer is small\")\n") + }) + g.emit("}\n") + g.emit("if _, ok := x.CheckedMarshal(b); !ok {\n") + g.inIndent(func() { + g.emit("t.Errorf(\"CheckedMarshal should have succeeded because buffer size is okay\")\n") + }) + g.emit("}\n\n") + + g.emit("if _, ok := x.CheckedUnmarshal(b[:size-1]); ok {\n") + g.inIndent(func() { + g.emit("t.Errorf(\"CheckedUnmarshal should have failed because buffer is small\")\n") + }) + g.emit("}\n") + g.emit("if _, ok := x.CheckedUnmarshal(b); !ok {\n") + g.inIndent(func() { + g.emit("t.Errorf(\"CheckedUnmarshal should have succeeded because buffer size is okay\")\n") + }) + g.emit("}\n") + }) +} + +func (g *testGenerator) emitTests(slice *sliceAPI, boundCheck bool) { g.emitTestNonZeroSize() g.emitTestSuspectAlignment() g.emitTestMarshalUnmarshalPreservesData() @@ -226,6 +256,9 @@ func (g *testGenerator) emitTests(slice *sliceAPI) { if slice != nil { g.emitTestMarshalUnmarshalSlicePreservesData(slice) } + if boundCheck { + g.emitTestBoundCheck() + } } func (g *testGenerator) write(out io.Writer) error { diff --git a/tools/go_marshal/test/test.go b/tools/go_marshal/test/test.go index e7e3ed74a..9c821c28a 100644 --- a/tools/go_marshal/test/test.go +++ b/tools/go_marshal/test/test.go @@ -39,7 +39,7 @@ type Type1 struct { // Type2 is a test data type. // -// +marshal +// +marshal boundCheck type Type2 struct { n int64 c byte @@ -134,12 +134,12 @@ type Stat struct { // InetAddr is an example marshallable newtype on an array. // -// +marshal +// +marshal boundCheck type InetAddr [4]byte // SignalSet is an example marshallable newtype on a primitive. // -// +marshal slice:SignalSetSlice:inner +// +marshal slice:SignalSetSlice:inner boundCheck type SignalSet uint64 // SignalSetAlias is an example newtype on another marshallable type.