diff options
Diffstat (limited to 'pkg/p9')
| -rw-r--r-- | pkg/p9/BUILD | 2 | ||||
| -rw-r--r-- | pkg/p9/buffer_test.go | 31 | ||||
| -rw-r--r-- | pkg/p9/client.go | 6 | ||||
| -rw-r--r-- | pkg/p9/client_file.go | 4 | ||||
| -rw-r--r-- | pkg/p9/file.go | 151 | ||||
| -rw-r--r-- | pkg/p9/handlers.go | 697 | ||||
| -rw-r--r-- | pkg/p9/local_server/BUILD | 4 | ||||
| -rw-r--r-- | pkg/p9/local_server/local_server.go | 5 | ||||
| -rw-r--r-- | pkg/p9/messages_test.go | 37 | ||||
| -rw-r--r-- | pkg/p9/p9.go | 24 | ||||
| -rw-r--r-- | pkg/p9/p9test/BUILD | 76 | ||||
| -rw-r--r-- | pkg/p9/p9test/client_test.go | 2245 | ||||
| -rw-r--r-- | pkg/p9/p9test/mocks.go | 489 | ||||
| -rw-r--r-- | pkg/p9/p9test/p9test.go | 329 | ||||
| -rw-r--r-- | pkg/p9/path_tree.go | 109 | ||||
| -rw-r--r-- | pkg/p9/server.go | 228 | ||||
| -rw-r--r-- | pkg/p9/transport.go | 10 |
17 files changed, 3439 insertions, 1008 deletions
diff --git a/pkg/p9/BUILD b/pkg/p9/BUILD index 1cf5c6458..2c224e65b 100644 --- a/pkg/p9/BUILD +++ b/pkg/p9/BUILD @@ -15,6 +15,7 @@ go_library( "handlers.go", "messages.go", "p9.go", + "path_tree.go", "pool.go", "server.go", "transport.go", @@ -32,6 +33,7 @@ go_test( name = "p9_test", size = "small", srcs = [ + "buffer_test.go", "client_test.go", "messages_test.go", "p9_test.go", diff --git a/pkg/p9/buffer_test.go b/pkg/p9/buffer_test.go new file mode 100644 index 000000000..97eceefa7 --- /dev/null +++ b/pkg/p9/buffer_test.go @@ -0,0 +1,31 @@ +// Copyright 2018 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package p9 + +import ( + "testing" +) + +func TestBufferOverrun(t *testing.T) { + buf := &buffer{ + // This header indicates that a large string should follow, but + // it is only two bytes. Reading a string should cause an + // overrun. + data: []byte{0x0, 0x16}, + } + if s := buf.ReadString(); s != "" { + t.Errorf("overrun read got %s, want empty", s) + } +} diff --git a/pkg/p9/client.go b/pkg/p9/client.go index 3ebfab82a..67887874a 100644 --- a/pkg/p9/client.go +++ b/pkg/p9/client.go @@ -116,6 +116,7 @@ func NewClient(socket *unet.Socket, messageSize uint32, version string) (*Client msize: largestFixedSize, } } + // Compute a payload size and round to 512 (normal block size) // if it's larger than a single block. payloadSize := messageSize - largestFixedSize @@ -299,3 +300,8 @@ func (c *Client) sendRecv(t message, r message) error { func (c *Client) Version() uint32 { return c.version } + +// Close closes the underlying socket. +func (c *Client) Close() error { + return c.socket.Close() +} diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index 066639fda..992d1daf7 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -172,6 +172,9 @@ func (c *clientFile) SetAttr(valid SetAttrMask, attr SetAttr) error { } // Remove implements File.Remove. +// +// N.B. This method is no longer part of the file interface and should be +// considered deprecated. func (c *clientFile) Remove() error { // Avoid double close. if !atomic.CompareAndSwapUint32(&c.closed, 0, 1) { @@ -181,7 +184,6 @@ func (c *clientFile) Remove() error { // Send the remove message. if err := c.client.sendRecv(&Tremove{FID: c.fid}, &Rremove{}); err != nil { - log.Warningf("Tremove failed, losing FID %v: %v", c.fid, err) return err } diff --git a/pkg/p9/file.go b/pkg/p9/file.go index d2e89e373..55ceb52e1 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -31,35 +31,63 @@ type Attacher interface { // File is a set of operations corresponding to a single node. // -// Functions below MUST return syscall.Errno values. -// TODO: Enforce that with the type. +// Note that on the server side, the server logic places constraints on +// concurrent operations to make things easier. This may reduce the need for +// complex, error-prone locking and logic in the backend. These are documented +// for each method. // -// These must be implemented in all circumstances. +// There are three different types of guarantees provided: +// +// none: There is no concurrency guarantee. The method may be invoked +// concurrently with any other method on any other file. +// +// read: The method is guaranteed to be exclusive of any write or global +// operation that is mutating the state of the directory tree starting at this +// node. For example, this means creating new files, symlinks, directories or +// renaming a directory entry (or renaming in to this target), but the method +// may be called concurrently with other read methods. +// +// write: The method is guaranteed to be exclusive of any read, write or global +// operation that is mutating the state of the directory tree starting at this +// node, as described in read above. There may however, be other write +// operations executing concurrently on other components in the directory tree. +// +// global: The method is guaranteed to be exclusive of any read, write or +// global operation. type File interface { // Walk walks to the path components given in names. // // Walk returns QIDs in the same order that the names were passed in. // // An empty list of arguments should return a copy of the current file. + // + // On the server, Walk has a read concurrency guarantee. Walk(names []string) ([]QID, File, error) + // WalkGetAttr walks to the next file and returns its maximal set of + // attributes. + // + // Server-side p9.Files may return syscall.ENOSYS to indicate that Walk + // and GetAttr should be used separately to satisfy this request. + // + // On the server, WalkGetAttr has a read concurrency guarantee. + WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) + // StatFS returns information about the file system associated with // this file. + // + // On the server, StatFS has no concurrency guarantee. StatFS() (FSStat, error) // GetAttr returns attributes of this node. + // + // On the server, GetAttr has a read concurrency guarantee. GetAttr(req AttrMask) (QID, AttrMask, Attr, error) // SetAttr sets attributes on this node. - SetAttr(valid SetAttrMask, attr SetAttr) error - - // Remove removes the file. // - // This is deprecated in favor of UnlinkAt below. - Remove() error - - // Rename renames the file. - Rename(directory File, name string) error + // On the server, SetAttr has a write concurrency guarantee. + SetAttr(valid SetAttrMask, attr SetAttr) error // Close is called when all references are dropped on the server side, // and Close should be called by the client to drop all references. @@ -67,65 +95,93 @@ type File interface { // For server-side implementations of Close, the error is ignored. // // Close must be called even when Open has not been called. + // + // On the server, Close has no concurrency guarantee. Close() error - // Open is called prior to using read/write. + // Open must be called prior to using Read, Write or Readdir. Once Open + // is called, some operations, such as Walk, will no longer work. // - // The *fd.FD may be nil. If an *fd.FD is provided, ownership now - // belongs to the caller and the FD must be non-blocking. + // On the client, Open should be called only once. The fd return is + // optional, and may be nil. // - // If Open returns a non-nil *fd.FD, it should do so for all possible - // OpenFlags. If Open returns a nil *fd.FD, it should similarly return - // a nil *fd.FD for all possible OpenFlags. + // On the server, Open has a read concurrency guarantee. If an *fd.FD + // is provided, ownership now belongs to the caller. Open is guaranteed + // to be called only once. // - // This can be assumed to be one-shot only. + // N.B. The server must resolve any lazy paths when open is called. + // After this point, read and write may be called on files with no + // deletion check, so resolving in the data path is not viable. Open(mode OpenFlags) (*fd.FD, QID, uint32, error) - // Read reads from this file. + // Read reads from this file. Open must be called first. // // This may return io.EOF in addition to syscall.Errno values. // - // Preconditions: Open has been called and returned success. + // On the server, ReadAt has a read concurrency guarantee. See Open for + // additional requirements regarding lazy path resolution. ReadAt(p []byte, offset uint64) (int, error) - // Write writes to this file. + // Write writes to this file. Open must be called first. // // This may return io.EOF in addition to syscall.Errno values. // - // Preconditions: Open has been called and returned success. + // On the server, WriteAt has a read concurrency guarantee. See Open + // for additional requirements regarding lazy path resolution. WriteAt(p []byte, offset uint64) (int, error) - // FSync syncs this node. + // FSync syncs this node. Open must be called first. // - // Preconditions: Open has been called and returned success. + // On the server, FSync has a read concurrency guarantee. FSync() error // Create creates a new regular file and opens it according to the - // flags given. + // flags given. This file is already Open. + // + // N.B. On the client, the returned file is a reference to the current + // file, which now represents the created file. This is not the case on + // the server. These semantics are very subtle and can easily lead to + // bugs, but are a consequence of the 9P create operation. // // See p9.File.Open for a description of *fd.FD. + // + // On the server, Create has a write concurrency guarantee. Create(name string, flags OpenFlags, permissions FileMode, uid UID, gid GID) (*fd.FD, File, QID, uint32, error) // Mkdir creates a subdirectory. + // + // On the server, Mkdir has a write concurrency guarantee. Mkdir(name string, permissions FileMode, uid UID, gid GID) (QID, error) // Symlink makes a new symbolic link. - Symlink(oldname string, newname string, uid UID, gid GID) (QID, error) + // + // On the server, Symlink has a write concurrency guarantee. + Symlink(oldName string, newName string, uid UID, gid GID) (QID, error) // Link makes a new hard link. - Link(target File, newname string) error + // + // On the server, Link has a write concurrency guarantee. + Link(target File, newName string) error // Mknod makes a new device node. + // + // On the server, Mknod has a write concurrency guarantee. Mknod(name string, permissions FileMode, major uint32, minor uint32, uid UID, gid GID) (QID, error) + // Rename renames the file. + // + // Rename will never be called on the server, and RenameAt will always + // be used instead. + Rename(newDir File, newName string) error + // RenameAt renames a given file to a new name in a potentially new // directory. // - // oldname must be a name relative to this file, which must be a - // directory. newname is a name relative to newdir. + // oldName must be a name relative to this file, which must be a + // directory. newName is a name relative to newDir. // - // This is deprecated in favor of Rename. - RenameAt(oldname string, newdir File, newname string) error + // On the server, RenameAt has a global concurrency guarantee. + RenameAt(oldName string, newDir File, newName string) error // UnlinkAt the given named file. // @@ -133,16 +189,20 @@ type File interface { // // Flags are implementation-specific (e.g. O_DIRECTORY), but are // generally Linux unlinkat(2) flags. + // + // On the server, UnlinkAt has a write concurrency guarantee. UnlinkAt(name string, flags uint32) error // Readdir reads directory entries. // // This may return io.EOF in addition to syscall.Errno values. // - // Preconditions: Open has been called and returned success. + // On the server, Readdir has a read concurrency guarantee. Readdir(offset uint64, count uint32) ([]Dirent, error) // Readlink reads the link target. + // + // On the server, Readlink has a read concurrency guarantee. Readlink() (string, error) // Flush is called prior to Close. @@ -150,16 +210,11 @@ type File interface { // Whereas Close drops all references to the file, Flush cleans up the // file state. Behavior is implementation-specific. // - // Flush is not related to flush(9p). Flush is an extension to 9P2000.L, + // Flush is not related to flush(9p). Flush is an extension to 9P2000.L, // see version.go. - Flush() error - - // WalkGetAttr walks to the next file and returns its maximal set of - // attributes. // - // Server-side p9.Files may return syscall.ENOSYS to indicate that Walk - // and GetAttr should be used separately to satisfy this request. - WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) + // On the server, Flush has a read concurrency guarantee. + Flush() error // Connect establishes a new host-socket backed connection with a // socket. A File does not need to be opened before it can be connected @@ -170,8 +225,22 @@ type File interface { // // The returned FD must be non-blocking. // - // flags indicates the requested type of socket. + // Flags indicates the requested type of socket. + // + // On the server, Connect has a read concurrency guarantee. Connect(flags ConnectFlags) (*fd.FD, error) + + // Renamed is called when this node is renamed. + // + // This may not fail. The file will hold a reference to its parent + // within the p9 package, and is therefore safe to use for the lifetime + // of this File (until Close is called). + // + // This method should not be called by clients, who should use the + // relevant Rename methods. (Although the method will be a no-op.) + // + // On the server, Renamed has a global concurrency guarantee. + Renamed(newDir File, newName string) } // DefaultWalkGetAttr implements File.WalkGetAttr to return ENOSYS for server-side Files. diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 959dff31d..0d7a6138f 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -15,6 +15,7 @@ package p9 import ( + "fmt" "io" "os" "path" @@ -22,22 +23,43 @@ import ( "sync/atomic" "syscall" + "gvisor.googlesource.com/gvisor/pkg/fd" "gvisor.googlesource.com/gvisor/pkg/log" ) -// newErr returns a new error message from an error. -func newErr(err error) *Rlerror { +const maximumNameLength = 255 + +// ExtractErrno extracts a syscall.Errno from a error, best effort. +func ExtractErrno(err error) syscall.Errno { + switch err { + case os.ErrNotExist: + return syscall.ENOENT + case os.ErrExist: + return syscall.EEXIST + case os.ErrPermission: + return syscall.EACCES + case os.ErrInvalid: + return syscall.EINVAL + } + + // Attempt to unwrap. switch e := err.(type) { case syscall.Errno: - return &Rlerror{Error: uint32(e)} + return e case *os.PathError: - return newErr(e.Err) + return ExtractErrno(e.Err) case *os.SyscallError: - return newErr(e.Err) - default: - log.Warningf("unknown error: %v", err) - return &Rlerror{Error: uint32(syscall.EIO)} + return ExtractErrno(e.Err) } + + // Default case. + log.Warningf("unknown error: %v", err) + return syscall.EIO +} + +// newErr returns a new error message from an error. +func newErr(err error) *Rlerror { + return &Rlerror{Error: uint32(ExtractErrno(err))} } // handler is implemented for server-handled messages. @@ -85,13 +107,15 @@ func (t *Tflush) handle(cs *connState) message { return &Rflush{} } -// isSafeName returns true iff the name does not contain directory characters. -// -// We permit walks only on safe names and store the sequence of paths used for -// any given walk in each FID. (This is immutable.) We use this to mark -// relevant FIDs as moved when a successful rename occurs. -func isSafeName(name string) bool { - return name != "" && !strings.Contains(name, "/") && name != "." && name != ".." +// checkSafeName validates the name and returns nil or returns an error. +func checkSafeName(name string) error { + if name == "" || strings.Contains(name, "/") || name == "." || name == ".." { + return syscall.EINVAL + } + if len(name) > maximumNameLength { + return syscall.ENAMETOOLONG + } + return nil } // handle implements handler.handle. @@ -110,22 +134,54 @@ func (t *Tremove) handle(cs *connState) message { } defer ref.DecRef() + // Frustratingly, because we can't be guaranteed that a rename is not + // occurring simultaneously with this removal, we need to acquire the + // global rename lock for this kind of remove operation to ensure that + // ref.parent does not change out from underneath us. + // + // This is why Tremove is a bad idea, and clients should generally use + // Tunlinkat. All p9 clients will use Tunlinkat. + err := ref.safelyGlobal(func() error { + // Is this a root? Can't remove that. + if ref.isRoot() { + return syscall.EINVAL + } + + // N.B. this remove operation is permitted, even if the file is open. + // See also rename below for reasoning. + + // Is this file already deleted? + if ref.isDeleted() { + return syscall.EINVAL + } + + // Retrieve the file's proper name. + name := ref.parent.pathNode.nameFor(ref) + + // Attempt the removal. + if err := ref.parent.file.UnlinkAt(name, 0); err != nil { + return err + } + + // Mark all relevant fids as deleted. We don't need to lock any + // individual nodes because we already hold the global lock. + ref.parent.markChildDeleted(name) + return nil + }) + // "The remove request asks the file server both to remove the file // represented by fid and to clunk the fid, even if the remove fails." // // "It is correct to consider remove to be a clunk with the side effect // of removing the file if permissions allow." // https://swtch.com/plan9port/man/man9/remove.html - err := ref.file.Remove() - - // Clunk the FID regardless of Remove error. if !cs.DeleteFID(t.FID) { return newErr(syscall.EBADF) } - if err != nil { return newErr(err) } + return &Rremove{} } @@ -168,9 +224,12 @@ func (t *Tattach) handle(cs *connState) message { // Build a transient reference. root := &fidRef{ + server: cs.server, + parent: nil, file: sf, refs: 1, - walkable: attr.Mode.IsDir(), + mode: attr.Mode.FileType(), + pathNode: &cs.server.pathTree, } defer root.DecRef() @@ -183,20 +242,24 @@ func (t *Tattach) handle(cs *connState) message { // We want the same traversal checks to apply on attach, so always // attach at the root and use the regular walk paths. names := strings.Split(t.Auth.AttachName, "/") - _, target, _, attr, err := doWalk(cs, root, names) + _, newRef, _, attr, err := doWalk(cs, root, names) if err != nil { return newErr(err) } + defer newRef.DecRef() // Insert the FID. - cs.InsertFID(t.FID, &fidRef{ - file: target, - walkable: attr.Mode.IsDir(), - }) - + cs.InsertFID(t.FID, newRef) return &Rattach{} } +// CanOpen returns whether this file open can be opened, read and written to. +// +// This includes everything except symlinks and sockets. +func CanOpen(mode FileMode) bool { + return mode.IsRegular() || mode.IsDir() || mode.IsNamedPipe() || mode.IsBlockDevice() || mode.IsCharacterDevice() +} + // handle implements handler.handle. func (t *Tlopen) handle(cs *connState) message { // Lookup the FID. @@ -210,13 +273,35 @@ func (t *Tlopen) handle(cs *connState) message { defer ref.openedMu.Unlock() // Has it been opened already? - if ref.opened { + if ref.opened || !CanOpen(ref.mode) { return newErr(syscall.EINVAL) } - // Do the open. - osFile, qid, ioUnit, err := ref.file.Open(t.Flags) - if err != nil { + // Are flags valid? + if t.Flags&^OpenFlagsModeMask != 0 { + return newErr(syscall.EINVAL) + } + + // Is this an attempt to open a directory as writable? Don't accept. + if ref.mode.IsDir() && t.Flags != ReadOnly { + return newErr(syscall.EINVAL) + } + + var ( + qid QID + ioUnit uint32 + osFile *fd.FD + ) + if err := ref.safelyRead(func() (err error) { + // Has it been deleted already? + if ref.isDeleted() { + return syscall.EINVAL + } + + // Do the open. + osFile, qid, ioUnit, err = ref.file.Open(t.Flags) + return err + }); err != nil { return newErr(err) } @@ -229,8 +314,8 @@ func (t *Tlopen) handle(cs *connState) message { func (t *Tlcreate) do(cs *connState, uid UID) (*Rlcreate, error) { // Don't allow complex names. - if !isSafeName(t.Name) { - return nil, syscall.EINVAL + if err := checkSafeName(t.Name); err != nil { + return nil, err } // Lookup the FID. @@ -240,20 +325,48 @@ func (t *Tlcreate) do(cs *connState, uid UID) (*Rlcreate, error) { } defer ref.DecRef() - // Do the create. - osFile, nsf, qid, ioUnit, err := ref.file.Create(t.Name, t.OpenFlags, t.Permissions, uid, t.GID) - if err != nil { + var ( + osFile *fd.FD + nsf File + qid QID + ioUnit uint32 + newRef *fidRef + ) + if err := ref.safelyWrite(func() (err error) { + // Don't allow creation from non-directories or deleted directories. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Do the create. + osFile, nsf, qid, ioUnit, err = ref.file.Create(t.Name, t.OpenFlags, t.Permissions, uid, t.GID) + if err != nil { + return err + } + + newRef = &fidRef{ + server: cs.server, + parent: ref, + file: nsf, + opened: true, + openFlags: t.OpenFlags, + mode: ModeRegular, + pathNode: ref.pathNode.pathNodeFor(t.Name), + } + ref.pathNode.addChild(newRef, t.Name) + ref.IncRef() // Acquire parent reference. + return nil + }); err != nil { return nil, err } // Replace the FID reference. - // - // The new file will be opened already. - cs.InsertFID(t.FID, &fidRef{ - file: nsf, - opened: true, - openFlags: t.OpenFlags, - }) + cs.InsertFID(t.FID, newRef) return &Rlcreate{Rlopen: Rlopen{QID: qid, IoUnit: ioUnit, File: osFile}}, nil } @@ -278,8 +391,8 @@ func (t *Tsymlink) handle(cs *connState) message { func (t *Tsymlink) do(cs *connState, uid UID) (*Rsymlink, error) { // Don't allow complex names. - if !isSafeName(t.Name) { - return nil, syscall.EINVAL + if err := checkSafeName(t.Name); err != nil { + return nil, err } // Lookup the FID. @@ -289,9 +402,22 @@ func (t *Tsymlink) do(cs *connState, uid UID) (*Rsymlink, error) { } defer ref.DecRef() - // Do the symlink. - qid, err := ref.file.Symlink(t.Target, t.Name, uid, t.GID) - if err != nil { + var qid QID + if err := ref.safelyWrite(func() (err error) { + // Don't allow symlinks from non-directories or deleted directories. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Do the symlink. + qid, err = ref.file.Symlink(t.Target, t.Name, uid, t.GID) + return err + }); err != nil { return nil, err } @@ -301,8 +427,8 @@ func (t *Tsymlink) do(cs *connState, uid UID) (*Rsymlink, error) { // handle implements handler.handle. func (t *Tlink) handle(cs *connState) message { // Don't allow complex names. - if !isSafeName(t.Name) { - return newErr(syscall.EINVAL) + if err := checkSafeName(t.Name); err != nil { + return newErr(err) } // Lookup the FID. @@ -319,8 +445,20 @@ func (t *Tlink) handle(cs *connState) message { } defer refTarget.DecRef() - // Do the link. - if err := ref.file.Link(refTarget.file, t.Name); err != nil { + if err := ref.safelyWrite(func() (err error) { + // Don't allow create links from non-directories or deleted directories. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Do the link. + return ref.file.Link(refTarget.file, t.Name) + }); err != nil { return newErr(err) } @@ -330,8 +468,11 @@ func (t *Tlink) handle(cs *connState) message { // handle implements handler.handle. func (t *Trenameat) handle(cs *connState) message { // Don't allow complex names. - if !isSafeName(t.OldName) || !isSafeName(t.NewName) { - return newErr(syscall.EINVAL) + if err := checkSafeName(t.OldName); err != nil { + return newErr(err) + } + if err := checkSafeName(t.NewName); err != nil { + return newErr(err) } // Lookup the FID. @@ -348,8 +489,32 @@ func (t *Trenameat) handle(cs *connState) message { } defer refTarget.DecRef() - // Do the rename. - if err := ref.file.RenameAt(t.OldName, refTarget.file, t.NewName); err != nil { + // Perform the rename holding the global lock. + if err := ref.safelyGlobal(func() (err error) { + // Don't allow renaming across deleted directories. + if ref.isDeleted() || !ref.mode.IsDir() || refTarget.isDeleted() || !refTarget.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Is this the same file? If yes, short-circuit and return success. + if ref.pathNode == refTarget.pathNode && t.OldName == t.NewName { + return nil + } + + // Attempt the actual rename. + if err := ref.file.RenameAt(t.OldName, refTarget.file, t.NewName); err != nil { + return err + } + + // Update the path tree. + ref.renameChildTo(t.OldName, refTarget, t.NewName) + return nil + }); err != nil { return newErr(err) } @@ -359,8 +524,8 @@ func (t *Trenameat) handle(cs *connState) message { // handle implements handler.handle. func (t *Tunlinkat) handle(cs *connState) message { // Don't allow complex names. - if !isSafeName(t.Name) { - return newErr(syscall.EINVAL) + if err := checkSafeName(t.Name); err != nil { + return newErr(err) } // Lookup the FID. @@ -370,8 +535,40 @@ func (t *Tunlinkat) handle(cs *connState) message { } defer ref.DecRef() - // Do the unlink. - if err := ref.file.UnlinkAt(t.Name, t.Flags); err != nil { + if err := ref.safelyWrite(func() (err error) { + // Don't allow deletion from non-directories or deleted directories. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Before we do the unlink itself, we need to ensure that there + // are no operations in flight on associated path node. The + // child's path node lock must be held to ensure that the + // unlink at marking the child deleted below is atomic with + // respect to any other read or write operations. + // + // This is one case where we have a lock ordering issue, but + // since we always acquire deeper in the hierarchy, we know + // that we are free of lock cycles. + childPathNode := ref.pathNode.pathNodeFor(t.Name) + childPathNode.mu.Lock() + defer childPathNode.mu.Unlock() + + // Do the unlink. + err = ref.file.UnlinkAt(t.Name, t.Flags) + if err != nil { + return err + } + + // Mark the path as deleted. + ref.markChildDeleted(t.Name) + return nil + }); err != nil { return newErr(err) } @@ -381,8 +578,8 @@ func (t *Tunlinkat) handle(cs *connState) message { // handle implements handler.handle. func (t *Trename) handle(cs *connState) message { // Don't allow complex names. - if !isSafeName(t.Name) { - return newErr(syscall.EINVAL) + if err := checkSafeName(t.Name); err != nil { + return newErr(err) } // Lookup the FID. @@ -399,8 +596,43 @@ func (t *Trename) handle(cs *connState) message { } defer refTarget.DecRef() - // Call the rename method. - if err := ref.file.Rename(refTarget.file, t.Name); err != nil { + if err := ref.safelyGlobal(func() (err error) { + // Don't allow a root rename. + if ref.isRoot() { + return syscall.EINVAL + } + + // Don't allow renaming deleting entries, or target non-directories. + if ref.isDeleted() || refTarget.isDeleted() || !refTarget.mode.IsDir() { + return syscall.EINVAL + } + + // If the parent is deleted, but we not, something is seriously wrong. + // It's fail to die at this point with an assertion failure. + if ref.parent.isDeleted() { + panic(fmt.Sprintf("parent %+v deleted, child %+v is not", ref.parent, ref)) + } + + // N.B. The rename operation is allowed to proceed on open files. It + // does impact the state of its parent, but this is merely a sanity + // check in any case, and the operation is safe. There may be other + // files corresponding to the same path that are renamed anyways. + + // Check for the exact same file and short-circuit. + oldName := ref.parent.pathNode.nameFor(ref) + if ref.parent.pathNode == refTarget.pathNode && oldName == t.Name { + return nil + } + + // Call the rename method on the parent. + if err := ref.parent.file.RenameAt(oldName, refTarget.file, t.Name); err != nil { + return err + } + + // Update the path tree. + ref.parent.renameChildTo(oldName, refTarget, t.Name) + return nil + }); err != nil { return newErr(err) } @@ -416,9 +648,19 @@ func (t *Treadlink) handle(cs *connState) message { } defer ref.DecRef() - // Do the read. - target, err := ref.file.Readlink() - if err != nil { + var target string + if err := ref.safelyRead(func() (err error) { + // Don't allow readlink on deleted files. There is no need to + // check if this file is opened because symlinks cannot be + // opened. + if ref.isDeleted() || !ref.mode.IsSymlink() { + return syscall.EINVAL + } + + // Do the read. + target, err = ref.file.Readlink() + return err + }); err != nil { return newErr(err) } @@ -434,26 +676,30 @@ func (t *Tread) handle(cs *connState) message { } defer ref.DecRef() - // Has it been opened already? - openFlags, opened := ref.OpenFlags() - if !opened { - return newErr(syscall.EINVAL) - } - - // Can it be read? Check permissions. - if openFlags&OpenFlagsModeMask == WriteOnly { - return newErr(syscall.EPERM) - } - // Constrain the size of the read buffer. if int(t.Count) > int(maximumLength) { return newErr(syscall.ENOBUFS) } - // Do the read. - data := make([]byte, t.Count) - n, err := ref.file.ReadAt(data, t.Offset) - if err != nil && err != io.EOF { + var ( + data = make([]byte, t.Count) + n int + ) + if err := ref.safelyRead(func() (err error) { + // Has it been opened already? + openFlags, opened := ref.OpenFlags() + if !opened { + return syscall.EINVAL + } + + // Can it be read? Check permissions. + if openFlags&OpenFlagsModeMask == WriteOnly { + return syscall.EPERM + } + + n, err = ref.file.ReadAt(data, t.Offset) + return err + }); err != nil && err != io.EOF { return newErr(err) } @@ -469,20 +715,22 @@ func (t *Twrite) handle(cs *connState) message { } defer ref.DecRef() - // Has it been opened already? - openFlags, opened := ref.OpenFlags() - if !opened { - return newErr(syscall.EINVAL) - } + var n int + if err := ref.safelyRead(func() (err error) { + // Has it been opened already? + openFlags, opened := ref.OpenFlags() + if !opened { + return syscall.EINVAL + } - // Can it be write? Check permissions. - if openFlags&OpenFlagsModeMask == ReadOnly { - return newErr(syscall.EPERM) - } + // Can it be write? Check permissions. + if openFlags&OpenFlagsModeMask == ReadOnly { + return syscall.EPERM + } - // Do the write. - n, err := ref.file.WriteAt(t.Data, t.Offset) - if err != nil { + n, err = ref.file.WriteAt(t.Data, t.Offset) + return err + }); err != nil { return newErr(err) } @@ -500,8 +748,8 @@ func (t *Tmknod) handle(cs *connState) message { func (t *Tmknod) do(cs *connState, uid UID) (*Rmknod, error) { // Don't allow complex names. - if !isSafeName(t.Name) { - return nil, syscall.EINVAL + if err := checkSafeName(t.Name); err != nil { + return nil, err } // Lookup the FID. @@ -511,9 +759,22 @@ func (t *Tmknod) do(cs *connState, uid UID) (*Rmknod, error) { } defer ref.DecRef() - // Do the mknod. - qid, err := ref.file.Mknod(t.Name, t.Permissions, t.Major, t.Minor, uid, t.GID) - if err != nil { + var qid QID + if err := ref.safelyWrite(func() (err error) { + // Don't allow mknod on deleted files. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Do the mknod. + qid, err = ref.file.Mknod(t.Name, t.Permissions, t.Major, t.Minor, uid, t.GID) + return err + }); err != nil { return nil, err } @@ -531,8 +792,8 @@ func (t *Tmkdir) handle(cs *connState) message { func (t *Tmkdir) do(cs *connState, uid UID) (*Rmkdir, error) { // Don't allow complex names. - if !isSafeName(t.Name) { - return nil, syscall.EINVAL + if err := checkSafeName(t.Name); err != nil { + return nil, err } // Lookup the FID. @@ -542,9 +803,22 @@ func (t *Tmkdir) do(cs *connState, uid UID) (*Rmkdir, error) { } defer ref.DecRef() - // Do the mkdir. - qid, err := ref.file.Mkdir(t.Name, t.Permissions, uid, t.GID) - if err != nil { + var qid QID + if err := ref.safelyWrite(func() (err error) { + // Don't allow mkdir on deleted files. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Not allowed on open directories. + if _, opened := ref.OpenFlags(); opened { + return syscall.EINVAL + } + + // Do the mkdir. + qid, err = ref.file.Mkdir(t.Name, t.Permissions, uid, t.GID) + return err + }); err != nil { return nil, err } @@ -560,9 +834,20 @@ func (t *Tgetattr) handle(cs *connState) message { } defer ref.DecRef() - // Get attributes. - qid, valid, attr, err := ref.file.GetAttr(t.AttrMask) - if err != nil { + // We allow getattr on deleted files. Depending on the backing + // implementation, it's possible that races exist that might allow + // fetching attributes of other files. But we need to generally allow + // refreshing attributes and this is a minor leak, if at all. + + var ( + qid QID + valid AttrMask + attr Attr + ) + if err := ref.safelyRead(func() (err error) { + qid, valid, attr, err = ref.file.GetAttr(t.AttrMask) + return err + }); err != nil { return newErr(err) } @@ -578,8 +863,18 @@ func (t *Tsetattr) handle(cs *connState) message { } defer ref.DecRef() - // Set attributes. - if err := ref.file.SetAttr(t.Valid, t.SetAttr); err != nil { + if err := ref.safelyWrite(func() error { + // We don't allow setattr on files that have been deleted. + // This might be technically incorrect, as it's possible that + // there were multiple links and you can still change the + // corresponding inode information. + if ref.isDeleted() { + return syscall.EINVAL + } + + // Set the attributes. + return ref.file.SetAttr(t.Valid, t.SetAttr) + }); err != nil { return newErr(err) } @@ -621,14 +916,25 @@ func (t *Treaddir) handle(cs *connState) message { } defer ref.DecRef() - // Has it been opened already? - if _, opened := ref.OpenFlags(); !opened { - return newErr(syscall.EINVAL) - } + var entries []Dirent + if err := ref.safelyRead(func() (err error) { + // Don't allow reading deleted directories. + if ref.isDeleted() || !ref.mode.IsDir() { + return syscall.EINVAL + } + + // Has it been opened already? + if _, opened := ref.OpenFlags(); !opened { + return syscall.EINVAL + } - // Read the entries. - entries, err := ref.file.Readdir(t.Offset, t.Count) - if err != nil && err != io.EOF { + // Read the entries. + entries, err = ref.file.Readdir(t.Offset, t.Count) + if err != nil && err != io.EOF { + return err + } + return nil + }); err != nil { return newErr(err) } @@ -644,13 +950,15 @@ func (t *Tfsync) handle(cs *connState) message { } defer ref.DecRef() - // Has it been opened already? - if _, opened := ref.OpenFlags(); !opened { - return newErr(syscall.EINVAL) - } + if err := ref.safelyRead(func() (err error) { + // Has it been opened already? + if _, opened := ref.OpenFlags(); !opened { + return syscall.EINVAL + } - err := ref.file.FSync() - if err != nil { + // Perform the sync. + return ref.file.FSync() + }); err != nil { return newErr(err) } @@ -671,6 +979,11 @@ func (t *Tstatfs) handle(cs *connState) message { return newErr(err) } + // Constrain the name length. + if st.NameLength > maximumNameLength { + st.NameLength = maximumNameLength + } + return &Rstatfs{st} } @@ -682,7 +995,7 @@ func (t *Tflushf) handle(cs *connState) message { } defer ref.DecRef() - if err := ref.file.Flush(); err != nil { + if err := ref.safelyRead(ref.file.Flush); err != nil { return newErr(err) } @@ -726,12 +1039,14 @@ func walkOne(qids []QID, from File, names []string) ([]QID, File, AttrMask, Attr // doWalk walks from a given fidRef. // -// This enforces that all intermediate nodes are walkable (directories). -func doWalk(cs *connState, ref *fidRef, names []string) (qids []QID, sf File, valid AttrMask, attr Attr, err error) { +// This enforces that all intermediate nodes are walkable (directories). The +// fidRef returned (newRef) has a reference associated with it that is now +// owned by the caller and must be handled appropriately. +func doWalk(cs *connState, ref *fidRef, names []string) (qids []QID, newRef *fidRef, valid AttrMask, attr Attr, err error) { // Check the names. for _, name := range names { - if !isSafeName(name) { - err = syscall.EINVAL + err = checkSafeName(name) + if err != nil { return } } @@ -745,44 +1060,88 @@ func doWalk(cs *connState, ref *fidRef, names []string) (qids []QID, sf File, va // Is this an empty list? Handle specially. We don't actually need to // validate anything since this is always permitted. if len(names) == 0 { - return walkOne(nil, ref.file, nil) - } - - // Is it walkable? - if !ref.walkable { - err = syscall.EINVAL - return + var sf File // Temporary. + if err := ref.maybeParent().safelyRead(func() (err error) { + // Clone the single element. + qids, sf, valid, attr, err = walkOne(nil, ref.file, nil) + if err != nil { + return err + } + + newRef = &fidRef{ + server: cs.server, + parent: ref.parent, + file: sf, + mode: ref.mode, + pathNode: ref.pathNode, + + // For the clone case, the cloned fid must + // preserve the deleted property of the + // original FID. + deleted: ref.deleted, + } + if !ref.isRoot() { + if !newRef.isDeleted() { + // Add only if a non-root node; the same node. + ref.parent.pathNode.addChild(newRef, ref.parent.pathNode.nameFor(ref)) + } + ref.parent.IncRef() // Acquire parent reference. + } + // doWalk returns a reference. + newRef.IncRef() + return nil + }); err != nil { + return nil, nil, AttrMask{}, Attr{}, err + } + return qids, newRef, valid, attr, nil } - from := ref.file // Start at the passed ref. - // Do the walk, one element at a time. + walkRef := ref + walkRef.IncRef() for i := 0; i < len(names); i++ { - qids, sf, valid, attr, err = walkOne(qids, from, names[i:i+1]) - - // Close the intermediate file. Note that we don't close the - // first file because in that case we are walking from the - // existing reference. - if i > 0 { - from.Close() - } - from = sf // Use the new file. - - // Was there an error walking? - if err != nil { - return nil, nil, AttrMask{}, Attr{}, err - } - // We won't allow beyond past symlinks; stop here if this isn't // a proper directory and we have additional paths to walk. - if !valid.Mode || (!attr.Mode.IsDir() && i < len(names)-1) { - from.Close() // Not using the file object. + if !walkRef.mode.IsDir() { + walkRef.DecRef() // Drop walk reference; no lock required. return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL } + + var sf File // Temporary. + if err := walkRef.safelyRead(func() (err error) { + qids, sf, valid, attr, err = walkOne(qids, walkRef.file, names[i:i+1]) + if err != nil { + return err + } + + // Note that we don't need to acquire a lock on any of + // these individual instances. That's because they are + // not actually addressable via a FID. They are + // anonymous. They exist in the tree for tracking + // purposes. + newRef := &fidRef{ + server: cs.server, + parent: walkRef, + file: sf, + mode: attr.Mode.FileType(), + pathNode: walkRef.pathNode.pathNodeFor(names[i]), + } + walkRef.pathNode.addChild(newRef, names[i]) + // We allow our walk reference to become the new parent + // reference here and so we don't IncRef. Instead, just + // set walkRef to the newRef above and acquire a new + // walk reference. + walkRef = newRef + walkRef.IncRef() + return nil + }); err != nil { + walkRef.DecRef() // Drop the old walkRef. + return nil, nil, AttrMask{}, Attr{}, err + } } // Success. - return qids, sf, valid, attr, nil + return qids, walkRef, valid, attr, nil } // handle implements handler.handle. @@ -795,17 +1154,14 @@ func (t *Twalk) handle(cs *connState) message { defer ref.DecRef() // Do the walk. - qids, sf, _, attr, err := doWalk(cs, ref, t.Names) + qids, newRef, _, _, err := doWalk(cs, ref, t.Names) if err != nil { return newErr(err) } + defer newRef.DecRef() // Install the new FID. - cs.InsertFID(t.NewFID, &fidRef{ - file: sf, - walkable: attr.Mode.IsDir(), - }) - + cs.InsertFID(t.NewFID, newRef) return &Rwalk{QIDs: qids} } @@ -819,17 +1175,14 @@ func (t *Twalkgetattr) handle(cs *connState) message { defer ref.DecRef() // Do the walk. - qids, sf, valid, attr, err := doWalk(cs, ref, t.Names) + qids, newRef, valid, attr, err := doWalk(cs, ref, t.Names) if err != nil { return newErr(err) } + defer newRef.DecRef() // Install the new FID. - cs.InsertFID(t.NewFID, &fidRef{ - file: sf, - walkable: attr.Mode.IsDir(), - }) - + cs.InsertFID(t.NewFID, newRef) return &Rwalkgetattr{QIDs: qids, Valid: valid, Attr: attr} } @@ -878,9 +1231,17 @@ func (t *Tlconnect) handle(cs *connState) message { } defer ref.DecRef() - // Do the connect. - osFile, err := ref.file.Connect(t.Flags) - if err != nil { + var osFile *fd.FD + if err := ref.safelyRead(func() (err error) { + // Don't allow connecting to deleted files. + if ref.isDeleted() || !ref.mode.IsSocket() { + return syscall.EINVAL + } + + // Do the connect. + osFile, err = ref.file.Connect(t.Flags) + return err + }); err != nil { return newErr(err) } diff --git a/pkg/p9/local_server/BUILD b/pkg/p9/local_server/BUILD index 8229e6308..b17ebb79d 100644 --- a/pkg/p9/local_server/BUILD +++ b/pkg/p9/local_server/BUILD @@ -1,7 +1,7 @@ -package(licenses = ["notice"]) # Apache 2.0 - load("@io_bazel_rules_go//go:def.bzl", "go_binary") +package(licenses = ["notice"]) # Apache 2.0 + go_binary( name = "local_server", srcs = ["local_server.go"], diff --git a/pkg/p9/local_server/local_server.go b/pkg/p9/local_server/local_server.go index 1e6aaa762..69b90c6cd 100644 --- a/pkg/p9/local_server/local_server.go +++ b/pkg/p9/local_server/local_server.go @@ -318,6 +318,11 @@ func (l *local) Connect(p9.ConnectFlags) (*fd.FD, error) { return nil, syscall.ECONNREFUSED } +// Renamed implements p9.File.Renamed. +func (l *local) Renamed(parent p9.File, newName string) { + l.path = path.Join(parent.(*local).path, newName) +} + func main() { log.SetLevel(log.Debug) diff --git a/pkg/p9/messages_test.go b/pkg/p9/messages_test.go index dfb41bb76..c0d65d82c 100644 --- a/pkg/p9/messages_test.go +++ b/pkg/p9/messages_test.go @@ -15,6 +15,7 @@ package p9 import ( + "fmt" "reflect" "testing" ) @@ -186,6 +187,13 @@ func TestEncodeDecode(t *testing.T) { &Rxattrwalk{ Size: 1, }, + &Txattrcreate{ + FID: 1, + Name: "a", + AttrSize: 2, + Flags: 3, + }, + &Rxattrcreate{}, &Treaddir{ Directory: 1, Offset: 2, @@ -389,3 +397,32 @@ func TestEncodeDecode(t *testing.T) { } } } + +func TestMessageStrings(t *testing.T) { + for typ, fn := range messageRegistry { + name := fmt.Sprintf("%+v", typ) + t.Run(name, func(t *testing.T) { + defer func() { // Ensure no panic. + if r := recover(); r != nil { + t.Errorf("printing %s failed: %v", name, r) + } + }() + m := fn() + _ = fmt.Sprintf("%v", m) + err := ErrInvalidMsgType{typ} + _ = err.Error() + }) + } +} + +func TestRegisterDuplicate(t *testing.T) { + defer func() { + if r := recover(); r == nil { + // We expect a panic. + t.FailNow() + } + }() + + // Register a duplicate. + register(&Rlerror{}) +} diff --git a/pkg/p9/p9.go b/pkg/p9/p9.go index 3b0993ecd..be644e7bf 100644 --- a/pkg/p9/p9.go +++ b/pkg/p9/p9.go @@ -984,6 +984,30 @@ func (s *SetAttr) Encode(b *buffer) { b.Write64(s.MTimeNanoSeconds) } +// Apply applies this to the given Attr. +func (a *Attr) Apply(mask SetAttrMask, attr SetAttr) { + if mask.Permissions { + a.Mode = a.Mode&^PermissionsMask | (attr.Permissions & PermissionsMask) + } + if mask.UID { + a.UID = attr.UID + } + if mask.GID { + a.GID = attr.GID + } + if mask.Size { + a.Size = attr.Size + } + if mask.ATime { + a.ATimeSeconds = attr.ATimeSeconds + a.ATimeNanoSeconds = attr.ATimeNanoSeconds + } + if mask.MTime { + a.MTimeSeconds = attr.MTimeSeconds + a.MTimeNanoSeconds = attr.MTimeNanoSeconds + } +} + // Dirent is used for readdir. type Dirent struct { // QID is the entry QID. diff --git a/pkg/p9/p9test/BUILD b/pkg/p9/p9test/BUILD index d6f428e11..7c4b875ce 100644 --- a/pkg/p9/p9test/BUILD +++ b/pkg/p9/p9test/BUILD @@ -1,16 +1,60 @@ +load("//tools/go_stateify:defs.bzl", "go_library", "go_test") +load("@io_bazel_rules_go//go:def.bzl", "go_binary") + package(licenses = ["notice"]) # Apache 2.0 -load("//tools/go_stateify:defs.bzl", "go_library", "go_test") +alias( + name = "mockgen", + actual = "@com_github_golang_mock//mockgen:mockgen", +) -go_test( - name = "p9test_test", - size = "small", - srcs = ["client_test.go"], - embed = [":p9test"], +MOCK_SRC_PACKAGE = "gvisor.googlesource.com/gvisor/pkg/p9" + +# mockgen_reflect is a source file that contains mock generation code that +# imports the p9 package and generates a specification via reflection. The +# usual generation path must be split into two distinct parts because the full +# source tree is not available to all build targets. Only declared depencies +# are available (and even then, not the Go source files). +genrule( + name = "mockgen_reflect", + testonly = 1, + outs = ["mockgen_reflect.go"], + cmd = ( + "$(location :mockgen) " + + "-package p9test " + + "-prog_only " + MOCK_SRC_PACKAGE + " " + + "Attacher,File > $@" + ), + tools = [":mockgen"], +) + +# mockgen_exec is the binary that includes the above reflection generator. +# Running this binary will emit an encoded version of the p9 Attacher and File +# structures. This is consumed by the mocks genrule, below. +go_binary( + name = "mockgen_exec", + testonly = 1, + srcs = ["mockgen_reflect.go"], deps = [ - "//pkg/fd", "//pkg/p9", - "//pkg/unet", + "@com_github_golang_mock//mockgen/model:go_default_library", + ], +) + +# mocks consumes the encoded output above, and generates the full source for a +# set of mocks. These are included directly in the p9test library. +genrule( + name = "mocks", + testonly = 1, + outs = ["mocks.go"], + cmd = ( + "$(location :mockgen) " + + "-package p9test " + + "-exec_only $(location :mockgen_exec) " + MOCK_SRC_PACKAGE + " File > $@" + ), + tools = [ + ":mockgen", + ":mockgen_exec", ], ) @@ -18,11 +62,27 @@ go_library( name = "p9test", srcs = [ "mocks.go", + "p9test.go", ], importpath = "gvisor.googlesource.com/gvisor/pkg/p9/p9test", visibility = ["//:sandbox"], deps = [ "//pkg/fd", + "//pkg/log", + "//pkg/p9", + "//pkg/unet", + "@com_github_golang_mock//gomock:go_default_library", + ], +) + +go_test( + name = "client_test", + size = "small", + srcs = ["client_test.go"], + embed = [":p9test"], + deps = [ + "//pkg/fd", "//pkg/p9", + "@com_github_golang_mock//gomock:go_default_library", ], ) diff --git a/pkg/p9/p9test/client_test.go b/pkg/p9/p9test/client_test.go index db562b9ba..242d81b95 100644 --- a/pkg/p9/p9test/client_test.go +++ b/pkg/p9/p9test/client_test.go @@ -15,360 +15,2059 @@ package p9test import ( - "io/ioutil" + "bytes" + "fmt" + "io" + "math/rand" "os" "reflect" + "strings" + "sync" "syscall" "testing" + "time" + "github.com/golang/mock/gomock" "gvisor.googlesource.com/gvisor/pkg/fd" "gvisor.googlesource.com/gvisor/pkg/p9" - "gvisor.googlesource.com/gvisor/pkg/unet" ) -func TestDonateFD(t *testing.T) { - // Temporary file. - osFile, err := ioutil.TempFile("", "p9") +func TestPanic(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + // Create a new root. + d := h.NewDirectory(nil)(nil) + defer d.Close() // Needed manually. + h.Attacher.EXPECT().Attach().Return(d, nil).Do(func() { + // Panic here, and ensure that we get back EFAULT. + panic("handler") + }) + + // Attach to the client. + if _, err := c.Attach("/"); err != syscall.EFAULT { + t.Fatalf("got attach err %v, want EFAULT", err) + } +} + +func TestAttachNoLeak(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + // Create a new root. + d := h.NewDirectory(nil)(nil) + h.Attacher.EXPECT().Attach().Return(d, nil).Times(1) + + // Attach to the client. + f, err := c.Attach("/") if err != nil { - t.Fatalf("could not create temporary file: %v", err) + t.Fatalf("got attach err %v, want nil", err) + } + + // Don't close the file. This should be closed automatically when the + // client disconnects. The mock asserts that everything is closed + // exactly once. This statement just removes the unused variable error. + _ = f +} + +func TestBadAttach(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + // Return an error on attach. + h.Attacher.EXPECT().Attach().Return(nil, syscall.EINVAL).Times(1) + + // Attach to the client. + if _, err := c.Attach("/"); err != syscall.EINVAL { + t.Fatalf("got attach err %v, want syscall.EINVAL", err) } - os.Remove(osFile.Name()) +} - hfi, err := osFile.Stat() +func TestWalkAttach(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + // Create a new root. + d := h.NewDirectory(map[string]Generator{ + "a": h.NewDirectory(map[string]Generator{ + "b": h.NewFile(), + }), + })(nil) + h.Attacher.EXPECT().Attach().Return(d, nil).Times(1) + + // Attach to the client as a non-root, and ensure that the walk above + // occurs as expected. We should get back b, and all references should + // be dropped when the file is closed. + f, err := c.Attach("/a/b") if err != nil { - osFile.Close() - t.Fatalf("stat failed: %v", err) + t.Fatalf("got attach err %v, want nil", err) } - osFileStat := hfi.Sys().(*syscall.Stat_t) + defer f.Close() - f, err := fd.NewFromFile(osFile) - // osFile should always be closed. - osFile.Close() + // Check that's a regular file. + if _, _, attr, err := f.GetAttr(p9.AttrMaskAll()); err != nil { + t.Errorf("got err %v, want nil", err) + } else if !attr.Mode.IsRegular() { + t.Errorf("got mode %v, want regular file", err) + } +} + +// newTypeMap returns a new type map dictionary. +func newTypeMap(h *Harness) map[string]Generator { + return map[string]Generator{ + "directory": h.NewDirectory(map[string]Generator{}), + "file": h.NewFile(), + "symlink": h.NewSymlink(), + "block-device": h.NewBlockDevice(), + "character-device": h.NewCharacterDevice(), + "named-pipe": h.NewNamedPipe(), + "socket": h.NewSocket(), + } +} + +// newRoot returns a new root filesystem. +// +// This is set up in a deterministic way for testing most operations. +// +// The represented file system looks like: +// - file +// - symlink +// - directory +// ... +// + one +// - file +// - symlink +// - directory +// ... +// + two +// - file +// - symlink +// - directory +// ... +// + three +// - file +// - symlink +// - directory +// ... +func newRoot(h *Harness, c *p9.Client) (*Mock, p9.File) { + root := newTypeMap(h) + one := newTypeMap(h) + two := newTypeMap(h) + three := newTypeMap(h) + one["two"] = h.NewDirectory(two) // Will be nested in one. + root["one"] = h.NewDirectory(one) // Top level. + root["three"] = h.NewDirectory(three) // Alternate top-level. + + // Create a new root. + rootBackend := h.NewDirectory(root)(nil) + h.Attacher.EXPECT().Attach().Return(rootBackend, nil) + + // Attach to the client. + r, err := c.Attach("/") if err != nil { - t.Fatalf("unable to create file: %v", err) + h.t.Fatalf("got attach err %v, want nil", err) } - // Craft attacher to attach to the mocked file which will return our - // temporary file. - fileMock := &FileMock{ - OpenMock: OpenMock{File: f}, - GetAttrMock: GetAttrMock{ - // The mode must be valid always. - Valid: p9.AttrMask{Mode: true}, - }, + return rootBackend, r +} + +func allInvalidNames(from string) []string { + return []string{ + from + "/other", + from + "/..", + from + "/.", + from + "/", + "other/" + from, + "/" + from, + "./" + from, + "../" + from, + ".", + "..", + "/", + "", + } +} + +func TestWalkInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Run relevant tests. + for name := range newTypeMap(h) { + // These are all the various ways that one might attempt to + // construct compound paths. They should all be rejected, as + // any compound that contains a / is not allowed, as well as + // the singular paths of '.' and '..'. + if _, _, err := root.Walk([]string{".", name}); err != syscall.EINVAL { + t.Errorf("Walk through . %s wanted EINVAL, got %v", name, err) + } + if _, _, err := root.Walk([]string{"..", name}); err != syscall.EINVAL { + t.Errorf("Walk through . %s wanted EINVAL, got %v", name, err) + } + if _, _, err := root.Walk([]string{name, "."}); err != syscall.EINVAL { + t.Errorf("Walk through %s . wanted EINVAL, got %v", name, err) + } + if _, _, err := root.Walk([]string{name, ".."}); err != syscall.EINVAL { + t.Errorf("Walk through %s .. wanted EINVAL, got %v", name, err) + } + for _, invalidName := range allInvalidNames(name) { + if _, _, err := root.Walk([]string{invalidName}); err != syscall.EINVAL { + t.Errorf("Walk through %s wanted EINVAL, got %v", invalidName, err) + } + } + wantErr := syscall.EINVAL + if name == "directory" { + // We can attempt a walk through a directory. However, + // we should never see a file named "other", so we + // expect this to return ENOENT. + wantErr = syscall.ENOENT + } + if _, _, err := root.Walk([]string{name, "other"}); err != wantErr { + t.Errorf("Walk through %s/other wanted %v, got %v", name, wantErr, err) + } + + // Do a successful walk. + _, f, err := root.Walk([]string{name}) + if err != nil { + t.Errorf("Walk to %s wanted nil, got %v", name, err) + } + defer f.Close() + local := h.Pop(f) + + // Check that the file matches. + _, localMask, localAttr, localErr := local.GetAttr(p9.AttrMaskAll()) + if _, mask, attr, err := f.GetAttr(p9.AttrMaskAll()); mask != localMask || attr != localAttr || err != localErr { + t.Errorf("GetAttr got (%v, %v, %v), wanted (%v, %v, %v)", + mask, attr, err, localMask, localAttr, localErr) + } + + // Ensure we can't walk backwards. + if _, _, err := f.Walk([]string{"."}); err != syscall.EINVAL { + t.Errorf("Walk through %s/. wanted EINVAL, got %v", name, err) + } + if _, _, err := f.Walk([]string{".."}); err != syscall.EINVAL { + t.Errorf("Walk through %s/.. wanted EINVAL, got %v", name, err) + } + } +} + +// fileGenerator is a function to generate files via walk or create. +// +// Examples are: +// - walkHelper +// - walkAndOpenHelper +// - createHelper +type fileGenerator func(*Harness, string, p9.File) (*Mock, *Mock, p9.File) + +// walkHelper walks to the given file. +// +// The backends of the parent and walked file are returned, as well as the +// walked client file. +func walkHelper(h *Harness, name string, dir p9.File) (parentBackend *Mock, walkedBackend *Mock, walked p9.File) { + _, parent, err := dir.Walk(nil) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + defer parent.Close() + parentBackend = h.Pop(parent) + + _, walked, err = parent.Walk([]string{name}) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + walkedBackend = h.Pop(walked) + + return parentBackend, walkedBackend, walked +} + +// walkAndOpenHelper additionally opens the walked file, if possible. +func walkAndOpenHelper(h *Harness, name string, dir p9.File) (*Mock, *Mock, p9.File) { + parentBackend, walkedBackend, walked := walkHelper(h, name, dir) + if p9.CanOpen(walkedBackend.Attr.Mode) { + // Open for all file types that we can. We stick to a read-only + // open here because directories may not be opened otherwise. + walkedBackend.EXPECT().Open(p9.ReadOnly).Times(1) + if _, _, _, err := walked.Open(p9.ReadOnly); err != nil { + h.t.Errorf("got open err %v, want nil", err) + } + } else { + // ... or assert an error for others. + if _, _, _, err := walked.Open(p9.ReadOnly); err != syscall.EINVAL { + h.t.Errorf("got open err %v, want EINVAL", err) + } + } + return parentBackend, walkedBackend, walked +} + +// createHelper creates the given file and returns the parent directory, +// created file and client file, which must be closed when done. +func createHelper(h *Harness, name string, dir p9.File) (*Mock, *Mock, p9.File) { + // Clone the directory first, since Create replaces the existing file. + // We change the type after calling create. + _, dirThenFile, err := dir.Walk(nil) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + + // Create a new server-side file. On the server-side, the a new file is + // returned from a create call. The client will reuse the same file, + // but we still expect the normal chain of closes. This complicates + // things a bit because the "parent" will always chain to the cloned + // dir above. + dirBackend := h.Pop(dirThenFile) // New backend directory. + newFile := h.NewFile()(dirBackend) // New file with backend parent. + dirBackend.EXPECT().Create(name, gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, newFile, newFile.QID, uint32(0), nil) + + // Create via the client. + _, dirThenFile, _, _, err = dirThenFile.Create(name, p9.ReadOnly, 0, 0, 0) + if err != nil { + h.t.Fatalf("got create err %v, want nil", err) + } + + // Ensure subsequent walks succeed. + dirBackend.AddChild(name, h.NewFile()) + return dirBackend, newFile, dirThenFile +} + +// deprecatedRemover allows us to access the deprecated Remove operation within +// the p9.File client object. +type deprecatedRemover interface { + Remove() error +} + +// checkDeleted asserts that relevant methods fail for an unlinked file. +// +// This function will close the file at the end. +func checkDeleted(h *Harness, file p9.File) { + defer file.Close() // See doc. + + if _, _, _, err := file.Open(p9.ReadOnly); err != syscall.EINVAL { + h.t.Errorf("open while deleted, got %v, want EINVAL", err) + } + if _, _, _, _, err := file.Create("created", p9.ReadOnly, 0, 0, 0); err != syscall.EINVAL { + h.t.Errorf("create while deleted, got %v, want EINVAL", err) + } + if _, err := file.Symlink("old", "new", 0, 0); err != syscall.EINVAL { + h.t.Errorf("symlink while deleted, got %v, want EINVAL", err) + } + // N.B. This link is technically invalid, but if a call to link is + // actually made in the backend then the mock will panic. + if err := file.Link(file, "new"); err != syscall.EINVAL { + h.t.Errorf("link while deleted, got %v, want EINVAL", err) + } + if err := file.RenameAt("src", file, "dst"); err != syscall.EINVAL { + h.t.Errorf("renameAt while deleted, got %v, want EINVAL", err) + } + if err := file.UnlinkAt("file", 0); err != syscall.EINVAL { + h.t.Errorf("unlinkAt while deleted, got %v, want EINVAL", err) + } + if err := file.Rename(file, "dst"); err != syscall.EINVAL { + h.t.Errorf("rename while deleted, got %v, want EINVAL", err) + } + if _, err := file.Readlink(); err != syscall.EINVAL { + h.t.Errorf("readlink while deleted, got %v, want EINVAL", err) } - attacher := &AttachMock{ - File: fileMock, + if _, err := file.Mkdir("dir", p9.ModeDirectory, 0, 0); err != syscall.EINVAL { + h.t.Errorf("mkdir while deleted, got %v, want EINVAL", err) + } + if _, err := file.Mknod("dir", p9.ModeDirectory, 0, 0, 0, 0); err != syscall.EINVAL { + h.t.Errorf("mknod while deleted, got %v, want EINVAL", err) + } + if _, err := file.Readdir(0, 1); err != syscall.EINVAL { + h.t.Errorf("readdir while deleted, got %v, want EINVAL", err) + } + if _, err := file.Connect(p9.ConnectFlags(0)); err != syscall.EINVAL { + h.t.Errorf("connect while deleted, got %v, want EINVAL", err) } - // Make socket pair. - serverSocket, clientSocket, err := unet.SocketPair(false) + // The remove method is technically deprecated, but we want to ensure + // that it still checks for deleted appropriately. We must first clone + // the file because remove is equivalent to close. + _, newFile, err := file.Walk(nil) + if err == syscall.EBUSY { + // We can't walk from here because this reference is open + // aleady. Okay, we will also have unopened cases through + // TestUnlink, just skip the remove operation for now. + return + } else if err != nil { + h.t.Fatalf("clone failed, got %v, want nil", err) + } + if err := newFile.(deprecatedRemover).Remove(); err != syscall.EINVAL { + h.t.Errorf("remove while deleted, got %v, want EINVAL", err) + } +} + +// deleter is a function to remove a file. +type deleter func(parent p9.File, name string) error + +// unlinkAt is a deleter. +func unlinkAt(parent p9.File, name string) error { + // Call unlink. Note that a filesystem may normally impose additional + // constaints on unlinkat success, such as ensuring that a directory is + // empty, requiring AT_REMOVEDIR in flags to remove a directory, etc. + // None of that is required internally (entire trees can be marked + // deleted when this operation succeeds), so the mock will succeed. + return parent.UnlinkAt(name, 0) +} + +// remove is a deleter. +func remove(parent p9.File, name string) error { + // See notes above re: remove. + _, newFile, err := parent.Walk([]string{name}) if err != nil { - t.Fatalf("socketpair got err %v wanted nil", err) + // Should not be expected. + return err + } + + // Do the actual remove. + if err := newFile.(deprecatedRemover).Remove(); err != nil { + return err + } + + // Ensure that the remove closed the file. + if err := newFile.(deprecatedRemover).Remove(); err != syscall.EBADF { + return syscall.EBADF // Propagate this code. } - defer clientSocket.Close() - server := p9.NewServer(attacher) - go server.Handle(serverSocket) - client, err := p9.NewClient(clientSocket, 1024*1024 /* 1M message size */, p9.HighestVersionString()) + + return nil +} + +// unlinkHelper unlinks the noted path, and ensures that all relevant +// operations on that path, acquired from multiple paths, start failing. +func unlinkHelper(h *Harness, root p9.File, targetNames []string, targetGen fileGenerator, deleteFn deleter) { + // name is the file to be unlinked. + name := targetNames[len(targetNames)-1] + + // Walk to the directory containing the target. + _, parent, err := root.Walk(targetNames[:len(targetNames)-1]) if err != nil { - t.Fatalf("new client got %v, expected nil", err) + h.t.Fatalf("got walk err %v, want nil", err) } + defer parent.Close() + parentBackend := h.Pop(parent) + + // Walk to or generate the target file. + _, _, target := targetGen(h, name, parent) + defer checkDeleted(h, target) - // Attach to the mocked file. - cFile, err := client.Attach("") + // Walk to a second reference. + _, second, err := parent.Walk([]string{name}) if err != nil { - t.Fatalf("attach failed: %v", err) + h.t.Fatalf("got walk err %v, want nil", err) } + defer checkDeleted(h, second) - // Try to open the mocked file. - clientHostFile, _, _, err := cFile.Open(0) + // Walk to a third reference, from the start. + _, third, err := root.Walk(targetNames) if err != nil { - t.Fatalf("open failed: %v", err) + h.t.Fatalf("got walk err %v, want nil", err) } - var clientStat syscall.Stat_t - if err := syscall.Fstat(clientHostFile.FD(), &clientStat); err != nil { - t.Fatalf("stat failed: %v", err) + defer checkDeleted(h, third) + + // This will be translated in the backend to an unlinkat. + parentBackend.EXPECT().UnlinkAt(name, uint32(0)).Return(nil) + + // Actually perform the deletion. + if err := deleteFn(parent, name); err != nil { + h.t.Fatalf("got delete err %v, want nil", err) } +} + +func unlinkTest(t *testing.T, targetNames []string, targetGen fileGenerator) { + t.Run(fmt.Sprintf("unlinkAt(%s)", strings.Join(targetNames, "/")), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + unlinkHelper(h, root, targetNames, targetGen, unlinkAt) + }) + t.Run(fmt.Sprintf("remove(%s)", strings.Join(targetNames, "/")), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() - // Compare inode nums to make sure it's the same file. - if clientStat.Ino != osFileStat.Ino { - t.Errorf("fd donation failed") + unlinkHelper(h, root, targetNames, targetGen, remove) + }) +} + +func TestUnlink(t *testing.T) { + // Unlink all files. + for name := range newTypeMap(nil) { + unlinkTest(t, []string{name}, walkHelper) + unlinkTest(t, []string{name}, walkAndOpenHelper) + unlinkTest(t, []string{"one", name}, walkHelper) + unlinkTest(t, []string{"one", name}, walkAndOpenHelper) + unlinkTest(t, []string{"one", "two", name}, walkHelper) + unlinkTest(t, []string{"one", "two", name}, walkAndOpenHelper) } + + // Unlink a directory. + unlinkTest(t, []string{"one"}, walkHelper) + unlinkTest(t, []string{"one"}, walkAndOpenHelper) + unlinkTest(t, []string{"one", "two"}, walkHelper) + unlinkTest(t, []string{"one", "two"}, walkAndOpenHelper) + + // Unlink created files. + unlinkTest(t, []string{"created"}, createHelper) + unlinkTest(t, []string{"one", "created"}, createHelper) + unlinkTest(t, []string{"one", "two", "created"}, createHelper) } -// TestClient is a megatest. -// -// This allows us to probe various edge cases, while changing the state of the -// underlying server in expected ways. The test slowly builds server state and -// is documented inline. -// -// We wind up with the following, after probing edge cases: -// -// FID 1: ServerFile (sf). -// FID 2: Directory (d). -// FID 3: File (f). -// FID 4: Symlink (s). -// -// Although you should use the FID method on the individual files. -func TestClient(t *testing.T) { +func TestUnlinkAtInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + if err := root.UnlinkAt(invalidName, 0); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +// expectRenamed asserts an ordered sequence of rename calls, based on all the +// elements in elements being the source, and the first element therein +// changing to dstName, parented at dstParent. +func expectRenamed(file *Mock, elements []string, dstParent *Mock, dstName string) *gomock.Call { + if len(elements) > 0 { + // Recurse to the parent, if necessary. + call := expectRenamed(file.parent, elements[:len(elements)-1], dstParent, dstName) + + // Recursive case: this element is unchanged, but should have + // it's hook called after the parent. + return file.EXPECT().Renamed(file.parent, elements[len(elements)-1]).Do(func(p p9.File, _ string) { + file.parent = p.(*Mock) + }).After(call) + } + + // Base case: this is the changed element. + return file.EXPECT().Renamed(dstParent, dstName).Do(func(p p9.File, name string) { + file.parent = p.(*Mock) + }) +} + +// renamer is a rename function. +type renamer func(h *Harness, srcParent, dstParent p9.File, origName, newName string, selfRename bool) error + +// renameAt is a renamer. +func renameAt(_ *Harness, srcParent, dstParent p9.File, srcName, dstName string, selfRename bool) error { + return srcParent.RenameAt(srcName, dstParent, dstName) +} + +// rename is a renamer. +func rename(h *Harness, srcParent, dstParent p9.File, srcName, dstName string, selfRename bool) error { + _, f, err := srcParent.Walk([]string{srcName}) + if err != nil { + return err + } + defer f.Close() + if !selfRename { + backend := h.Pop(f) + backend.EXPECT().Renamed(gomock.Any(), dstName).Do(func(p p9.File, name string) { + backend.parent = p.(*Mock) // Required for close ordering. + }) + } + return f.Rename(dstParent, dstName) +} + +// renameHelper executes a rename, and asserts that all relevant elements +// receive expected notifications. If overwriting a file, this includes +// ensuring that the target has been appropriately marked as unlinked. +func renameHelper(h *Harness, root p9.File, srcNames []string, dstNames []string, target fileGenerator, renameFn renamer) { + // Walk to the directory containing the target. + srcQID, targetParent, err := root.Walk(srcNames[:len(srcNames)-1]) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + defer targetParent.Close() + targetParentBackend := h.Pop(targetParent) + + // Walk to or generate the target file. + _, targetBackend, src := target(h, srcNames[len(srcNames)-1], targetParent) + defer src.Close() + + // Walk to a second reference. + _, second, err := targetParent.Walk([]string{srcNames[len(srcNames)-1]}) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + defer second.Close() + secondBackend := h.Pop(second) + + // Walk to a third reference, from the start. + _, third, err := root.Walk(srcNames) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + defer third.Close() + thirdBackend := h.Pop(third) + + // Find the common suffix to identify the rename parent. var ( - // Sentinel error. - sentinelErr = syscall.Errno(4383) - - // Backend mocks. - a = &AttachMock{} - sf = &FileMock{} - d = &FileMock{} - f = &FileMock{} - s = &FileMock{} - - // Client Files for the above. - sfFile p9.File + renameDestPath []string + renameSrcPath []string + selfRename bool ) + for i := 1; i <= len(srcNames) && i <= len(dstNames); i++ { + if srcNames[len(srcNames)-i] != dstNames[len(dstNames)-i] { + // Take the full prefix of dstNames up until this + // point, including the first mismatched name. The + // first mismatch must be the renamed entry. + renameDestPath = dstNames[:len(dstNames)-i+1] + renameSrcPath = srcNames[:len(srcNames)-i+1] - testSteps := []struct { - name string - fn func(*p9.Client) error - want error - }{ - { - name: "bad-attach", - want: sentinelErr, - fn: func(c *p9.Client) error { - a.File = nil - a.Err = sentinelErr - _, err := c.Attach("") - return err - }, + // Does the renameDestPath fully contain the + // renameSrcPath here? If yes, then this is a mismatch. + // We can't rename the src to some subpath of itself. + if len(renameDestPath) > len(renameSrcPath) && + reflect.DeepEqual(renameDestPath[:len(renameSrcPath)], renameSrcPath) { + renameDestPath = nil + renameSrcPath = nil + continue + } + break + } + } + if len(renameSrcPath) == 0 || len(renameDestPath) == 0 { + // This must be a rename to self, or a tricky look-alike. This + // happens iff we fail to find a suitable divergence in the two + // paths. It's a true self move if the path length is the same. + renameDestPath = dstNames + renameSrcPath = srcNames + selfRename = len(srcNames) == len(dstNames) + } + + // Walk to the source parent. + _, srcParent, err := root.Walk(renameSrcPath[:len(renameSrcPath)-1]) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + defer srcParent.Close() + srcParentBackend := h.Pop(srcParent) + + // Walk to the destination parent. + _, dstParent, err := root.Walk(renameDestPath[:len(renameDestPath)-1]) + if err != nil { + h.t.Fatalf("got walk err %v, want nil", err) + } + defer dstParent.Close() + dstParentBackend := h.Pop(dstParent) + + // expectedErr is the result of the rename operation. + var expectedErr error + + // Walk to the target file, if one exists. + dstQID, dst, err := root.Walk(renameDestPath) + if err == nil { + if !selfRename && srcQID[0].Type == dstQID[0].Type { + // If there is a destination file, and is it of the + // same type as the source file, then we expect the + // rename to succeed. We expect the destination file to + // be deleted, so we run a deletion test on it in this + // case. + defer checkDeleted(h, dst) + } else { + if !selfRename { + // If the type is different than the + // destination, then we expect the rename to + // fail. We expect ensure that this is + // returned. + expectedErr = syscall.EINVAL + } else { + // This is the file being renamed to itself. + // This is technically allowed and a no-op, but + // all the triggers will fire. + } + dst.Close() + } + } + dstName := renameDestPath[len(renameDestPath)-1] // Renamed element. + srcName := renameSrcPath[len(renameSrcPath)-1] // Renamed element. + if expectedErr == nil && !selfRename { + // Expect all to be renamed appropriately. Note that if this is + // a final file being renamed, then we expect the file to be + // called with the new parent. If not, then we expect the + // rename hook to be called, but the parent will remain + // unchanged. + elements := srcNames[len(renameSrcPath):] + expectRenamed(targetBackend, elements, dstParentBackend, dstName) + expectRenamed(secondBackend, elements, dstParentBackend, dstName) + expectRenamed(thirdBackend, elements, dstParentBackend, dstName) + + // The target parent has also been opened, and may be moved + // directly or indirectly. + if len(elements) > 1 { + expectRenamed(targetParentBackend, elements[:len(elements)-1], dstParentBackend, dstName) + } + } + + // Expect the rename if it's not the same file. Note that like unlink, + // renames are always translated to the at variant in the backend. + if !selfRename { + srcParentBackend.EXPECT().RenameAt(srcName, dstParentBackend, dstName).Return(expectedErr) + } + + // Perform the actual rename; everything has been lined up. + if err := renameFn(h, srcParent, dstParent, srcName, dstName, selfRename); err != expectedErr { + h.t.Fatalf("got rename err %v, want %v", err, expectedErr) + } +} + +func renameTest(t *testing.T, srcNames []string, dstNames []string, target fileGenerator) { + t.Run(fmt.Sprintf("renameAt(%s->%s)", strings.Join(srcNames, "/"), strings.Join(dstNames, "/")), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + renameHelper(h, root, srcNames, dstNames, target, renameAt) + }) + t.Run(fmt.Sprintf("rename(%s->%s)", strings.Join(srcNames, "/"), strings.Join(dstNames, "/")), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + renameHelper(h, root, srcNames, dstNames, target, rename) + }) +} + +func TestRename(t *testing.T) { + // In-directory rename, simple case. + for name := range newTypeMap(nil) { + // Within the root. + renameTest(t, []string{name}, []string{"renamed"}, walkHelper) + renameTest(t, []string{name}, []string{"renamed"}, walkAndOpenHelper) + + // Within a subdirectory. + renameTest(t, []string{"one", name}, []string{"one", "renamed"}, walkHelper) + renameTest(t, []string{"one", name}, []string{"one", "renamed"}, walkAndOpenHelper) + } + + // ... with created files. + renameTest(t, []string{"created"}, []string{"renamed"}, createHelper) + renameTest(t, []string{"one", "created"}, []string{"one", "renamed"}, createHelper) + + // Across directories. + for name := range newTypeMap(nil) { + // Down one level. + renameTest(t, []string{"one", name}, []string{"one", "two", "renamed"}, walkHelper) + renameTest(t, []string{"one", name}, []string{"one", "two", "renamed"}, walkAndOpenHelper) + + // Up one level. + renameTest(t, []string{"one", "two", name}, []string{"one", "renamed"}, walkHelper) + renameTest(t, []string{"one", "two", name}, []string{"one", "renamed"}, walkAndOpenHelper) + + // Across at the same level. + renameTest(t, []string{"one", name}, []string{"three", "renamed"}, walkHelper) + renameTest(t, []string{"one", name}, []string{"three", "renamed"}, walkAndOpenHelper) + } + + // ... with created files. + renameTest(t, []string{"one", "created"}, []string{"one", "two", "renamed"}, createHelper) + renameTest(t, []string{"one", "two", "created"}, []string{"one", "renamed"}, createHelper) + renameTest(t, []string{"one", "created"}, []string{"three", "renamed"}, createHelper) + + // Renaming parents. + for name := range newTypeMap(nil) { + // Rename a parent. + renameTest(t, []string{"one", name}, []string{"renamed", name}, walkHelper) + renameTest(t, []string{"one", name}, []string{"renamed", name}, walkAndOpenHelper) + + // Rename a super parent. + renameTest(t, []string{"one", "two", name}, []string{"renamed", name}, walkHelper) + renameTest(t, []string{"one", "two", name}, []string{"renamed", name}, walkAndOpenHelper) + } + + // ... with created files. + renameTest(t, []string{"one", "created"}, []string{"renamed", "created"}, createHelper) + renameTest(t, []string{"one", "two", "created"}, []string{"renamed", "created"}, createHelper) + + // Over existing files, including itself. + for name := range newTypeMap(nil) { + for other := range newTypeMap(nil) { + // Overwrite the noted file (may be itself). + renameTest(t, []string{"one", name}, []string{"one", other}, walkHelper) + renameTest(t, []string{"one", name}, []string{"one", other}, walkAndOpenHelper) + + // Overwrite other files in another directory. + renameTest(t, []string{"one", name}, []string{"one", "two", other}, walkHelper) + renameTest(t, []string{"one", name}, []string{"one", "two", other}, walkAndOpenHelper) + } + + // Overwrite by moving the parent. + renameTest(t, []string{"three", name}, []string{"one", name}, walkHelper) + renameTest(t, []string{"three", name}, []string{"one", name}, walkAndOpenHelper) + + // Create over the types. + renameTest(t, []string{"one", "created"}, []string{"one", name}, createHelper) + renameTest(t, []string{"one", "created"}, []string{"one", "two", name}, createHelper) + renameTest(t, []string{"three", "created"}, []string{"one", name}, createHelper) + } +} + +func TestRenameInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + if err := root.Rename(root, invalidName); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +func TestRenameAtInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + if err := root.RenameAt(invalidName, root, "okay"); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + if err := root.RenameAt("okay", root, invalidName); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +func TestReadlink(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Walk to the file normally. + _, f, err := root.Walk([]string{name}) + if err != nil { + t.Fatalf("walk failed: got %v, wanted nil", err) + } + defer f.Close() + backend := h.Pop(f) + + const symlinkTarget = "symlink-target" + + if backend.Attr.Mode.IsSymlink() { + // This should only go through on symlinks. + backend.EXPECT().Readlink().Return(symlinkTarget, nil) + } + + // Attempt a Readlink operation. + target, err := f.Readlink() + if err != nil && err != syscall.EINVAL { + t.Errorf("readlink got %v, wanted EINVAL", err) + } else if err == nil && target != symlinkTarget { + t.Errorf("readlink got %v, wanted %v", target, symlinkTarget) + } + }) + } +} + +// fdTest is a wrapper around operations that may send file descriptors. This +// asserts that the file descriptors are working as intended. +func fdTest(t *testing.T, sendFn func(*fd.FD) *fd.FD) { + // Create a pipe that we can read from. + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("unable to create pipe: %v", err) + } + defer r.Close() + defer w.Close() + + // Attempt to send the write end. + wFD, err := fd.NewFromFile(w) + if err != nil { + t.Fatalf("unable to convert file: %v", err) + } + defer wFD.Close() // This is a copy. + + // Send wFD and receive newFD. + newFD := sendFn(wFD) + defer newFD.Close() + + // Attempt to write. + const message = "hello" + if _, err := newFD.Write([]byte(message)); err != nil { + t.Fatalf("write got %v, wanted nil", err) + } + + // Should see the message on our end. + buffer := []byte(message) + if _, err := io.ReadFull(r, buffer); err != nil { + t.Fatalf("read got %v, wanted nil", err) + } + if string(buffer) != message { + t.Errorf("got message %v, wanted %v", string(buffer), message) + } +} + +func TestConnect(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Walk to the file normally. + _, backend, f := walkHelper(h, name, root) + defer f.Close() + + // Catch all the non-socket cases. + if !backend.Attr.Mode.IsSocket() { + // This has been set up to fail if Connect is called. + if _, err := f.Connect(p9.ConnectFlags(0)); err != syscall.EINVAL { + t.Errorf("connect got %v, wanted EINVAL", err) + } + return + } + + // Ensure the fd exchange works. + fdTest(t, func(send *fd.FD) *fd.FD { + backend.EXPECT().Connect(p9.ConnectFlags(0)).Return(send, nil) + recv, err := backend.Connect(p9.ConnectFlags(0)) + if err != nil { + t.Fatalf("connect got %v, wanted nil", err) + } + return recv + }) + }) + } +} + +func TestReaddir(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Walk to the file normally. + _, backend, f := walkHelper(h, name, root) + defer f.Close() + + // Catch all the non-directory cases. + if !backend.Attr.Mode.IsDir() { + // This has also been set up to fail if Readdir is called. + if _, err := f.Readdir(0, 1); err != syscall.EINVAL { + t.Errorf("readdir got %v, wanted EINVAL", err) + } + return + } + + // Ensure that readdir works for directories. + if _, err := f.Readdir(0, 1); err != syscall.EINVAL { + t.Errorf("readdir got %v, wanted EINVAL", err) + } + if _, _, _, err := f.Open(p9.ReadWrite); err != syscall.EINVAL { + t.Errorf("readdir got %v, wanted EINVAL", err) + } + if _, _, _, err := f.Open(p9.WriteOnly); err != syscall.EINVAL { + t.Errorf("readdir got %v, wanted EINVAL", err) + } + backend.EXPECT().Open(p9.ReadOnly).Times(1) + if _, _, _, err := f.Open(p9.ReadOnly); err != nil { + t.Errorf("readdir got %v, wanted nil", err) + } + backend.EXPECT().Readdir(uint64(0), uint32(1)).Times(1) + if _, err := f.Readdir(0, 1); err != nil { + t.Errorf("readdir got %v, wanted nil", err) + } + }) + } +} + +func TestOpen(t *testing.T) { + type openTest struct { + name string + mode p9.OpenFlags + err error + match func(p9.FileMode) bool + } + + cases := []openTest{ + { + name: "invalid", + mode: ^p9.OpenFlagsModeMask, + err: syscall.EINVAL, + match: func(p9.FileMode) bool { return true }, }, { - name: "attach", - fn: func(c *p9.Client) error { - a.Called = false - a.File = sf - a.Err = nil - // The attached root must have a valid mode. - sf.GetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory} - sf.GetAttrMock.Valid = p9.AttrMask{Mode: true} - var err error - sfFile, err = c.Attach("") - if !a.Called { - t.Errorf("Attach never Called?") - } - return err - }, + name: "not-openable-read-only", + mode: p9.ReadOnly, + err: syscall.EINVAL, + match: func(mode p9.FileMode) bool { return !p9.CanOpen(mode) }, }, { - name: "bad-walk", - want: sentinelErr, - fn: func(c *p9.Client) error { - // Walk only called when WalkGetAttr not available. - sf.WalkGetAttrMock.Err = syscall.ENOSYS - sf.WalkMock.File = d - sf.WalkMock.Err = sentinelErr - _, _, err := sfFile.Walk([]string{"foo", "bar"}) - return err - }, + name: "not-openable-write-only", + mode: p9.WriteOnly, + err: syscall.EINVAL, + match: func(mode p9.FileMode) bool { return !p9.CanOpen(mode) }, }, { - name: "walk-to-dir", - fn: func(c *p9.Client) error { - // Walk only called when WalkGetAttr not available. - sf.WalkGetAttrMock.Err = syscall.ENOSYS - sf.WalkMock.Called = false - sf.WalkMock.Names = nil - sf.WalkMock.File = d - sf.WalkMock.Err = nil - sf.WalkMock.QIDs = []p9.QID{{Type: 1}} - // All intermediate values must be directories. - d.WalkGetAttrMock.Err = syscall.ENOSYS - d.WalkMock.Called = false - d.WalkMock.Names = nil - d.WalkMock.File = d // Walk to self. - d.WalkMock.Err = nil - d.WalkMock.QIDs = []p9.QID{{Type: 1}} - d.GetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory} - d.GetAttrMock.Valid = p9.AttrMask{Mode: true} - var qids []p9.QID - var err error - qids, _, err = sfFile.Walk([]string{"foo", "bar"}) - if !sf.WalkMock.Called { - t.Errorf("Walk never Called?") - } - if !d.GetAttrMock.Called { - t.Errorf("GetAttr never Called?") - } - if !reflect.DeepEqual(sf.WalkMock.Names, []string{"foo"}) { - t.Errorf("got names %v wanted []{foo}", sf.WalkMock.Names) - } - if !reflect.DeepEqual(d.WalkMock.Names, []string{"bar"}) { - t.Errorf("got names %v wanted []{bar}", d.WalkMock.Names) - } - if len(qids) != 2 || qids[len(qids)-1].Type != 1 { - t.Errorf("got qids %v wanted []{..., {Type: 1}}", qids) - } - return err - }, + name: "not-openable-read-write", + mode: p9.ReadWrite, + err: syscall.EINVAL, + match: func(mode p9.FileMode) bool { return !p9.CanOpen(mode) }, }, { - name: "walkgetattr-to-dir", - fn: func(c *p9.Client) error { - sf.WalkGetAttrMock.Called = false - sf.WalkGetAttrMock.Names = nil - sf.WalkGetAttrMock.File = d - sf.WalkGetAttrMock.Err = nil - sf.WalkGetAttrMock.QIDs = []p9.QID{{Type: 1}} - sf.WalkGetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory, UID: 1} - sf.WalkGetAttrMock.Valid = p9.AttrMask{Mode: true} - // See above. - d.WalkGetAttrMock.Called = false - d.WalkGetAttrMock.Names = nil - d.WalkGetAttrMock.File = d // Walk to self. - d.WalkGetAttrMock.Err = nil - d.WalkGetAttrMock.QIDs = []p9.QID{{Type: 1}} - d.WalkGetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory, UID: 1} - d.WalkGetAttrMock.Valid = p9.AttrMask{Mode: true} - var qids []p9.QID - var err error - var mask p9.AttrMask - var attr p9.Attr - qids, _, mask, attr, err = sfFile.WalkGetAttr([]string{"foo", "bar"}) - if !sf.WalkGetAttrMock.Called { - t.Errorf("Walk never Called?") - } - if !reflect.DeepEqual(sf.WalkGetAttrMock.Names, []string{"foo"}) { - t.Errorf("got names %v wanted []{foo}", sf.WalkGetAttrMock.Names) - } - if !reflect.DeepEqual(d.WalkGetAttrMock.Names, []string{"bar"}) { - t.Errorf("got names %v wanted []{bar}", d.WalkGetAttrMock.Names) - } - if len(qids) != 2 || qids[len(qids)-1].Type != 1 { - t.Errorf("got qids %v wanted []{..., {Type: 1}}", qids) - } - if !reflect.DeepEqual(attr, sf.WalkGetAttrMock.Attr) { - t.Errorf("got attrs %s wanted %s", attr, sf.WalkGetAttrMock.Attr) - } - if !reflect.DeepEqual(mask, sf.WalkGetAttrMock.Valid) { - t.Errorf("got mask %s wanted %s", mask, sf.WalkGetAttrMock.Valid) - } - return err - }, + name: "directory-read-only", + mode: p9.ReadOnly, + err: nil, + match: func(mode p9.FileMode) bool { return mode.IsDir() }, }, { - name: "walk-to-file", - fn: func(c *p9.Client) error { - // Basic sanity check is done in walk-to-dir. - // - // Here we just create basic file FIDs to use. - sf.WalkMock.File = f - sf.WalkMock.Err = nil - var err error - _, _, err = sfFile.Walk(nil) - return err - }, + name: "directory-read-write", + mode: p9.ReadWrite, + err: syscall.EINVAL, + match: func(mode p9.FileMode) bool { return mode.IsDir() }, }, { - name: "walk-to-symlink", - fn: func(c *p9.Client) error { - // See note in walk-to-file. - sf.WalkMock.File = s - sf.WalkMock.Err = nil - var err error - _, _, err = sfFile.Walk(nil) - return err - }, + name: "directory-write-only", + mode: p9.WriteOnly, + err: syscall.EINVAL, + match: func(mode p9.FileMode) bool { return mode.IsDir() }, }, { - name: "bad-statfs", - want: sentinelErr, - fn: func(c *p9.Client) error { - sf.StatFSMock.Err = sentinelErr - _, err := sfFile.StatFS() - return err - }, + name: "read-only", + mode: p9.ReadOnly, + err: nil, + match: func(mode p9.FileMode) bool { return p9.CanOpen(mode) }, + }, + { + name: "write-only", + mode: p9.WriteOnly, + err: nil, + match: func(mode p9.FileMode) bool { return p9.CanOpen(mode) && !mode.IsDir() }, }, { - name: "statfs", - fn: func(c *p9.Client) error { - sf.StatFSMock.Called = false - sf.StatFSMock.Stat = p9.FSStat{Type: 1} - sf.StatFSMock.Err = nil - stat, err := sfFile.StatFS() - if !sf.StatFSMock.Called { - t.Errorf("StatfS never Called?") + name: "read-write", + mode: p9.ReadWrite, + err: nil, + match: func(mode p9.FileMode) bool { return p9.CanOpen(mode) && !mode.IsDir() }, + }, + } + + // Open(mode OpenFlags) (*fd.FD, QID, uint32, error) + // - only works on Regular, NamedPipe, BLockDevice, CharacterDevice + // - returning a file works as expected + for name := range newTypeMap(nil) { + for _, tc := range cases { + t.Run(fmt.Sprintf("%s-%s", tc.name, name), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Walk to the file normally. + _, backend, f := walkHelper(h, name, root) + defer f.Close() + + // Does this match the case? + if !tc.match(backend.Attr.Mode) { + t.SkipNow() } - if stat.Type != 1 { - t.Errorf("got stat %v wanted {Type: 1}", stat) + + // Ensure open-required operations fail. + if _, err := f.ReadAt([]byte("hello"), 0); err != syscall.EINVAL { + t.Errorf("readAt got %v, wanted EINVAL", err) } - return err + if _, err := f.WriteAt(make([]byte, 6), 0); err != syscall.EINVAL { + t.Errorf("writeAt got %v, wanted EINVAL", err) + } + if err := f.FSync(); err != syscall.EINVAL { + t.Errorf("fsync got %v, wanted EINVAL", err) + } + if _, err := f.Readdir(0, 1); err != syscall.EINVAL { + t.Errorf("readdir got %v, wanted EINVAL", err) + } + + // Attempt the given open. + if tc.err != nil { + // We expect an error, just test and return. + if _, _, _, err := f.Open(tc.mode); err != tc.err { + t.Fatalf("open with mode %v got %v, want %v", tc.mode, err, tc.err) + } + return + } + + // Run an FD test, since we expect success. + fdTest(t, func(send *fd.FD) *fd.FD { + backend.EXPECT().Open(tc.mode).Return(send, p9.QID{}, uint32(0), nil).Times(1) + recv, _, _, err := f.Open(tc.mode) + if err != tc.err { + t.Fatalf("open with mode %v got %v, want %v", tc.mode, err, tc.err) + } + return recv + }) + + // If the open was successful, attempt another one. + if _, _, _, err := f.Open(tc.mode); err != syscall.EINVAL { + t.Errorf("second open with mode %v got %v, want EINVAL", tc.mode, err) + } + + // Ensure that all illegal operations fail. + if _, _, err := f.Walk(nil); err != syscall.EINVAL && err != syscall.EBUSY { + t.Errorf("walk got %v, wanted EINVAL or EBUSY", err) + } + if _, _, _, _, err := f.WalkGetAttr(nil); err != syscall.EINVAL && err != syscall.EBUSY { + t.Errorf("walkgetattr got %v, wanted EINVAL or EBUSY", err) + } + }) + } + } +} + +func TestClose(t *testing.T) { + type closeTest struct { + name string + closeFn func(backend *Mock, f p9.File) + } + + cases := []closeTest{ + { + name: "close", + closeFn: func(_ *Mock, f p9.File) { + f.Close() + }, + }, + { + name: "remove", + closeFn: func(backend *Mock, f p9.File) { + // Allow the rename call in the parent, automatically translated. + backend.parent.EXPECT().UnlinkAt(gomock.Any(), gomock.Any()).Times(1) + f.(deprecatedRemover).Remove() }, }, } - // First, create a new server and connection. - serverSocket, clientSocket, err := unet.SocketPair(false) - if err != nil { - t.Fatalf("socketpair got err %v wanted nil", err) + for name := range newTypeMap(nil) { + for _, tc := range cases { + t.Run(fmt.Sprintf("%s(%s)", tc.name, name), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Walk to the file normally. + _, backend, f := walkHelper(h, name, root) + + // Close via the prescribed method. + tc.closeFn(backend, f) + + // Everything should fail with EBADF. + if _, _, err := f.Walk(nil); err != syscall.EBADF { + t.Errorf("walk got %v, wanted EBADF", err) + } + if _, err := f.StatFS(); err != syscall.EBADF { + t.Errorf("statfs got %v, wanted EBADF", err) + } + if _, _, _, err := f.GetAttr(p9.AttrMaskAll()); err != syscall.EBADF { + t.Errorf("getattr got %v, wanted EBADF", err) + } + if err := f.SetAttr(p9.SetAttrMask{}, p9.SetAttr{}); err != syscall.EBADF { + t.Errorf("setattrk got %v, wanted EBADF", err) + } + if err := f.Rename(root, "new-name"); err != syscall.EBADF { + t.Errorf("rename got %v, wanted EBADF", err) + } + if err := f.Close(); err != syscall.EBADF { + t.Errorf("close got %v, wanted EBADF", err) + } + if _, _, _, err := f.Open(p9.ReadOnly); err != syscall.EBADF { + t.Errorf("open got %v, wanted EBADF", err) + } + if _, err := f.ReadAt([]byte("hello"), 0); err != syscall.EBADF { + t.Errorf("readAt got %v, wanted EBADF", err) + } + if _, err := f.WriteAt(make([]byte, 6), 0); err != syscall.EBADF { + t.Errorf("writeAt got %v, wanted EBADF", err) + } + if err := f.FSync(); err != syscall.EBADF { + t.Errorf("fsync got %v, wanted EBADF", err) + } + if _, _, _, _, err := f.Create("new-file", p9.ReadWrite, 0, 0, 0); err != syscall.EBADF { + t.Errorf("create got %v, wanted EBADF", err) + } + if _, err := f.Mkdir("new-directory", 0, 0, 0); err != syscall.EBADF { + t.Errorf("mkdir got %v, wanted EBADF", err) + } + if _, err := f.Symlink("old-name", "new-name", 0, 0); err != syscall.EBADF { + t.Errorf("symlink got %v, wanted EBADF", err) + } + if err := f.Link(root, "new-name"); err != syscall.EBADF { + t.Errorf("link got %v, wanted EBADF", err) + } + if _, err := f.Mknod("new-block-device", 0, 0, 0, 0, 0); err != syscall.EBADF { + t.Errorf("mknod got %v, wanted EBADF", err) + } + if err := f.RenameAt("old-name", root, "new-name"); err != syscall.EBADF { + t.Errorf("renameAt got %v, wanted EBADF", err) + } + if err := f.UnlinkAt("name", 0); err != syscall.EBADF { + t.Errorf("unlinkAt got %v, wanted EBADF", err) + } + if _, err := f.Readdir(0, 1); err != syscall.EBADF { + t.Errorf("readdir got %v, wanted EBADF", err) + } + if _, err := f.Readlink(); err != syscall.EBADF { + t.Errorf("readlink got %v, wanted EBADF", err) + } + if err := f.Flush(); err != syscall.EBADF { + t.Errorf("flush got %v, wanted EBADF", err) + } + if _, _, _, _, err := f.WalkGetAttr(nil); err != syscall.EBADF { + t.Errorf("walkgetattr got %v, wanted EBADF", err) + } + if _, err := f.Connect(p9.ConnectFlags(0)); err != syscall.EBADF { + t.Errorf("connect got %v, wanted EBADF", err) + } + }) + } } - defer clientSocket.Close() - server := p9.NewServer(a) - go server.Handle(serverSocket) - client, err := p9.NewClient(clientSocket, 1024*1024 /* 1M message size */, p9.HighestVersionString()) - if err != nil { - t.Fatalf("new client got err %v, wanted nil", err) +} + +// onlyWorksOnOpenThings is a helper test method for operations that should +// only work on files that have been explicitly opened. +func onlyWorksOnOpenThings(h *Harness, t *testing.T, name string, root p9.File, mode p9.OpenFlags, expectedErr error, fn func(backend *Mock, f p9.File, shouldSucceed bool) error) { + // Walk to the file normally. + _, backend, f := walkHelper(h, name, root) + defer f.Close() + + // Does it work before opening? + if err := fn(backend, f, false); err != syscall.EINVAL { + t.Errorf("operation got %v, wanted EINVAL", err) + } + + // Is this openable? + if !p9.CanOpen(backend.Attr.Mode) { + return // Nothing to do. + } + + // If this is a directory, we can't handle writing. + if backend.Attr.Mode.IsDir() && (mode == p9.ReadWrite || mode == p9.WriteOnly) { + return // Skip. + } + + // Open the file. + backend.EXPECT().Open(mode) + if _, _, _, err := f.Open(mode); err != nil { + t.Fatalf("open got %v, wanted nil", err) + } + + // Attempt the operation. + if err := fn(backend, f, expectedErr == nil); err != expectedErr { + t.Fatalf("operation got %v, wanted %v", err, expectedErr) + } +} + +func TestRead(t *testing.T) { + type readTest struct { + name string + mode p9.OpenFlags + err error } - // Now, run through each of the test steps. - for _, step := range testSteps { - err := step.fn(client) - if err != step.want { - // Don't fail, just note this one step failed. - t.Errorf("step %q got %v wanted %v", step.name, err, step.want) + cases := []readTest{ + { + name: "read-only", + mode: p9.ReadOnly, + err: nil, + }, + { + name: "read-write", + mode: p9.ReadWrite, + err: nil, + }, + { + name: "write-only", + mode: p9.WriteOnly, + err: syscall.EPERM, + }, + } + + for name := range newTypeMap(nil) { + for _, tc := range cases { + t.Run(fmt.Sprintf("%s-%s", tc.name, name), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + const message = "hello" + + onlyWorksOnOpenThings(h, t, name, root, tc.mode, tc.err, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if !shouldSucceed { + _, err := f.ReadAt([]byte(message), 0) + return err + } + + // Prepare for the call to readAt in the backend. + backend.EXPECT().ReadAt(gomock.Any(), uint64(0)).Do(func(p []byte, offset uint64) { + copy(p, message) + }).Return(len(message), nil) + + // Make the client call. + p := make([]byte, 2*len(message)) // Double size. + n, err := f.ReadAt(p, 0) + + // Sanity check result. + if err != nil { + return err + } + if n != len(message) { + t.Fatalf("message length incorrect, got %d, want %d", n, len(message)) + } + if !bytes.Equal(p[:n], []byte(message)) { + t.Fatalf("message incorrect, got %v, want %v", p, []byte(message)) + } + return nil // Success. + }) + }) } } } -func BenchmarkClient(b *testing.B) { - // Backend mock. - a := &AttachMock{ - File: &FileMock{ - ReadAtMock: ReadAtMock{N: 1}, +func TestWrite(t *testing.T) { + type writeTest struct { + name string + mode p9.OpenFlags + err error + } + + cases := []writeTest{ + { + name: "read-only", + mode: p9.ReadOnly, + err: syscall.EPERM, + }, + { + name: "read-write", + mode: p9.ReadWrite, + err: nil, + }, + { + name: "write-only", + mode: p9.WriteOnly, + err: nil, }, } - // First, create a new server and connection. - serverSocket, clientSocket, err := unet.SocketPair(false) - if err != nil { - b.Fatalf("socketpair got err %v wanted nil", err) + for name := range newTypeMap(nil) { + for _, tc := range cases { + t.Run(fmt.Sprintf("%s-%s", tc.name, name), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + const message = "hello" + + onlyWorksOnOpenThings(h, t, name, root, tc.mode, tc.err, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if !shouldSucceed { + _, err := f.WriteAt([]byte(message), 0) + return err + } + + // Prepare for the call to readAt in the backend. + var output []byte // Saved by Do below. + backend.EXPECT().WriteAt(gomock.Any(), uint64(0)).Do(func(p []byte, offset uint64) { + output = p + }).Return(len(message), nil) + + // Make the client call. + n, err := f.WriteAt([]byte(message), 0) + + // Sanity check result. + if err != nil { + return err + } + if n != len(message) { + t.Fatalf("message length incorrect, got %d, want %d", n, len(message)) + } + if !bytes.Equal(output, []byte(message)) { + t.Fatalf("message incorrect, got %v, want %v", output, []byte(message)) + } + return nil // Success. + }) + }) + } } - defer clientSocket.Close() - server := p9.NewServer(a) - go server.Handle(serverSocket) - client, err := p9.NewClient(clientSocket, 1024*1024 /* 1M message size */, p9.HighestVersionString()) - if err != nil { - b.Fatalf("new client got %v, expected nil", err) +} + +func TestFSync(t *testing.T) { + for name := range newTypeMap(nil) { + for _, mode := range []p9.OpenFlags{p9.ReadOnly, p9.WriteOnly, p9.ReadWrite} { + t.Run(fmt.Sprintf("%s-%s", mode, name), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + onlyWorksOnOpenThings(h, t, name, root, mode, nil, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if shouldSucceed { + backend.EXPECT().FSync().Times(1) + } + return f.FSync() + }) + }) + } } +} - // Attach to the server. - f, err := client.Attach("") - if err != nil { - b.Fatalf("error during attach, got %v wanted nil", err) +func TestFlush(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + _, backend, f := walkHelper(h, name, root) + defer f.Close() + + backend.EXPECT().Flush() + f.Flush() + }) } +} - // Open the file. +// onlyWorksOnDirectories is a helper test method for operations that should +// only work on unopened directories, such as create, mkdir and symlink. +func onlyWorksOnDirectories(h *Harness, t *testing.T, name string, root p9.File, fn func(backend *Mock, f p9.File, shouldSucceed bool) error) { + // Walk to the file normally. + _, backend, f := walkHelper(h, name, root) + defer f.Close() + + // Only directories support mknod. + if !backend.Attr.Mode.IsDir() { + if err := fn(backend, f, false); err != syscall.EINVAL { + t.Errorf("operation got %v, wanted EINVAL", err) + } + return // Nothing else to do. + } + + // Should succeed. + if err := fn(backend, f, true); err != nil { + t.Fatalf("operation got %v, wanted nil", err) + } + + // Open the directory. + backend.EXPECT().Open(p9.ReadOnly).Times(1) if _, _, _, err := f.Open(p9.ReadOnly); err != nil { - b.Fatalf("error during open, got %v wanted nil", err) + t.Fatalf("open got %v, wanted nil", err) + } + + // Should not work again. + if err := fn(backend, f, false); err != syscall.EINVAL { + t.Fatalf("operation got %v, wanted EINVAL", err) + } +} + +func TestCreate(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + onlyWorksOnDirectories(h, t, name, root, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if !shouldSucceed { + _, _, _, _, err := f.Create("new-file", p9.ReadWrite, 0, 1, 2) + return err + } + + // If the create is going to succeed, then we + // need to create a new backend file, and we + // clone to ensure that we don't close the + // original. + _, newF, err := f.Walk(nil) + if err != nil { + t.Fatalf("clone got %v, wanted nil", err) + } + defer newF.Close() + newBackend := h.Pop(newF) + + // Run a regular FD test to validate that path. + fdTest(t, func(send *fd.FD) *fd.FD { + // Return the send FD on success. + newFile := h.NewFile()(backend) // New file with the parent backend. + newBackend.EXPECT().Create("new-file", p9.ReadWrite, p9.FileMode(0), p9.UID(1), p9.GID(2)).Return(send, newFile, p9.QID{}, uint32(0), nil) + + // Receive the fd back. + recv, _, _, _, err := newF.Create("new-file", p9.ReadWrite, 0, 1, 2) + if err != nil { + t.Fatalf("create got %v, wanted nil", err) + } + return recv + }) + + // The above will fail via normal test flow, so + // we can assume that it passed. + return nil + }) + }) + } +} + +func TestCreateInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + if _, _, _, _, err := root.Create(invalidName, p9.ReadWrite, 0, 0, 0); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +func TestMkdir(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + onlyWorksOnDirectories(h, t, name, root, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if shouldSucceed { + backend.EXPECT().Mkdir("new-directory", p9.FileMode(0), p9.UID(1), p9.GID(2)) + } + _, err := f.Mkdir("new-directory", 0, 1, 2) + return err + }) + }) + } +} + +func TestMkdirInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + if _, err := root.Mkdir(invalidName, 0, 0, 0); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +func TestSymlink(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + onlyWorksOnDirectories(h, t, name, root, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if shouldSucceed { + backend.EXPECT().Symlink("old-name", "new-name", p9.UID(1), p9.GID(2)) + } + _, err := f.Symlink("old-name", "new-name", 1, 2) + return err + }) + }) + } +} + +func TestSyminkInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + // We need only test for invalid names in the new name, + // the target can be an arbitrary string and we don't + // need to sanity check it. + if _, err := root.Symlink("old-name", invalidName, 0, 0); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +func TestLink(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + onlyWorksOnDirectories(h, t, name, root, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if shouldSucceed { + backend.EXPECT().Link(gomock.Any(), "new-link") + } + return f.Link(f, "new-link") + }) + }) + } +} + +func TestLinkInvalid(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + for name := range newTypeMap(nil) { + for _, invalidName := range allInvalidNames(name) { + if err := root.Link(root, invalidName); err != syscall.EINVAL { + t.Errorf("got %v for name %q, want EINVAL", err, invalidName) + } + } + } +} + +func TestMknod(t *testing.T) { + for name := range newTypeMap(nil) { + t.Run(name, func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + onlyWorksOnDirectories(h, t, name, root, func(backend *Mock, f p9.File, shouldSucceed bool) error { + if shouldSucceed { + backend.EXPECT().Mknod("new-block-device", p9.FileMode(0), uint32(1), uint32(2), p9.UID(3), p9.GID(4)).Times(1) + } + _, err := f.Mknod("new-block-device", 0, 1, 2, 3, 4) + return err + }) + }) } +} - // Reset the clock. - b.ResetTimer() +// concurrentFn is a specification of a concurrent operation. This is used to +// drive the concurrency tests below. +type concurrentFn struct { + name string + match func(p9.FileMode) bool + op func(h *Harness, backend *Mock, f p9.File, callback func()) +} - // Do N reads. - var buf [1]byte - for i := 0; i < b.N; i++ { - _, err := f.ReadAt(buf[:], 0) +func concurrentTest(t *testing.T, name string, fn1, fn2 concurrentFn, sameDir, expectedOkay bool) { + var ( + names1 []string + names2 []string + ) + if sameDir { + // Use the same file one directory up. + names1, names2 = []string{"one", name}, []string{"one", name} + } else { + // For different directories, just use siblings. + names1, names2 = []string{"one", name}, []string{"three", name} + } + + t.Run(fmt.Sprintf("%s(%v)+%s(%v)", fn1.name, names1, fn2.name, names2), func(t *testing.T) { + h, c := NewHarness(t) + defer h.Finish() + + _, root := newRoot(h, c) + defer root.Close() + + // Walk to both files as given. + _, f1, err := root.Walk(names1) if err != nil { - b.Fatalf("error during read %d, got %v wanted nil", i, err) + t.Fatalf("error walking, got %v, want nil", err) + } + defer f1.Close() + b1 := h.Pop(f1) + _, f2, err := root.Walk(names2) + if err != nil { + t.Fatalf("error walking, got %v, want nil", err) + } + defer f2.Close() + b2 := h.Pop(f2) + + // Are these a good match for the current test case? + if !fn1.match(b1.Attr.Mode) { + t.SkipNow() + } + if !fn2.match(b2.Attr.Mode) { + t.SkipNow() + } + + // Construct our "concurrency creator". + in1 := make(chan struct{}, 1) + in2 := make(chan struct{}, 1) + var top sync.WaitGroup + var fns sync.WaitGroup + defer top.Wait() + top.Add(2) // Accounting for below. + defer fns.Done() + fns.Add(1) // See line above; released before top.Wait. + go func() { + defer top.Done() + fn1.op(h, b1, f1, func() { + in1 <- struct{}{} + fns.Wait() + }) + }() + go func() { + defer top.Done() + fn2.op(h, b2, f2, func() { + in2 <- struct{}{} + fns.Wait() + }) + }() + + // Compute a reasonable timeout. If we expect the operation to hang, + // give it 10 milliseconds before we assert that it's fine. After all, + // there will be a lot of these tests. If we don't expect it to hang, + // give it a full minute, since the machine could be slow. + timeout := 10 * time.Millisecond + if expectedOkay { + timeout = 1 * time.Minute + } + + // Read the first channel. + var second chan struct{} + select { + case <-in1: + second = in2 + case <-in2: + second = in1 + } + + // Catch concurrency. + select { + case <-second: + // We finished successful. Is this good? Depends on the + // expected result. + if !expectedOkay { + t.Errorf("%q and %q proceeded concurrently!", fn1.name, fn2.name) + } + case <-time.After(timeout): + // Great, things did not proceed concurrently. Is that what we + // expected? + if expectedOkay { + t.Errorf("%q and %q hung concurrently!", fn1.name, fn2.name) + } + } + }) +} + +func randomFileName() string { + return fmt.Sprintf("%x", rand.Int63()) +} + +func TestConcurrency(t *testing.T) { + readExclusive := []concurrentFn{ + { + // N.B. We can't explicitly check WalkGetAttr behavior, + // but we rely on the fact that the internal code paths + // are the same. + name: "walk", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + // See the documentation of WalkCallback. + // Because walk is actually implemented by the + // mock, we need a special place for this + // callback. + // + // Note that a clone actually locks the parent + // node. So we walk from this node to test + // concurrent operations appropriately. + backend.WalkCallback = func() error { + callback() + return nil + } + f.Walk([]string{randomFileName()}) // Won't exist. + }, + }, + { + name: "fsync", + match: func(mode p9.FileMode) bool { return p9.CanOpen(mode) }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Open(gomock.Any()) + backend.EXPECT().FSync().Do(func() { + callback() + }) + f.Open(p9.ReadOnly) // Required. + f.FSync() + }, + }, + { + name: "readdir", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Open(gomock.Any()) + backend.EXPECT().Readdir(gomock.Any(), gomock.Any()).Do(func(uint64, uint32) { + callback() + }) + f.Open(p9.ReadOnly) // Required. + f.Readdir(0, 1) + }, + }, + { + name: "readlink", + match: func(mode p9.FileMode) bool { return mode.IsSymlink() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Readlink().Do(func() { + callback() + }) + f.Readlink() + }, + }, + { + name: "connect", + match: func(mode p9.FileMode) bool { return mode.IsSocket() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Connect(gomock.Any()).Do(func(p9.ConnectFlags) { + callback() + }) + f.Connect(0) + }, + }, + { + name: "open", + match: func(mode p9.FileMode) bool { return p9.CanOpen(mode) }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Open(gomock.Any()).Do(func(p9.OpenFlags) { + callback() + }) + f.Open(p9.ReadOnly) + }, + }, + { + name: "flush", + match: func(mode p9.FileMode) bool { return true }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Flush().Do(func() { + callback() + }) + f.Flush() + }, + }, + } + writeExclusive := []concurrentFn{ + { + // N.B. We can't really check getattr. But this is an + // extremely low-risk function, it seems likely that + // this check is paranoid anyways. + name: "setattr", + match: func(mode p9.FileMode) bool { return true }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().SetAttr(gomock.Any(), gomock.Any()).Do(func(p9.SetAttrMask, p9.SetAttr) { + callback() + }) + f.SetAttr(p9.SetAttrMask{}, p9.SetAttr{}) + }, + }, + { + name: "unlinkAt", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().UnlinkAt(gomock.Any(), gomock.Any()).Do(func(string, uint32) { + callback() + }) + f.UnlinkAt(randomFileName(), 0) + }, + }, + { + name: "mknod", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Mknod(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(func(string, p9.FileMode, uint32, uint32, p9.UID, p9.GID) { + callback() + }) + f.Mknod(randomFileName(), 0, 0, 0, 0, 0) + }, + }, + { + name: "link", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Link(gomock.Any(), gomock.Any()).Do(func(p9.File, string) { + callback() + }) + f.Link(f, randomFileName()) + }, + }, + { + name: "symlink", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Symlink(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(func(string, string, p9.UID, p9.GID) { + callback() + }) + f.Symlink(randomFileName(), randomFileName(), 0, 0) + }, + }, + { + name: "mkdir", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().Mkdir(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(func(string, p9.FileMode, p9.UID, p9.GID) { + callback() + }) + f.Mkdir(randomFileName(), 0, 0, 0) + }, + }, + { + name: "create", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + // Return an error for the creation operation, as this is the simplest. + backend.EXPECT().Create(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil, p9.QID{}, uint32(0), syscall.EINVAL).Do(func(string, p9.OpenFlags, p9.FileMode, p9.UID, p9.GID) { + callback() + }) + f.Create(randomFileName(), p9.ReadOnly, 0, 0, 0) + }, + }, + } + globalExclusive := []concurrentFn{ + { + name: "remove", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + // Remove operates on a locked parent. So we + // add a child, walk to it and call remove. + // Note that because this operation can operate + // concurrently with itself, we need to + // generate a random file name. + randomFile := randomFileName() + backend.AddChild(randomFile, h.NewFile()) + defer backend.RemoveChild(randomFile) + _, file, err := f.Walk([]string{randomFile}) + if err != nil { + h.t.Fatalf("walk got %v, want nil", err) + } + + // Remove is automatically translated to the parent. + backend.EXPECT().UnlinkAt(gomock.Any(), gomock.Any()).Do(func(string, uint32) { + callback() + }) + + // Remove is also a close. + file.(deprecatedRemover).Remove() + }, + }, + { + name: "rename", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + // Similarly to remove, because we need to + // operate on a child, we allow a walk. + randomFile := randomFileName() + backend.AddChild(randomFile, h.NewFile()) + defer backend.RemoveChild(randomFile) + _, file, err := f.Walk([]string{randomFile}) + if err != nil { + h.t.Fatalf("walk got %v, want nil", err) + } + defer file.Close() + fileBackend := h.Pop(file) + + // Rename is automatically translated to the parent. + backend.EXPECT().RenameAt(gomock.Any(), gomock.Any(), gomock.Any()).Do(func(string, p9.File, string) { + callback() + }) + + // Attempt the rename. + fileBackend.EXPECT().Renamed(gomock.Any(), gomock.Any()) + file.Rename(f, randomFileName()) + }, + }, + { + name: "renameAt", + match: func(mode p9.FileMode) bool { return mode.IsDir() }, + op: func(h *Harness, backend *Mock, f p9.File, callback func()) { + backend.EXPECT().RenameAt(gomock.Any(), gomock.Any(), gomock.Any()).Do(func(string, p9.File, string) { + callback() + }) + + // Attempt the rename. There are no active fids + // with this name, so we don't need to expect + // Renamed hooks on anything. + f.RenameAt(randomFileName(), f, randomFileName()) + }, + }, + } + + for _, fn1 := range readExclusive { + for _, fn2 := range readExclusive { + for name := range newTypeMap(nil) { + // Everything should be able to proceed in parallel. + concurrentTest(t, name, fn1, fn2, true, true) + concurrentTest(t, name, fn1, fn2, false, true) + } + } + } + + for _, fn1 := range append(readExclusive, writeExclusive...) { + for _, fn2 := range writeExclusive { + for name := range newTypeMap(nil) { + // Only cross-directory functions should proceed in parallel. + concurrentTest(t, name, fn1, fn2, true, false) + concurrentTest(t, name, fn1, fn2, false, true) + } + } + } + + for _, fn1 := range append(append(readExclusive, writeExclusive...), globalExclusive...) { + for _, fn2 := range globalExclusive { + for name := range newTypeMap(nil) { + // Nothing should be able to run in parallel. + concurrentTest(t, name, fn1, fn2, true, false) + concurrentTest(t, name, fn1, fn2, false, false) + } } } } diff --git a/pkg/p9/p9test/mocks.go b/pkg/p9/p9test/mocks.go deleted file mode 100644 index 9a8c14975..000000000 --- a/pkg/p9/p9test/mocks.go +++ /dev/null @@ -1,489 +0,0 @@ -// Copyright 2018 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package p9test - -import ( - "gvisor.googlesource.com/gvisor/pkg/fd" - "gvisor.googlesource.com/gvisor/pkg/p9" -) - -// StatFSMock mocks p9.File.StatFS. -type StatFSMock struct { - Called bool - - // Return. - Stat p9.FSStat - Err error -} - -// StatFS implements p9.File.StatFS. -func (f *StatFSMock) StatFS() (p9.FSStat, error) { - f.Called = true - return f.Stat, f.Err -} - -// GetAttrMock mocks p9.File.GetAttr. -type GetAttrMock struct { - Called bool - - // Args. - Req p9.AttrMask - - // Return. - QID p9.QID - Valid p9.AttrMask - Attr p9.Attr - Err error -} - -// GetAttr implements p9.File.GetAttr. -func (g *GetAttrMock) GetAttr(req p9.AttrMask) (p9.QID, p9.AttrMask, p9.Attr, error) { - g.Called, g.Req = true, req - return g.QID, g.Valid, g.Attr, g.Err -} - -// WalkGetAttrMock mocks p9.File.WalkGetAttr. -type WalkGetAttrMock struct { - Called bool - - // Args. - Names []string - - // Return. - QIDs []p9.QID - File p9.File - Valid p9.AttrMask - Attr p9.Attr - Err error -} - -// WalkGetAttr implements p9.File.WalkGetAttr. -func (w *WalkGetAttrMock) WalkGetAttr(names []string) ([]p9.QID, p9.File, p9.AttrMask, p9.Attr, error) { - w.Called = true - w.Names = append(w.Names, names...) - return w.QIDs, w.File, w.Valid, w.Attr, w.Err -} - -// SetAttrMock mocks p9.File.SetAttr. -type SetAttrMock struct { - Called bool - - // Args. - Valid p9.SetAttrMask - Attr p9.SetAttr - - // Return. - Err error -} - -// SetAttr implements p9.File.SetAttr. -func (s *SetAttrMock) SetAttr(valid p9.SetAttrMask, attr p9.SetAttr) error { - s.Called, s.Valid, s.Attr = true, valid, attr - return s.Err -} - -// RemoveMock mocks p9.File.Remove. -type RemoveMock struct { - Called bool - - // Return. - Err error -} - -// Remove implements p9.File.Remove. -func (r *RemoveMock) Remove() error { - r.Called = true - return r.Err -} - -// OpenMock mocks p9.File.Open. -type OpenMock struct { - Called bool - - // Args. - Flags p9.OpenFlags - - // Return. - File *fd.FD - QID p9.QID - IOUnit uint32 - Err error -} - -// Open implements p9.File.Open. -func (o *OpenMock) Open(flags p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) { - o.Called, o.Flags = true, flags - return o.File, o.QID, o.IOUnit, o.Err -} - -// ReadAtMock mocks p9.File.ReadAt. -type ReadAtMock struct { - Called bool - - // Args. - P []byte - Offset uint64 - - // Return. - N int - Err error -} - -// ReadAt implements p9.File.ReadAt. -func (r *ReadAtMock) ReadAt(p []byte, offset uint64) (int, error) { - r.Called, r.P, r.Offset = true, p, offset - return r.N, r.Err -} - -// WriteAtMock mocks p9.File.WriteAt. -type WriteAtMock struct { - Called bool - - // Args. - P []byte - Offset uint64 - - // Return. - N int - Err error -} - -// WriteAt implements p9.File.WriteAt. -func (w *WriteAtMock) WriteAt(p []byte, offset uint64) (int, error) { - w.Called, w.P, w.Offset = true, p, offset - return w.N, w.Err -} - -// FSyncMock mocks p9.File.FSync. -type FSyncMock struct { - Called bool - - // Return. - Err error -} - -// FSync implements p9.File.FSync. -func (f *FSyncMock) FSync() error { - f.Called = true - return f.Err -} - -// MkdirMock mocks p9.File.Mkdir. -type MkdirMock struct { - Called bool - - // Args. - Name string - Permissions p9.FileMode - UID p9.UID - GID p9.GID - - // Return. - QID p9.QID - Err error -} - -// Mkdir implements p9.File.Mkdir. -func (s *MkdirMock) Mkdir(name string, permissions p9.FileMode, uid p9.UID, gid p9.GID) (p9.QID, error) { - s.Called, s.Name, s.Permissions, s.UID, s.GID = true, name, permissions, uid, gid - return s.QID, s.Err -} - -// SymlinkMock mocks p9.File.Symlink. -type SymlinkMock struct { - Called bool - - // Args. - Oldname string - Newname string - UID p9.UID - GID p9.GID - - // Return. - QID p9.QID - Err error -} - -// Symlink implements p9.File.Symlink. -func (s *SymlinkMock) Symlink(oldname string, newname string, uid p9.UID, gid p9.GID) (p9.QID, error) { - s.Called, s.Oldname, s.Newname, s.UID, s.GID = true, oldname, newname, uid, gid - return s.QID, s.Err -} - -// MknodMock mocks p9.File.Mknod. -type MknodMock struct { - Called bool - - // Args. - Name string - Permissions p9.FileMode - Major uint32 - Minor uint32 - UID p9.UID - GID p9.GID - - // Return. - QID p9.QID - Err error -} - -// Mknod implements p9.File.Mknod. -func (m *MknodMock) Mknod(name string, permissions p9.FileMode, major uint32, minor uint32, uid p9.UID, gid p9.GID) (p9.QID, error) { - m.Called, m.Name, m.Permissions, m.Major, m.Minor, m.UID, m.GID = true, name, permissions, major, minor, uid, gid - return m.QID, m.Err -} - -// UnlinkAtMock mocks p9.File.UnlinkAt. -type UnlinkAtMock struct { - Called bool - - // Args. - Name string - Flags uint32 - - // Return. - Err error -} - -// UnlinkAt implements p9.File.UnlinkAt. -func (u *UnlinkAtMock) UnlinkAt(name string, flags uint32) error { - u.Called, u.Name, u.Flags = true, name, flags - return u.Err -} - -// ReaddirMock mocks p9.File.Readdir. -type ReaddirMock struct { - Called bool - - // Args. - Offset uint64 - Count uint32 - - // Return. - Dirents []p9.Dirent - Err error -} - -// Readdir implements p9.File.Readdir. -func (r *ReaddirMock) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { - r.Called, r.Offset, r.Count = true, offset, count - return r.Dirents, r.Err -} - -// ReadlinkMock mocks p9.File.Readlink. -type ReadlinkMock struct { - Called bool - - // Return. - Target string - Err error -} - -// Readlink implements p9.File.Readlink. -func (r *ReadlinkMock) Readlink() (string, error) { - r.Called = true - return r.Target, r.Err -} - -// AttachMock mocks p9.Attacher.Attach. -type AttachMock struct { - Called bool - - // Return. - File p9.File - Err error -} - -// Attach implements p9.Attacher.Attach. -func (a *AttachMock) Attach() (p9.File, error) { - a.Called = true - return a.File, a.Err -} - -// WalkMock mocks p9.File.Walk. -type WalkMock struct { - Called bool - - // Args. - Names []string - - // Return. - QIDs []p9.QID - File p9.File - Err error -} - -// Walk implements p9.File.Walk. -func (w *WalkMock) Walk(names []string) ([]p9.QID, p9.File, error) { - w.Called = true - w.Names = append(w.Names, names...) - return w.QIDs, w.File, w.Err -} - -// RenameMock mocks p9.File.Rename. -type RenameMock struct { - Called bool - - // Args. - Directory p9.File - Name string - - // Return. - Err error -} - -// Rename implements p9.File.Rename. -func (r *RenameMock) Rename(directory p9.File, name string) error { - r.Called, r.Directory, r.Name = true, directory, name - return r.Err -} - -// CloseMock mocks p9.File.Close. -type CloseMock struct { - Called bool - - // Return. - Err error -} - -// Close implements p9.File.Close. -func (d *CloseMock) Close() error { - d.Called = true - return d.Err -} - -// CreateMock mocks p9.File.Create. -type CreateMock struct { - Called bool - - // Args. - Name string - Flags p9.OpenFlags - Permissions p9.FileMode - UID p9.UID - GID p9.GID - - // Return. - HostFile *fd.FD - File p9.File - QID p9.QID - IOUnit uint32 - Err error -} - -// Create implements p9.File.Create. -func (c *CreateMock) Create(name string, flags p9.OpenFlags, permissions p9.FileMode, uid p9.UID, gid p9.GID) (*fd.FD, p9.File, p9.QID, uint32, error) { - c.Called, c.Name, c.Flags, c.Permissions, c.UID, c.GID = true, name, flags, permissions, uid, gid - return c.HostFile, c.File, c.QID, c.IOUnit, c.Err -} - -// LinkMock mocks p9.File.Link. -type LinkMock struct { - Called bool - - // Args. - Target p9.File - Newname string - - // Return. - Err error -} - -// Link implements p9.File.Link. -func (l *LinkMock) Link(target p9.File, newname string) error { - l.Called, l.Target, l.Newname = true, target, newname - return l.Err -} - -// RenameAtMock mocks p9.File.RenameAt. -type RenameAtMock struct { - Called bool - - // Args. - Oldname string - Newdir p9.File - Newname string - - // Return. - Err error -} - -// RenameAt implements p9.File.RenameAt. -func (r *RenameAtMock) RenameAt(oldname string, newdir p9.File, newname string) error { - r.Called, r.Oldname, r.Newdir, r.Newname = true, oldname, newdir, newname - return r.Err -} - -// FlushMock mocks p9.File.Flush. -type FlushMock struct { - Called bool - - // Return. - Err error -} - -// Flush implements p9.File.Flush. -func (f *FlushMock) Flush() error { - return f.Err -} - -// ConnectMock mocks p9.File.Connect. -type ConnectMock struct { - Called bool - - // Args. - Flags p9.ConnectFlags - - // Return. - File *fd.FD - Err error -} - -// Connect implements p9.File.Connect. -func (o *ConnectMock) Connect(flags p9.ConnectFlags) (*fd.FD, error) { - o.Called, o.Flags = true, flags - return o.File, o.Err -} - -// FileMock mocks p9.File. -type FileMock struct { - WalkMock - WalkGetAttrMock - StatFSMock - GetAttrMock - SetAttrMock - RemoveMock - RenameMock - CloseMock - OpenMock - ReadAtMock - WriteAtMock - FSyncMock - CreateMock - MkdirMock - SymlinkMock - LinkMock - MknodMock - RenameAtMock - UnlinkAtMock - ReaddirMock - ReadlinkMock - FlushMock - ConnectMock -} - -var ( - _ p9.File = &FileMock{} -) diff --git a/pkg/p9/p9test/p9test.go b/pkg/p9/p9test/p9test.go new file mode 100644 index 000000000..417b55950 --- /dev/null +++ b/pkg/p9/p9test/p9test.go @@ -0,0 +1,329 @@ +// Copyright 2018 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package p9test provides standard mocks for p9. +package p9test + +import ( + "fmt" + "sync" + "sync/atomic" + "syscall" + "testing" + + "github.com/golang/mock/gomock" + "gvisor.googlesource.com/gvisor/pkg/p9" + "gvisor.googlesource.com/gvisor/pkg/unet" +) + +// Harness is an attacher mock. +type Harness struct { + t *testing.T + mockCtrl *gomock.Controller + Attacher *MockAttacher + wg sync.WaitGroup + clientSocket *unet.Socket + mu sync.Mutex + created []*Mock +} + +// globalPath is a QID.Path Generator. +var globalPath uint64 + +// MakePath returns a globally unique path. +func MakePath() uint64 { + return atomic.AddUint64(&globalPath, 1) +} + +// Generator is a function that generates a new file. +type Generator func(parent *Mock) *Mock + +// Mock is a common mock element. +type Mock struct { + p9.DefaultWalkGetAttr + *MockFile + parent *Mock + closed bool + harness *Harness + QID p9.QID + Attr p9.Attr + children map[string]Generator + + // WalkCallback is a special function that will be called from within + // the walk context. This is needed for the concurrent tests within + // this package. + WalkCallback func() error +} + +// globalMu protects the children maps in all mocks. Note that this is not a +// particularly elegant solution, but because the test has walks from the root +// through to final nodes, we must share maps below, and it's easiest to simply +// protect against concurrent access globally. +var globalMu sync.RWMutex + +// AddChild adds a new child to the Mock. +func (m *Mock) AddChild(name string, generator Generator) { + globalMu.Lock() + defer globalMu.Unlock() + m.children[name] = generator +} + +// RemoveChild removes the child with the given name. +func (m *Mock) RemoveChild(name string) { + globalMu.Lock() + defer globalMu.Unlock() + delete(m.children, name) +} + +// Matches implements gomock.Matcher.Matches. +func (m *Mock) Matches(x interface{}) bool { + if om, ok := x.(*Mock); ok { + return m.QID.Path == om.QID.Path + } + return false +} + +// String implements gomock.Matcher.String. +func (m *Mock) String() string { + return fmt.Sprintf("Mock{Mode: 0x%x, QID.Path: %d}", m.Attr.Mode, m.QID.Path) +} + +// GetAttr returns the current attributes. +func (m *Mock) GetAttr(mask p9.AttrMask) (p9.QID, p9.AttrMask, p9.Attr, error) { + return m.QID, p9.AttrMaskAll(), m.Attr, nil +} + +// Walk supports clone and walking in directories. +func (m *Mock) Walk(names []string) ([]p9.QID, p9.File, error) { + if m.WalkCallback != nil { + if err := m.WalkCallback(); err != nil { + return nil, nil, err + } + } + if len(names) == 0 { + // Clone the file appropriately. + nm := m.harness.NewMock(m.parent, m.QID.Path, m.Attr) + nm.children = m.children // Inherit children. + return []p9.QID{nm.QID}, nm, nil + } else if len(names) != 1 { + m.harness.t.Fail() // Should not happen. + return nil, nil, syscall.EINVAL + } + + if m.Attr.Mode.IsDir() { + globalMu.RLock() + defer globalMu.RUnlock() + if fn, ok := m.children[names[0]]; ok { + // Generate the child. + nm := fn(m) + return []p9.QID{nm.QID}, nm, nil + } + // No child found. + return nil, nil, syscall.ENOENT + } + + // Call the underlying mock. + return m.MockFile.Walk(names) +} + +// WalkGetAttr calls the default implementation; this is a client-side optimization. +func (m *Mock) WalkGetAttr(names []string) ([]p9.QID, p9.File, p9.AttrMask, p9.Attr, error) { + return m.DefaultWalkGetAttr.WalkGetAttr(names) +} + +// Pop pops off the most recently created Mock and assert that this mock +// represents the same file passed in. If nil is passed in, no check is +// performed. +// +// Precondition: there must be at least one Mock or this will panic. +func (h *Harness) Pop(clientFile p9.File) *Mock { + h.mu.Lock() + defer h.mu.Unlock() + + if clientFile == nil { + // If no clientFile is provided, then we always return the last + // created file. The caller can safely use this as long as + // there is no concurrency. + m := h.created[len(h.created)-1] + h.created = h.created[:len(h.created)-1] + return m + } + + qid, _, _, err := clientFile.GetAttr(p9.AttrMaskAll()) + if err != nil { + // We do not expect this to happen. + panic(fmt.Sprintf("err during Pop: %v", err)) + } + + // Find the relevant file in our created list. We must scan the last + // from back to front to ensure that we favor the most recently + // generated file. + for i := len(h.created) - 1; i >= 0; i-- { + m := h.created[i] + if qid.Path == m.QID.Path { + // Copy and truncate. + copy(h.created[i:], h.created[i+1:]) + h.created = h.created[:len(h.created)-1] + return m + } + } + + // Unable to find relevant file. + panic(fmt.Sprintf("unable to locate file with QID %+v", qid.Path)) +} + +// NewMock returns a new base file. +func (h *Harness) NewMock(parent *Mock, path uint64, attr p9.Attr) *Mock { + m := &Mock{ + MockFile: NewMockFile(h.mockCtrl), + parent: parent, + harness: h, + QID: p9.QID{ + Type: p9.QIDType((attr.Mode & p9.FileModeMask) >> 12), + Path: path, + }, + Attr: attr, + } + + // Always ensure Close is after the parent's close. Note that this + // can't be done via a straight-forward After call, because the parent + // might change after initial creation. We ensure that this is true at + // close time. + m.EXPECT().Close().Return(nil).Times(1).Do(func() { + if m.parent != nil && m.parent.closed { + h.t.FailNow() + } + // Note that this should not be racy, as this operation should + // be protected by the Times(1) above first. + m.closed = true + }) + + // Remember what was created. + h.mu.Lock() + defer h.mu.Unlock() + h.created = append(h.created, m) + + return m +} + +// NewFile returns a new file mock. +// +// Note that ReadAt and WriteAt must be mocked separately. +func (h *Harness) NewFile() Generator { + return func(parent *Mock) *Mock { + return h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeRegular}) + } +} + +// NewDirectory returns a new mock directory. +// +// Note that Mkdir, Link, Mknod, RenameAt, UnlinkAt and Readdir must be mocked +// separately. Walk is provided and children may be manipulated via AddChild +// and RemoveChild. After calling Walk remotely, one can use Pop to find the +// corresponding backend mock on the server side. +func (h *Harness) NewDirectory(contents map[string]Generator) Generator { + return func(parent *Mock) *Mock { + m := h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeDirectory}) + m.children = contents // Save contents. + return m + } +} + +// NewSymlink returns a new mock directory. +// +// Note that Readlink must be mocked separately. +func (h *Harness) NewSymlink() Generator { + return func(parent *Mock) *Mock { + return h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeSymlink}) + } +} + +// NewBlockDevice returns a new mock block device. +func (h *Harness) NewBlockDevice() Generator { + return func(parent *Mock) *Mock { + return h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeBlockDevice}) + } +} + +// NewCharacterDevice returns a new mock character device. +func (h *Harness) NewCharacterDevice() Generator { + return func(parent *Mock) *Mock { + return h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeCharacterDevice}) + } +} + +// NewNamedPipe returns a new mock named pipe. +func (h *Harness) NewNamedPipe() Generator { + return func(parent *Mock) *Mock { + return h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeNamedPipe}) + } +} + +// NewSocket returns a new mock socket. +func (h *Harness) NewSocket() Generator { + return func(parent *Mock) *Mock { + return h.NewMock(parent, MakePath(), p9.Attr{Mode: p9.ModeSocket}) + } +} + +// Finish completes all checks and shuts down the server. +func (h *Harness) Finish() { + h.clientSocket.Close() + h.wg.Wait() + h.mockCtrl.Finish() +} + +// NewHarness creates and returns a new test server. +// +// It should always be used as: +// +// h, c := NewHarness(t) +// defer h.Finish() +// +func NewHarness(t *testing.T) (*Harness, *p9.Client) { + // Create the mock. + mockCtrl := gomock.NewController(t) + h := &Harness{ + t: t, + mockCtrl: mockCtrl, + Attacher: NewMockAttacher(mockCtrl), + } + + // Make socket pair. + serverSocket, clientSocket, err := unet.SocketPair(false) + if err != nil { + t.Fatalf("socketpair got err %v wanted nil", err) + } + + // Start the server, synchronized on exit. + server := p9.NewServer(h.Attacher) + h.wg.Add(1) + go func() { + defer h.wg.Done() + server.Handle(serverSocket) + }() + + // Create the client. + client, err := p9.NewClient(clientSocket, 1024, p9.HighestVersionString()) + if err != nil { + serverSocket.Close() + clientSocket.Close() + t.Fatalf("new client got %v, expected nil", err) + return nil, nil // Never hit. + } + + // Capture the client socket. + h.clientSocket = clientSocket + return h, client +} diff --git a/pkg/p9/path_tree.go b/pkg/p9/path_tree.go new file mode 100644 index 000000000..97f90bcd5 --- /dev/null +++ b/pkg/p9/path_tree.go @@ -0,0 +1,109 @@ +// Copyright 2018 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package p9 + +import ( + "fmt" + "sync" +) + +// pathNode is a single node in a path traversal. +// +// These are shared by all fidRefs that point to the same path. +// +// These are not synchronized because we allow certain operations (file walk) +// to proceed without having to acquire a write lock. The lock in this +// structure exists to synchronize high-level, semantic operations, such as the +// simultaneous creation and deletion of a file. +// +// (+) below is the path component string. +type pathNode struct { + mu sync.RWMutex // See above. + fidRefs sync.Map // => map[*fidRef]string(+) + children sync.Map // => map[string(+)]*pathNode + count int64 +} + +// pathNodeFor returns the path node for the given name, or a new one. +// +// Precondition: mu must be held in a readable fashion. +func (p *pathNode) pathNodeFor(name string) *pathNode { + // Load the existing path node. + if pn, ok := p.children.Load(name); ok { + return pn.(*pathNode) + } + + // Create a new pathNode for shared use. + pn, _ := p.children.LoadOrStore(name, new(pathNode)) + return pn.(*pathNode) +} + +// nameFor returns the name for the given fidRef. +// +// Precondition: mu must be held in a readable fashion. +func (p *pathNode) nameFor(ref *fidRef) string { + if s, ok := p.fidRefs.Load(ref); ok { + return s.(string) + } + + // This should not happen, don't proceed. + panic(fmt.Sprintf("expected name for %+v, none found", ref)) +} + +// addChild adds a child to the given pathNode. +// +// This applies only to an individual fidRef. +// +// Precondition: mu must be held in a writable fashion. +func (p *pathNode) addChild(ref *fidRef, name string) { + if s, ok := p.fidRefs.Load(ref); ok { + // This should not happen, don't proceed. + panic(fmt.Sprintf("unexpected fidRef %+v with path %q, wanted %q", ref, s, name)) + } + + p.fidRefs.Store(ref, name) +} + +// removeChild removes the given child. +// +// This applies only to an individual fidRef. +// +// Precondition: mu must be held in a writable fashion. +func (p *pathNode) removeChild(ref *fidRef) { + p.fidRefs.Delete(ref) +} + +// removeWithName removes all references with the given name. +// +// The original pathNode is returned by this function, and removed from this +// pathNode. Any operations on the removed tree must use this value. +// +// The provided function is executed after removal. +// +// Precondition: mu must be held in a writable fashion. +func (p *pathNode) removeWithName(name string, fn func(ref *fidRef)) *pathNode { + p.fidRefs.Range(func(key, value interface{}) bool { + if value.(string) == name { + p.fidRefs.Delete(key) + fn(key.(*fidRef)) + } + return true + }) + + // Return the original path node. + origPathNode := p.pathNodeFor(name) + p.children.Delete(name) + return origPathNode +} diff --git a/pkg/p9/server.go b/pkg/p9/server.go index 5c7cb18c8..3ef151595 100644 --- a/pkg/p9/server.go +++ b/pkg/p9/server.go @@ -15,6 +15,8 @@ package p9 import ( + "io" + "runtime/debug" "sync" "sync/atomic" "syscall" @@ -27,6 +29,19 @@ import ( type Server struct { // attacher provides the attach function. attacher Attacher + + // pathTree is the full set of paths opened on this server. + // + // These may be across different connections, but rename operations + // must be serialized globally for safely. There is a single pathTree + // for the entire server, and not per connection. + pathTree pathNode + + // renameMu is a global lock protecting rename operations. With this + // lock, we can be certain that any given rename operation can safely + // acquire two path nodes in any order, as all other concurrent + // operations acquire at most a single node. + renameMu sync.RWMutex } // NewServer returns a new server. @@ -81,6 +96,9 @@ type connState struct { // fidRef wraps a node and tracks references. type fidRef struct { + // server is the associated server. + server *Server + // file is the associated File. file File @@ -97,13 +115,39 @@ type fidRef struct { // This is updated in handlers.go. opened bool - // walkable indicates this fidRef may be walked. - walkable bool + // mode is the fidRef's mode from the walk. Only the type bits are + // valid, the permissions may change. This is used to sanity check + // operations on this element, and prevent walks across + // non-directories. + mode FileMode // openFlags is the mode used in the open. // // This is updated in handlers.go. openFlags OpenFlags + + // pathNode is the current pathNode for this FID. + pathNode *pathNode + + // parent is the parent fidRef. We hold on to a parent reference to + // ensure that hooks, such as Renamed, can be executed safely by the + // server code. + // + // Note that parent cannot be changed without holding both the global + // rename lock and a writable lock on the associated pathNode for this + // fidRef. Holding either of these locks is sufficient to examine + // parent safely. + // + // The parent will be nil for root fidRefs, and non-nil otherwise. The + // method maybeParent can be used to return a cyclical reference, and + // isRoot should be used to check for root over looking at parent + // directly. + parent *fidRef + + // deleted indicates that the backing file has been deleted. We stop + // many operations at the API level if they are incompatible with a + // file that has already been unlinked. + deleted uint32 } // OpenFlags returns the flags the file was opened with and true iff the fid was opened previously. @@ -113,13 +157,146 @@ func (f *fidRef) OpenFlags() (OpenFlags, bool) { return f.openFlags, f.opened } +// IncRef increases the references on a fid. +func (f *fidRef) IncRef() { + atomic.AddInt64(&f.refs, 1) +} + // DecRef should be called when you're finished with a fid. func (f *fidRef) DecRef() { if atomic.AddInt64(&f.refs, -1) == 0 { f.file.Close() + + // Drop the parent reference. + // + // Since this fidRef is guaranteed to be non-discoverable when + // the references reach zero, we don't need to worry about + // clearing the parent. + if f.parent != nil { + // If we've been previously deleted, this removing this + // ref is a no-op. That's expected. + f.parent.pathNode.removeChild(f) + f.parent.DecRef() + } } } +// isDeleted returns true if this fidRef has been deleted. +func (f *fidRef) isDeleted() bool { + return atomic.LoadUint32(&f.deleted) != 0 +} + +// isRoot indicates whether this is a root fid. +func (f *fidRef) isRoot() bool { + return f.parent == nil +} + +// maybeParent returns a cyclic reference for roots, and the parent otherwise. +func (f *fidRef) maybeParent() *fidRef { + if f.parent != nil { + return f.parent + } + return f // Root has itself. +} + +// notifyDelete marks all fidRefs as deleted. +// +// Precondition: the write lock must be held on the given pathNode. +func notifyDelete(pn *pathNode) { + // Call on all local references. + pn.fidRefs.Range(func(key, _ interface{}) bool { + ref := key.(*fidRef) + atomic.StoreUint32(&ref.deleted, 1) + return true + }) + + // Call on all subtrees. + pn.children.Range(func(_, value interface{}) bool { + notifyDelete(value.(*pathNode)) + return true + }) +} + +// markChildDeleted marks all children below the given name as deleted. +// +// Precondition: this must be called via safelyWrite or safelyGlobal. +func (f *fidRef) markChildDeleted(name string) { + origPathNode := f.pathNode.removeWithName(name, func(ref *fidRef) { + atomic.StoreUint32(&ref.deleted, 1) + }) + + // Mark everything below as deleted. + notifyDelete(origPathNode) +} + +// notifyNameChange calls the relevant Renamed method on all nodes in the path, +// recursively. Note that this applies only for subtrees, as these +// notifications do not apply to the actual file whose name has changed. +// +// Precondition: the write lock must be held on the given pathNode. +func notifyNameChange(pn *pathNode) { + // Call on all local references. + pn.fidRefs.Range(func(key, value interface{}) bool { + ref := key.(*fidRef) + name := value.(string) + ref.file.Renamed(ref.parent.file, name) + return true + }) + + // Call on all subtrees. + pn.children.Range(func(_, value interface{}) bool { + notifyNameChange(value.(*pathNode)) + return true + }) +} + +// renameChildTo renames the given child to the target. +// +// Precondition: this must be called via safelyGlobal. +func (f *fidRef) renameChildTo(oldName string, target *fidRef, newName string) { + target.markChildDeleted(newName) + origPathNode := f.pathNode.removeWithName(oldName, func(ref *fidRef) { + ref.parent.DecRef() // Drop original reference. + ref.parent = target // Change parent. + ref.parent.IncRef() // Acquire new one. + target.pathNode.addChild(ref, newName) + ref.file.Renamed(target.file, newName) + }) + + // Replace the previous (now deleted) path node. + f.pathNode.children.Store(newName, origPathNode) + + // Call Renamed on everything above. + notifyNameChange(origPathNode) +} + +// safelyRead executes the given operation with the local path node locked. +// This implies that paths will not change during the operation. +func (f *fidRef) safelyRead(fn func() error) (err error) { + f.server.renameMu.RLock() + defer f.server.renameMu.RUnlock() + f.pathNode.mu.RLock() + defer f.pathNode.mu.RUnlock() + return fn() +} + +// safelyWrite executes the given operation with the local path node locked in +// a writable fashion. This implies some paths may change. +func (f *fidRef) safelyWrite(fn func() error) (err error) { + f.server.renameMu.RLock() + defer f.server.renameMu.RUnlock() + f.pathNode.mu.Lock() + defer f.pathNode.mu.Unlock() + return fn() +} + +// safelyGlobal executes the given operation with the global path lock held. +func (f *fidRef) safelyGlobal(fn func() error) (err error) { + f.server.renameMu.Lock() + defer f.server.renameMu.Unlock() + return fn() +} + // LookupFID finds the given FID. // // You should call fid.DecRef when you are finished using the fid. @@ -128,7 +305,7 @@ func (cs *connState) LookupFID(fid FID) (*fidRef, bool) { defer cs.fidMu.Unlock() fidRef, ok := cs.fids[fid] if ok { - atomic.AddInt64(&fidRef.refs, 1) + fidRef.IncRef() return fidRef, true } return nil, false @@ -145,7 +322,7 @@ func (cs *connState) InsertFID(fid FID, newRef *fidRef) { if ok { defer origRef.DecRef() } - atomic.AddInt64(&newRef.refs, 1) + newRef.IncRef() cs.fids[fid] = newRef } @@ -229,10 +406,9 @@ func (cs *connState) handleRequest() { cs.recvDone <- nil // Deal with other errors. - if err != nil { + if err != nil && err != io.EOF { // If it's not a connection error, but some other protocol error, // we can send a response immediately. - log.Debugf("err [%05d] %v", tag, err) cs.sendMu.Lock() err := send(cs.conn, tag, newErr(err)) cs.sendMu.Unlock() @@ -243,12 +419,38 @@ func (cs *connState) handleRequest() { // Try to start the tag. if !cs.StartTag(tag) { // Nothing we can do at this point; client is bogus. + log.Debugf("no valid tag [%05d]", tag) cs.sendDone <- ErrNoValidMessage return } // Handle the message. - var r message + var r message // r is the response. + defer func() { + if r == nil { + // Don't allow a panic to propagate. + recover() + + // Include a useful log message. + log.Warningf("panic in handler: %s", debug.Stack()) + + // Wrap in an EFAULT error; we don't really have a + // better way to describe this kind of error. It will + // usually manifest as a result of the test framework. + r = newErr(syscall.EFAULT) + } + + // Clear the tag before sending. That's because as soon as this + // hits the wire, the client can legally send another message + // with the same tag. + cs.ClearTag(tag) + + // Send back the result. + cs.sendMu.Lock() + err = send(cs.conn, tag, r) + cs.sendMu.Unlock() + cs.sendDone <- err + }() if handler, ok := m.(handler); ok { // Call the message handler. r = handler.handle(cs) @@ -256,18 +458,6 @@ func (cs *connState) handleRequest() { // Produce an ENOSYS error. r = newErr(syscall.ENOSYS) } - - // Clear the tag before sending. That's because as soon - // as this hits the wire, the client can legally send - // another message with the same tag. - cs.ClearTag(tag) - - // Send back the result. - cs.sendMu.Lock() - err = send(cs.conn, tag, r) - cs.sendMu.Unlock() - cs.sendDone <- err - return } func (cs *connState) handleRequests() { diff --git a/pkg/p9/transport.go b/pkg/p9/transport.go index 97396806c..bafb377de 100644 --- a/pkg/p9/transport.go +++ b/pkg/p9/transport.go @@ -167,7 +167,7 @@ func recv(s *unet.Socket, msize uint32, lookup lookupTagAndType) (Tag, message, r.EnableFDs(1) n, err := r.ReadVec([][]byte{hdr[:]}) - if err != nil { + if err != nil && (n == 0 || err != io.EOF) { r.CloseFDs() return NoTag, nil, ErrSocket{err} } @@ -189,10 +189,8 @@ func recv(s *unet.Socket, msize uint32, lookup lookupTagAndType) (Tag, message, // Continuing reading for a short header. for n < int(headerLength) { cur, err := r.ReadVec([][]byte{hdr[n:]}) - if err != nil { + if err != nil && (cur == 0 || err != io.EOF) { return NoTag, nil, ErrSocket{err} - } else if cur == 0 { - return NoTag, nil, ErrSocket{io.EOF} } n += cur } @@ -296,10 +294,8 @@ func recv(s *unet.Socket, msize uint32, lookup lookupTagAndType) (Tag, message, r := s.Reader(true) for n := 0; n < int(remaining); { cur, err := r.ReadVec(vecs) - if err != nil { + if err != nil && (cur == 0 || err != io.EOF) { return NoTag, nil, ErrSocket{err} - } else if cur == 0 { - return NoTag, nil, ErrSocket{io.EOF} } n += cur |