From 0fd9b69d5ccd85c1958030c796470f56ec22fffa Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Fri, 19 Nov 2021 20:14:18 -0800 Subject: [PATCH] Add Checked methods to go_marshal. This is as per proposal in #6450. I have gated this behind a tag because this is a very sparsely used feature and otherwise will leads to a lot of unused generated code. Secondly, we can not generate the CheckUnmarshal method for dynamic types. So the dynamic tag would now require its users to additionally implement CheckUnmarshal method which is more cumbersome. Fixes #6450 PiperOrigin-RevId: 411197734 --- pkg/abi/linux/file.go | 2 +- pkg/lisafs/README.md | 3 - pkg/lisafs/client.go | 18 +- pkg/lisafs/client_file.go | 38 +- pkg/lisafs/connection.go | 25 +- pkg/lisafs/connection_test.go | 12 +- pkg/lisafs/fd.go | 2 +- pkg/lisafs/handlers.go | 109 +++- pkg/lisafs/message.go | 485 +++++++++++------- pkg/lisafs/sample_message.go | 33 +- pkg/lisafs/sock_test.go | 2 +- pkg/marshal/marshal.go | 29 ++ pkg/marshal/primitive/primitive.go | 16 +- tools/go_marshal/gomarshal/generator.go | 26 +- .../generator_interfaces_array_newtype.go | 30 ++ .../generator_interfaces_primitive_newtype.go | 30 ++ .../gomarshal/generator_interfaces_struct.go | 26 + tools/go_marshal/gomarshal/generator_tests.go | 35 +- tools/go_marshal/test/test.go | 6 +- 19 files changed, 666 insertions(+), 261 deletions(-) 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.