Repository: pkg/sftp
Pull request #384 (2020-09-17 16:16:49)

Refactor: request-example openfile
While looking into [Issue-383](https://github.com/pkg/sftp/issues/383), I realized that there were a small bunch of corner cases not being covered properly. This ended up being a bit more than just the openfile refactor, but in the end ends up with much improved semantics when dealing with the MemFile handler.

During development a large number of tests started panicking when I broke file creation. The tests should not panic, and their `assert.Nil` or `assert.NoError` tests have been replaced with a `require.NoError`. All `assert.Nil` of error values have been replaced with either `assert.NoError` or `require.NoError`.

As such there are a heavy number of test lines changed, which do not actually change any behavior of the tests at all.

Resolved:
* `O_TRUNC` was being ignored on existing files.
* `O_EXCL` and `O_CREATE` were being ignored.
* in most places where symlinks should be followed, they are now followed until a non-symlink file/dir, or `os.ErrNotExist`
* infinite symlinks will eventually error; following 5 symlinks deep will assume a symlink loop, and error
* creating dangling symlink _files_ should actually be allowed, mkdir dangling symlinks should fail
* `O_CREATE` with a dangling symlink should create the target filename, not the symlink‘s name
* a file being made in a symlink to a directory should be made under the target directory, not under the symlink
* new symlinks were not checking for an existing parent directory
* symlink should not be able to overwrite an existing file/dir/symlink
* new directories were not checking for an existing parent directory
* mkdir should not be able to overwrite an existing file/dir/symlink
* new file/dir/symlink creation has been unified to ensure proper locating at a canonical name is made
* rmdir should only work on directories
* remove should only work on files

Known issue:
* Absolutely sure there are still a ton of symlink corner-case issues. These would require a pretty complete and total redesign though.


Patch:
diff --git a/request-example.go b/request-example.go
index 12e02de..0de41df 100644
--- a/request-example.go
+++ b/request-example.go
@@ -5,8 +5,7 @@ package sftp
 // works as a very simple filesystem with simple flat key-value lookup system.
 
 import (
-	"bytes"
-	"fmt"
+	"errors"
 	"io"
 	"os"
 	"path"
@@ -17,68 +16,128 @@ import (
 	"time"
 )
 
+const maxSymlinkFollows = 5
+
+var errTooManySymlinks = errors.New("too many symbolic links")
+
 // InMemHandler returns a Hanlders object with the test handlers.
 func InMemHandler() Handlers {
 	root := &root{
-		files: make(map[string]*memFile),
+		rootFile: &memFile{name: "/", modtime: time.Now(), isdir: true},
+		files:    make(map[string]*memFile),
 	}
-	root.memFile = newMemFile("/", true)
 	return Handlers{root, root, root, root}
 }
 
 // Example Handlers
 func (fs *root) Fileread(r *Request) (io.ReaderAt, error) {
+	flags := r.Pflags()
+	if !flags.Read {
+		// sanity check
+		return nil, os.ErrInvalid
+	}
+
+	return fs.OpenFile(r)
+}
+
+func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
+	flags := r.Pflags()
+	if !flags.Write {
+		// sanity check
+		return nil, os.ErrInvalid
+	}
+
+	return fs.OpenFile(r)
+}
+
+func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) {
 	if fs.mockErr != nil {
 		return nil, fs.mockErr
 	}
 	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
-	fs.filesLock.Lock()
-	defer fs.filesLock.Unlock()
-	file, err := fs.fetch(r.Filepath)
+
+	fs.mu.Lock()
+	defer fs.mu.Unlock()
+
+	return fs.openfile(r.Filepath, r.Flags)
+}
+
+func (fs *root) putfile(pathname string, file *memFile) error {
+	pathname, err := fs.canonName(pathname)
 	if err != nil {
-		return nil, err
+		return err
 	}
-	if file.symlink != "" {
-		file, err = fs.fetch(file.symlink)
-		if err != nil {
-			return nil, err
-		}
+
+	if !strings.HasPrefix(pathname, "/") {
+		return os.ErrInvalid
 	}
-	return file.ReaderAt()
-}
 
-func (fs *root) getFileForWrite(r *Request) (*memFile, error) {
-	if fs.mockErr != nil {
-		return nil, fs.mockErr
+	if _, err := fs.lfetch(pathname); err != os.ErrNotExist {
+		return os.ErrExist
 	}
-	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
-	fs.filesLock.Lock()
-	defer fs.filesLock.Unlock()
-	file, err := fs.fetch(r.Filepath)
+
+	file.name = pathname
+	fs.files[pathname] = file
+
+	return nil
+}
+
+func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) {
+	pflags := newFileOpenFlags(flags)
+
+	file, err := fs.fetch(pathname)
 	if err == os.ErrNotExist {
-		dir, err := fs.fetch(path.Dir(r.Filepath))
-		if err != nil {
-			return nil, err
+		if !pflags.Creat {
+			return nil, os.ErrNotExist
 		}
-		if !dir.isdir {
-			return nil, os.ErrInvalid
+
+		var count int
+		// You can create files through dangling symlinks.
+		link, err := fs.lfetch(pathname)
+		for err == nil && link.symlink != "" {
+			if pflags.Excl {
+				// unless you also passed in O_EXCL
+				return nil, os.ErrInvalid
+			}
+
+			if count++; count > maxSymlinkFollows {
+				return nil, errTooManySymlinks
+			}
+
+			pathname = link.symlink
+			link, err = fs.lfetch(pathname)
+		}
+
+		file := &memFile{
+			modtime: time.Now(),
 		}
-		file = newMemFile(r.Filepath, false)
-		fs.files[r.Filepath] = file
+
+		if err := fs.putfile(pathname, file); err != nil {
+			return nil, err
+		}
+
+		return file, nil
 	}
-	return file, nil
-}
 
-func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
-	file, err := fs.getFileForWrite(r)
 	if err != nil {
 		return nil, err
 	}
-	return file.WriterAt()
-}
 
-func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) {
-	return fs.getFileForWrite(r)
+	if pflags.Creat && pflags.Excl {
+		return nil, os.ErrExist
+	}
+
+	if file.IsDir() {
+		return nil, os.ErrInvalid
+	}
+
+	if pflags.Trunc {
+		if err := file.Truncate(0); err != nil {
+			return nil, err
+		}
+	}
+
+	return file, nil
 }
 
 func (fs *root) Filecmd(r *Request) error {
@@ -86,84 +145,203 @@ func (fs *root) Filecmd(r *Request) error {
 		return fs.mockErr
 	}
 	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
-	fs.filesLock.Lock()
-	defer fs.filesLock.Unlock()
+
+	fs.mu.Lock()
+	defer fs.mu.Unlock()
+
 	switch r.Method {
 	case "Setstat":
-		file, err := fs.fetch(r.Filepath)
+		file, err := fs.openfile(r.Filepath, sshFxfWrite)
 		if err != nil {
 			return err
 		}
+
 		if r.AttrFlags().Size {
 			return file.Truncate(int64(r.Attributes().Size))
 		}
+
 		return nil
+
 	case "Rename":
-		file, err := fs.fetch(r.Filepath)
-		if err != nil {
-			return err
+		// SFTP-v2: "It is an error if there already exists a file with the name specified by newpath."
+		// This varies from the POSIX specification, which allows limited replacement of target files.
+		if fs.exists(r.Target) {
+			return os.ErrExist
 		}
-		if _, ok := fs.files[r.Target]; ok {
-			return &os.LinkError{Op: "rename", Old: r.Filepath, New: r.Target,
-				Err: fmt.Errorf("dest file exists")}
+
+		return fs.rename(r.Filepath, r.Target)
+
+	case "Rmdir":
+		return fs.rmdir(r.Filepath)
+
+	case "Remove":
+		// IEEE 1003.1 remove explicitly can unlink files and remove empty directories.
+		// We use instead here the semantics of unlink, which is allowed to be restricted against directories.
+		return fs.unlink(r.Filepath)
+
+	case "Mkdir":
+		return fs.mkdir(r.Filepath)
+
+	case "Link":
+		return fs.link(r.Filepath, r.Target)
+
+	case "Symlink":
+		// NOTE: r.Filepath is the target, and r.Target is the linkpath.
+		return fs.symlink(r.Filepath, r.Target)
+	}
+
+	return errors.New("unsupported")
+}
+
+func (fs *root) rename(oldpath, newpath string) error {
+	file, err := fs.lfetch(oldpath)
+	if err != nil {
+		return err
+	}
+
+	newpath, err = fs.canonName(newpath)
+	if err != nil {
+		return err
+	}
+
+	if !strings.HasPrefix(newpath, "/") {
+		return os.ErrInvalid
+	}
+
+	target, err := fs.lfetch(newpath)
+	if err != os.ErrNotExist {
+		if target == file {
+			// IEEE 1003.1: if oldpath and newpath are the same directory entry,
+			// then return no error, and perform no further action.
+			return nil
 		}
-		file.name = r.Target
-		fs.files[r.Target] = file
-		delete(fs.files, r.Filepath)
-
-		if file.IsDir() {
-			for path, file := range fs.files {
-				if strings.HasPrefix(path, r.Filepath+"/") {
-					file.name = r.Target + path[len(r.Filepath):]
-					fs.files[r.Target+path[len(r.Filepath):]] = file
-					delete(fs.files, path)
-				}
+
+		switch {
+		case file.IsDir():
+			// IEEE 1003.1: if oldpath is a directory, and newpath exists,
+			// then newpath must be a directory, and empty.
+			// It is to be removed prior to rename.
+			if err := fs.rmdir(newpath); err != nil {
+				return err
 			}
+
+		case target.IsDir():
+			// IEEE 1003.1: if oldpath is not a directory, and newpath exists,
+			// then newpath may not be a directory.
+			return syscall.EISDIR
 		}
-	case "Rmdir", "Remove":
-		file, err := fs.fetch(path.Dir(r.Filepath))
-		if err != nil {
-			return err
-		}
+	}
+
+	fs.files[newpath] = file
+
+	if file.IsDir() {
+		dirprefix := file.name + "/"
+
+		for name, file := range fs.files {
+			if strings.HasPrefix(name, dirprefix) {
+				newname := path.Join(newpath, strings.TrimPrefix(name, dirprefix))
 
-		if file.IsDir() {
-			for path := range fs.files {
-				if strings.HasPrefix(path, r.Filepath+"/") {
-					return &os.PathError{
-						Op:   "remove",
-						Path: r.Filepath + "/",
-						Err:  fmt.Errorf("directory is not empty"),
-					}
-				}
+				fs.files[newname] = file
+				file.name = newname
+				delete(fs.files, name)
 			}
 		}
+	}
 
-		delete(fs.files, r.Filepath)
+	file.name = newpath
+	delete(fs.files, oldpath)
 
-	case "Mkdir":
-		_, err := fs.fetch(path.Dir(r.Filepath))
-		if err != nil {
-			return err
-		}
-		fs.files[r.Filepath] = newMemFile(r.Filepath, true)
-	case "Link":
-		file, err := fs.fetch(r.Filepath)
-		if err != nil {
-			return err
-		}
-		if file.IsDir() {
-			return fmt.Errorf("hard link not allowed for directory")
-		}
-		fs.files[r.Target] = file
-	case "Symlink":
-		_, err := fs.fetch(r.Filepath)
-		if err != nil {
-			return err
+	return nil
+}
+
+func (fs *root) PosixRename(r *Request) error {
+	if fs.mockErr != nil {
+		return fs.mockErr
+	}
+	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
+
+	fs.mu.Lock()
+	defer fs.mu.Unlock()
+
+	return fs.rename(r.Filepath, r.Target)
+}
+
+func (fs *root) mkdir(pathname string) error {
+	dir := &memFile{
+		modtime: time.Now(),
+		isdir:   true,
+	}
+
+	return fs.putfile(pathname, dir)
+}
+
+func (fs *root) rmdir(pathname string) error {
+	// IEEE 1003.1: If pathname is a symlink, then rmdir should fail with ENOTDIR.
+	dir, err := fs.lfetch(pathname)
+	if err != nil {
+		return err
+	}
+
+	if !dir.IsDir() {
+		return syscall.ENOTDIR
+	}
+
+	// use the dir‘s internal name not the pathname we passed in.
+	// the dir.name is always the canonical name of a directory.
+	pathname = dir.name
+
+	for name := range fs.files {
+		if path.Dir(name) == pathname {
+			return errors.New("directory not empty")
 		}
-		link := newMemFile(r.Target, false)
-		link.symlink = r.Filepath
-		fs.files[r.Target] = link
 	}
+
+	delete(fs.files, pathname)
+
+	return nil
+}
+
+func (fs *root) link(oldpath, newpath string) error {
+	file, err := fs.lfetch(oldpath)
+	if err != nil {
+		return err
+	}
+
+	if file.IsDir() {
+		return errors.New("hard link not allowed for directory")
+	}
+
+	return fs.putfile(newpath, file)
+}
+
+// symlink() creates a symbolic link named `linkpath` which contains the string `target`.
+// NOTE! This would be called with `symlink(req.Filepath, req.Target)` due to different semantics.
+func (fs *root) symlink(target, linkpath string) error {
+	link := &memFile{
+		modtime: time.Now(),
+		symlink: target,
+	}
+
+	return fs.putfile(linkpath, link)
+}
+
+func (fs *root) unlink(pathname string) error {
+	// does not follow symlinks!
+	file, err := fs.lfetch(pathname)
+	if err != nil {
+		return err
+	}
+
+	if file.IsDir() {
+		// IEEE 1003.1: implementations may opt out of allowing the unlinking of directories.
+		// SFTP-v2: SSH_FXP_REMOVE may not remove directories.
+		return os.ErrInvalid
+	}
+
+	// DO NOT use the file’s internal name.
+	// because of hard-links files cannot have a single canonical name.
+	delete(fs.files, pathname)
+
 	return nil
 }
 
@@ -187,49 +365,78 @@ func (fs *root) Filelist(r *Request) (ListerAt, error) {
 		return nil, fs.mockErr
 	}
 	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
-	fs.filesLock.Lock()
-	defer fs.filesLock.Unlock()
 
-	file, err := fs.fetch(r.Filepath)
-	if err != nil {
-		return nil, err
-	}
+	fs.mu.Lock()
+	defer fs.mu.Unlock()
 
 	switch r.Method {
 	case "List":
-		if !file.IsDir() {
-			return nil, syscall.ENOTDIR
-		}
-		orderedNames := []string{}
-		for fn := range fs.files {
-			if path.Dir(fn) == r.Filepath {
-				orderedNames = append(orderedNames, fn)
-			}
-		}
-		sort.Strings(orderedNames)
-		list := make([]os.FileInfo, len(orderedNames))
-		for i, fn := range orderedNames {
-			list[i] = fs.files[fn]
+		files, err := fs.readdir(r.Filepath)
+		if err != nil {
+			return nil, err
 		}
-		return listerat(list), nil
+		return listerat(files), nil
+
 	case "Stat":
-		if file.symlink != "" {
-			file, err = fs.fetch(file.symlink)
-			if err != nil {
-				return nil, err
-			}
+		file, err := fs.fetch(r.Filepath)
+		if err != nil {
+			return nil, err
 		}
-		return listerat([]os.FileInfo{file}), nil
+		return listerat{file}, nil
+
 	case "Readlink":
-		if file.symlink != "" {
-			file, err = fs.fetch(file.symlink)
-			if err != nil {
-				return nil, err
-			}
+		symlink, err := fs.readlink(r.Filepath)
+		if err != nil {
+			return nil, err
 		}
-		return listerat([]os.FileInfo{file}), nil
+
+		// SFTP-v2: The server will respond with a SSH_FXP_NAME packet containing only
+		// one name and a dummy attributes value.
+		return listerat{
+			&memFile{
+				name: symlink,
+				err:  os.ErrNotExist, // prevent accidental use as a reader/writer.
+			},
+		}, nil
 	}
-	return nil, nil
+
+	return nil, errors.New("unsupported")
+}
+
+func (fs *root) readdir(pathname string) ([]os.FileInfo, error) {
+	dir, err := fs.fetch(pathname)
+	if err != nil {
+		return nil, err
+	}
+
+	if !dir.IsDir() {
+		return nil, syscall.ENOTDIR
+	}
+
+	var files []os.FileInfo
+
+	for name, file := range fs.files {
+		if path.Dir(name) == dir.name {
+			files = append(files, file)
+		}
+	}
+
+	sort.Slice(files, func(i, j int) bool { return files[i].Name() < files[j].Name() })
+
+	return files, nil
+}
+
+func (fs *root) readlink(pathname string) (string, error) {
+	file, err := fs.lfetch(pathname)
+	if err != nil {
+		return "", err
+	}
+
+	if file.symlink == "" {
+		return "", os.ErrInvalid
+	}
+
+	return file.symlink, nil
 }
 
 // implements LstatFileLister interface
@@ -238,22 +445,24 @@ func (fs *root) Lstat(r *Request) (ListerAt, error) {
 		return nil, fs.mockErr
 	}
 	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
-	fs.filesLock.Lock()
-	defer fs.filesLock.Unlock()
 
-	file, err := fs.fetch(r.Filepath)
+	fs.mu.Lock()
+	defer fs.mu.Unlock()
+
+	file, err := fs.lfetch(r.Filepath)
 	if err != nil {
 		return nil, err
 	}
-	return listerat([]os.FileInfo{file}), nil
+	return listerat{file}, nil
 }
 
 // In memory file-system-y thing that the Hanlders live on
 type root struct {
-	*memFile
-	files     map[string]*memFile
-	filesLock sync.Mutex
-	mockErr   error
+	rootFile *memFile
+	mockErr  error
+
+	mu    sync.Mutex
+	files map[string]*memFile
 }
 
 // Set a mocked error that the next handler call will return.
@@ -262,50 +471,107 @@ func (fs *root) returnErr(err error) {
 	fs.mockErr = err
 }
 
-func (fs *root) fetch(path string) (*memFile, error) {
+func (fs *root) lfetch(path string) (*memFile, error) {
 	if path == "/" {
-		return fs.memFile, nil
+		return fs.rootFile, nil
 	}
-	if file, ok := fs.files[path]; ok {
-		return file, nil
+
+	file, ok := fs.files[path]
+	if file == nil {
+		if ok {
+			delete(fs.files, path)
+		}
+
+		return nil, os.ErrNotExist
+	}
+
+	return file, nil
+}
+
+// canonName returns the “canonical” name of a file, that is:
+// if the directory of the pathname is a symlink, it follows that symlink to the valid directory name.
+// this is relatively easy, since `dir.name` will be the only valid canonical path for a directory.
+func (fs *root) canonName(pathname string) (string, error) {
+	dirname, filename := path.Dir(pathname), path.Base(pathname)
+
+	dir, err := fs.fetch(dirname)
+	if err != nil {
+		return "", err
+	}
+
+	if !dir.IsDir() {
+		return "", syscall.ENOTDIR
+	}
+
+	return path.Join(dir.name, filename), nil
+}
+
+func (fs *root) exists(path string) bool {
+	path, err := fs.canonName(path)
+	if err != nil {
+		return false
 	}
-	return nil, os.ErrNotExist
+
+	_, err = fs.lfetch(path)
+
+	return err != os.ErrNotExist
 }
 
-// Implements os.FileInfo, Reader and Writer interfaces.
+func (fs *root) fetch(path string) (*memFile, error) {
+	file, err := fs.lfetch(path)
+	if err != nil {
+		return nil, err
+	}
+
+	var count int
+	for file.symlink != "" {
+		if count++; count > maxSymlinkFollows {
+			return nil, errTooManySymlinks
+		}
+
+		file, err = fs.lfetch(file.symlink)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return file, nil
+}
+
+// Implements os.FileInfo, io.ReaderAt and io.WriterAt interfaces.
 // These are the 3 interfaces necessary for the Handlers.
 // Implements the optional interface TransferError.
 type memFile struct {
-	name          string
-	modtime       time.Time
-	symlink       string
-	isdir         bool
-	content       []byte
-	transferError error
-	contentLock   sync.RWMutex
-}
-
-// factory to make sure modtime is set
-func newMemFile(name string, isdir bool) *memFile {
-	return &memFile{
-		name:    name,
-		modtime: time.Now(),
-		isdir:   isdir,
-	}
+	name    string
+	modtime time.Time
+	symlink string
+	isdir   bool
+
+	mu      sync.RWMutex
+	content []byte
+	err     error
 }
 
+// These are helper functions, they must be called while holding the memFile.mu mutex
+func (f *memFile) size() int64  { return int64(len(f.content)) }
+func (f *memFile) grow(n int64) { f.content = append(f.content, make([]byte, n)...) }
+
 // Have memFile fulfill os.FileInfo interface
 func (f *memFile) Name() string { return path.Base(f.name) }
-func (f *memFile) Size() int64  { return int64(len(f.content)) }
+func (f *memFile) Size() int64 {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	return f.size()
+}
 func (f *memFile) Mode() os.FileMode {
-	ret := os.FileMode(0644)
 	if f.isdir {
-		ret = os.FileMode(0755) | os.ModeDir
+		return os.FileMode(0755) | os.ModeDir
 	}
 	if f.symlink != "" {
-		ret = os.FileMode(0777) | os.ModeSymlink
+		return os.FileMode(0777) | os.ModeSymlink
 	}
-	return ret
+	return os.FileMode(0644)
 }
 func (f *memFile) ModTime() time.Time { return f.modtime }
 func (f *memFile) IsDir() bool        { return f.isdir }
@@ -313,55 +579,71 @@ func (f *memFile) Sys() interface{} {
 	return fakeFileInfoSys()
 }
 
-// Read/Write
-func (f *memFile) ReaderAt() (io.ReaderAt, error) {
-	if f.isdir {
-		return nil, os.ErrInvalid
+func (f *memFile) ReadAt(b []byte, off int64) (int, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if f.err != nil {
+		return 0, f.err
 	}
-	return bytes.NewReader(f.content), nil
-}
 
-func (f *memFile) WriterAt() (io.WriterAt, error) {
-	if f.isdir {
-		return nil, os.ErrInvalid
+	if off < 0 {
+		return 0, errors.New("memFile.ReadAt: negative offset")
+	}
+
+	if off >= f.size() {
+		return 0, io.EOF
 	}
-	return f, nil
+
+	n := copy(b, f.content[off:])
+	if n < len(b) {
+		return n, io.EOF
+	}
+
+	return n, nil
 }
-func (f *memFile) WriteAt(p []byte, off int64) (int, error) {
+
+func (f *memFile) WriteAt(b []byte, off int64) (int, error) {
 	// fmt.Println(string(p), off)
 	// mimic write delays, should be optional
-	time.Sleep(time.Microsecond * time.Duration(len(p)))
-	f.contentLock.Lock()
-	defer f.contentLock.Unlock()
-	plen := len(p) + int(off)
-	if plen >= len(f.content) {
-		nc := make([]byte, plen)
-		copy(nc, f.content)
-		f.content = nc
+	time.Sleep(time.Microsecond * time.Duration(len(b)))
+
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if f.err != nil {
+		return 0, f.err
+	}
+
+	grow := int64(len(b)) + off - f.size()
+	if grow > 0 {
+		f.grow(grow)
 	}
-	copy(f.content[off:], p)
-	return len(p), nil
-}
 
-func (f *memFile) ReadAt(p []byte, off int64) (int, error) {
-	f.contentLock.Lock()
-	defer f.contentLock.Unlock()
-	reader := bytes.NewReader(f.content)
-	return reader.ReadAt(p, off)
+	return copy(f.content[off:], b), nil
 }
 
 func (f *memFile) Truncate(size int64) error {
-	f.contentLock.Lock()
-	defer f.contentLock.Unlock()
-	grow := size - int64(len(f.content))
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if f.err != nil {
+		return f.err
+	}
+
+	grow := size - f.size()
 	if grow <= 0 {
 		f.content = f.content[:size]
 	} else {
-		f.content = append(f.content, make([]byte, grow)...)
+		f.grow(grow)
 	}
+
 	return nil
 }
 
 func (f *memFile) TransferError(err error) {
-	f.transferError = err
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	f.err = err
 }
diff --git a/request-interfaces.go b/request-interfaces.go
index ccd89ea..e1d55f8 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -39,8 +39,7 @@ type FileWriter interface {
 	Filewrite(*Request) (io.WriterAt, error)
 }
 
-// OpenFileWriter is a FileWriter that implements the generic OpenFile
-// method.
+// OpenFileWriter is a FileWriter that implements the generic OpenFile method.
 // You need to implement this optional interface if you want to be able
 // to read and write from/to the same handle.
 // Called for Methods: Open
@@ -56,6 +55,14 @@ type FileCmder interface {
 	Filecmd(*Request) error
 }
 
+// PosixRenameFileCmder is a FileCmder that implements the Lstat method.
+// If this interface is implemented PosixRename requests will call it
+// otherwise they will be handled in the same way as Rename
+type PosixRenameFileCmder interface {
+	FileCmder
+	PosixRename(*Request) error
+}
+
 // FileLister should return an object that fulfils the ListerAt interface
 // Note in cases of an error, the error text will be sent to the client.
 // Called for Methods: List, Stat, Readlink
@@ -67,6 +74,7 @@ type FileLister interface {
 // If this interface is implemented Lstat requests will call it
 // otherwise they will be handled in the same way as Stat
 type LstatFileLister interface {
+	FileLister
 	Lstat(*Request) (ListerAt, error)
 }
 
diff --git a/request-server.go b/request-server.go
index b1fee47..2264994 100644
--- a/request-server.go
+++ b/request-server.go
@@ -223,7 +223,7 @@ func (rs *RequestServer) packetWorker(
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case *sshFxpExtendedPacketPosixRename:
-			request := NewRequest("Rename", pkt.Oldpath)
+			request := NewRequest("PosixRename", pkt.Oldpath)
 			request.Target = pkt.Newpath
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 		case hasHandle:
diff --git a/request.go b/request.go
index 4f3095c..98ff04c 100644
--- a/request.go
+++ b/request.go
@@ -57,6 +57,8 @@ func requestFromPacket(ctx context.Context, pkt hasPath) *Request {
 	case *sshFxpRenamePacket:
 		request.Target = cleanPath(p.Newpath)
 	case *sshFxpSymlinkPacket:
+		// NOTE: given a POSIX compliant signature: symlink(target, linkpath string)
+		// this makes Request.Target the linkpath, and Request.Filepath the target.
 		request.Target = cleanPath(p.Linkpath)
 	case *sshFxpExtendedPacketHardlink:
 		request.Target = cleanPath(p.Newpath)
@@ -210,7 +212,7 @@ func (r *Request) call(handlers Handlers, pkt requestPacket, alloc *allocator, o
 		return fileput(handlers.FilePut, r, pkt, alloc, orderID)
 	case "Open":
 		return fileputget(handlers.FilePut, r, pkt, alloc, orderID)
-	case "Setstat", "Rename", "Rmdir", "Mkdir", "Link", "Symlink", "Remove":
+	case "Setstat", "Rename", "Rmdir", "Mkdir", "Link", "Symlink", "Remove", "PosixRename":
 		return filecmd(handlers.FileCmd, r, pkt)
 	case "List":
 		return filelist(handlers.FileList, r, pkt)
@@ -351,12 +353,24 @@ func packetData(p requestPacket, alloc *allocator, orderID uint32) (data []byte,
 
 // wrap FileCmder handler
 func filecmd(h FileCmder, r *Request, pkt requestPacket) responsePacket {
-
 	switch p := pkt.(type) {
 	case *sshFxpFsetstatPacket:
 		r.Flags = p.Flags
 		r.Attrs = p.Attrs.([]byte)
 	}
+
+	if r.Method == "PosixRename" {
+		if posixRenamer, ok := h.(PosixRenameFileCmder); ok {
+			err := posixRenamer.PosixRename(r)
+			return statusFromError(pkt, err)
+		}
+
+		// PosixRenameFileCmder not implemented handle this request as a Rename
+		r.Method = "Rename"
+		err := h.Filecmd(r)
+		return statusFromError(pkt, err)
+	}
+
 	err := h.Filecmd(r)
 	return statusFromError(pkt, err)
 }
diff --git a/stat_plan9.go b/stat_plan9.go
index 7d82032..32f17e0 100644
--- a/stat_plan9.go
+++ b/stat_plan9.go
@@ -9,7 +9,7 @@ var EBADF = syscall.NewError("fd out of range or not open")
 
 func wrapPathError(filepath string, err error) error {
 	if errno, ok := err.(syscall.ErrorString); ok {
-		return &os.PathError{Path: filepath, Err: errno }
+		return &os.PathError{Path: filepath, Err: errno}
 	}
 	return err
 }
diff --git a/stat_posix.go b/stat_posix.go
index 45eff04..38ba26a 100644
--- a/stat_posix.go
+++ b/stat_posix.go
@@ -11,7 +11,7 @@ const EBADF = syscall.EBADF
 
 func wrapPathError(filepath string, err error) error {
 	if errno, ok := err.(syscall.Errno); ok {
-		return &os.PathError{Path: filepath, Err: errno }
+		return &os.PathError{Path: filepath, Err: errno}
 	}
 	return err
 }


Test patch:
diff --git a/request-server_test.go b/request-server_test.go
index d216d2d..7161006 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -4,9 +4,11 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net"
 	"os"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -46,7 +48,7 @@ func clientRequestServerPair(t *testing.T) *csPair {
 		}
 		ready <- true
 		fd, err := l.Accept()
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		handlers := InMemHandler()
 		var options []RequestServerOption
 		if *testAllocator {
@@ -58,7 +60,7 @@ func clientRequestServerPair(t *testing.T) *csPair {
 	<-ready
 	defer os.Remove(sock)
 	c, err := net.Dial("unix", sock)
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	client, err := NewClientPipe(c, c)
 	if err != nil {
 		t.Fatalf("%+v\n", err)
@@ -87,7 +89,8 @@ func TestRequestSplitWrite(t *testing.T) {
 	w.Write([]byte(contents))
 	w.Close()
 	r := p.testHandler()
-	f, _ := r.fetch("/foo")
+	f, err := r.fetch("/foo")
+	require.NoError(t, err)
 	assert.Equal(t, contents, string(f.content))
 	checkRequestServerAllocator(t, p)
 }
@@ -125,11 +128,11 @@ func TestRequestCacheState(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	assert.Len(t, p.svr.openRequests, 0)
 	// test operation that doesn't open/close
 	err = p.cli.Remove("/foo")
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	assert.Len(t, p.svr.openRequests, 0)
 	checkRequestServerAllocator(t, p)
 }
@@ -143,15 +146,25 @@ func putTestFile(cli *Client, path, content string) (int, error) {
 	return 0, err
 }
 
+func getTestFile(cli *Client, path string) ([]byte, error) {
+	r, err := cli.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	defer r.Close()
+
+	return ioutil.ReadAll(r)
+}
+
 func TestRequestWrite(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	n, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, 5, n)
 	r := p.testHandler()
 	f, err := r.fetch("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	assert.False(t, f.isdir)
 	assert.Equal(t, f.content, []byte("hello"))
 	checkRequestServerAllocator(t, p)
@@ -161,18 +174,17 @@ func TestRequestWriteEmpty(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	n, err := putTestFile(p.cli, "/foo", "")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, 0, n)
 	r := p.testHandler()
 	f, err := r.fetch("/foo")
-	if assert.Nil(t, err) {
-		assert.False(t, f.isdir)
-		assert.Len(t, f.content, 0)
-	}
+	require.NoError(t, err)
+	assert.False(t, f.isdir)
+	assert.Len(t, f.content, 0)
 	// lets test with an error
 	r.returnErr(os.ErrInvalid)
 	n, err = putTestFile(p.cli, "/bar", "")
-	assert.Error(t, err)
+	require.Error(t, err)
 	r.returnErr(nil)
 	assert.Equal(t, 0, n)
 	checkRequestServerAllocator(t, p)
@@ -182,10 +194,10 @@ func TestRequestFilename(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	r := p.testHandler()
 	f, err := r.fetch("/foo")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, f.Name(), "foo")
 	_, err = r.fetch("/bar")
 	assert.Error(t, err)
@@ -196,9 +208,9 @@ func TestRequestJustRead(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	rf, err := p.cli.Open("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	defer rf.Close()
 	contents := make([]byte, 5)
 	n, err := rf.Read(contents)
@@ -223,28 +235,79 @@ func TestRequestCreate(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	fh, err := p.cli.Create("foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	err = fh.Close()
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	checkRequestServerAllocator(t, p)
 }
 
 func TestRequestReadAndWrite(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
-	file, err := p.cli.OpenFile("/foo", os.O_RDWR)
-	require.NoError(t, err)
 
+	file, err := p.cli.OpenFile("/foo", os.O_RDWR|os.O_CREATE)
+	require.NoError(t, err)
 	defer file.Close()
 
 	n, err := file.Write([]byte("hello"))
 	require.NoError(t, err)
 	assert.Equal(t, 5, n)
+
 	buf := make([]byte, 4)
 	n, err = file.ReadAt(buf, 1)
 	require.NoError(t, err)
 	assert.Equal(t, 4, n)
 	assert.Equal(t, []byte{'e', 'l', 'l', 'o'}, buf)
+
+	checkRequestServerAllocator(t, p)
+}
+
+func TestOpenFileExclusive(t *testing.T) {
+	p := clientRequestServerPair(t)
+	defer p.Close()
+
+	// first open should work
+	file, err := p.cli.OpenFile("/foo", os.O_RDWR|os.O_CREATE|os.O_EXCL)
+	require.NoError(t, err)
+	file.Close()
+
+	// second open should return error
+	_, err = p.cli.OpenFile("/foo", os.O_RDWR|os.O_CREATE|os.O_EXCL)
+	assert.Error(t, err)
+
+	checkRequestServerAllocator(t, p)
+}
+
+func TestOpenFileExclusiveNoSymlinkFollowing(t *testing.T) {
+	p := clientRequestServerPair(t)
+	defer p.Close()
+
+	// make a directory
+	err := p.cli.Mkdir("/foo")
+	require.NoError(t, err)
+
+	// make a symlink to that directory
+	err = p.cli.Symlink("/foo", "/foo2")
+	require.NoError(t, err)
+
+	// with O_EXCL, we can follow directory symlinks
+	file, err := p.cli.OpenFile("/foo2/bar", os.O_RDWR|os.O_CREATE|os.O_EXCL)
+	require.NoError(t, err)
+	err = file.Close()
+	require.NoError(t, err)
+
+	// we should have created the file above; and this create should fail.
+	_, err = p.cli.OpenFile("/foo/bar", os.O_RDWR|os.O_CREATE|os.O_EXCL)
+	require.Error(t, err)
+
+	// create a dangling symlink
+	err = p.cli.Symlink("/notexist", "/bar")
+	require.NoError(t, err)
+
+	// opening a dangling symlink with O_CREATE and O_EXCL should fail, regardless of target not existing.
+	_, err = p.cli.OpenFile("/bar", os.O_RDWR|os.O_CREATE|os.O_EXCL)
+	require.Error(t, err)
+
 	checkRequestServerAllocator(t, p)
 }
 
@@ -252,11 +315,11 @@ func TestRequestMkdir(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	err := p.cli.Mkdir("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	r := p.testHandler()
 	f, err := r.fetch("/foo")
-	assert.Nil(t, err)
-	assert.True(t, f.isdir)
+	require.NoError(t, err)
+	assert.True(t, f.IsDir())
 	checkRequestServerAllocator(t, p)
 }
 
@@ -264,12 +327,12 @@ func TestRequestRemove(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	r := p.testHandler()
 	_, err = r.fetch("/foo")
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	err = p.cli.Remove("/foo")
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	_, err = r.fetch("/foo")
 	assert.Equal(t, err, os.ErrNotExist)
 	checkRequestServerAllocator(t, p)
@@ -278,29 +341,56 @@ func TestRequestRemove(t *testing.T) {
 func TestRequestRename(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
+
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
-	r := p.testHandler()
-	_, err = r.fetch("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
+	content, err := getTestFile(p.cli, "/foo")
+	require.NoError(t, err)
+	require.Equal(t, []byte("hello"), content)
+
 	err = p.cli.Rename("/foo", "/bar")
-	assert.Nil(t, err)
-	f, err := r.fetch("/bar")
-	if err != nil {
-		t.Fatal("unexpected error:", err)
-	}
-	assert.Equal(t, "bar", f.Name())
-	_, err = r.fetch("/foo")
-	assert.Equal(t, os.ErrNotExist, err)
+	require.NoError(t, err)
+
+	// file contents are now at /bar
+	content, err = getTestFile(p.cli, "/bar")
+	require.NoError(t, err)
+	require.Equal(t, []byte("hello"), content)
+
+	// /foo no longer exists
+	_, err = getTestFile(p.cli, "/foo")
+	require.Error(t, err)
+
+	_, err = putTestFile(p.cli, "/baz", "goodbye")
+	require.NoError(t, err)
+	content, err = getTestFile(p.cli, "/baz")
+	require.NoError(t, err)
+	require.Equal(t, []byte("goodbye"), content)
+
+	// SFTP-v2: SSH_FXP_RENAME may not overwrite existing files.
+	err = p.cli.Rename("/bar", "/baz")
+	require.Error(t, err)
+
+	// /bar and /baz are unchanged
+	content, err = getTestFile(p.cli, "/bar")
+	require.NoError(t, err)
+	require.Equal(t, []byte("hello"), content)
+	content, err = getTestFile(p.cli, "/baz")
+	require.NoError(t, err)
+	require.Equal(t, []byte("goodbye"), content)
+
+	// posix-rename@openssh.com extension allows overwriting existing files.
 	err = p.cli.PosixRename("/bar", "/baz")
-	assert.Nil(t, err)
-	f, err = r.fetch("/baz")
-	if err != nil {
-		t.Fatal("unexpected error:", err)
-	}
-	assert.Equal(t, "baz", f.Name())
-	_, err = r.fetch("/bar")
-	assert.Equal(t, os.ErrNotExist, err)
+	require.NoError(t, err)
+
+	// /baz now has the contents of /bar
+	content, err = getTestFile(p.cli, "/baz")
+	require.NoError(t, err)
+	require.Equal(t, []byte("hello"), content)
+
+	// /bar no longer exists
+	_, err = getTestFile(p.cli, "/bar")
+	require.Error(t, err)
+
 	checkRequestServerAllocator(t, p)
 }
 
@@ -308,9 +398,9 @@ func TestRequestRenameFail(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	_, err = putTestFile(p.cli, "/bar", "goodbye")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	err = p.cli.Rename("/foo", "/bar")
 	assert.IsType(t, &StatusError{}, err)
 	checkRequestServerAllocator(t, p)
@@ -320,13 +410,13 @@ func TestRequestStat(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	fi, err := p.cli.Stat("/foo")
+	require.NoError(t, err)
 	assert.Equal(t, fi.Name(), "foo")
 	assert.Equal(t, fi.Size(), int64(5))
 	assert.Equal(t, fi.Mode(), os.FileMode(0644))
 	assert.NoError(t, testOsSys(fi.Sys()))
-	assert.NoError(t, err)
 	checkRequestServerAllocator(t, p)
 }
 
@@ -336,12 +426,12 @@ func TestRequestSetstat(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	mode := os.FileMode(0644)
 	err = p.cli.Chmod("/foo", mode)
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	fi, err := p.cli.Stat("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, fi.Name(), "foo")
 	assert.Equal(t, fi.Size(), int64(5))
 	assert.Equal(t, fi.Mode(), os.FileMode(0644))
@@ -353,16 +443,15 @@ func TestRequestFstat(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	fp, err := p.cli.Open("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	fi, err := fp.Stat()
-	if assert.NoError(t, err) {
-		assert.Equal(t, fi.Name(), "foo")
-		assert.Equal(t, fi.Size(), int64(5))
-		assert.Equal(t, fi.Mode(), os.FileMode(0644))
-		assert.NoError(t, testOsSys(fi.Sys()))
-	}
+	require.NoError(t, err)
+	assert.Equal(t, fi.Name(), "foo")
+	assert.Equal(t, fi.Size(), int64(5))
+	assert.Equal(t, fi.Mode(), os.FileMode(0644))
+	assert.NoError(t, testOsSys(fi.Sys()))
 	checkRequestServerAllocator(t, p)
 }
 
@@ -422,14 +511,17 @@ func TestRequestLstat(t *testing.T) {
 func TestRequestLink(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
+
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
+
 	err = p.cli.Link("/foo", "/bar")
-	assert.Nil(t, err)
-	r := p.testHandler()
-	fi, err := r.fetch("/bar")
-	assert.Nil(t, err)
-	assert.True(t, int(fi.Size()) == len("hello"))
+	require.NoError(t, err)
+
+	content, err := getTestFile(p.cli, "/bar")
+	assert.NoError(t, err)
+	assert.Equal(t, []byte("hello"), content)
+
 	checkRequestServerAllocator(t, p)
 }
 
@@ -445,22 +537,143 @@ func TestRequestLinkFail(t *testing.T) {
 func TestRequestSymlink(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
+
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
+
 	err = p.cli.Symlink("/foo", "/bar")
-	assert.Nil(t, err)
+	require.NoError(t, err)
+	err = p.cli.Symlink("/bar", "/baz")
+	require.NoError(t, err)
+
 	r := p.testHandler()
-	fi, err := r.fetch("/bar")
-	assert.Nil(t, err)
+
+	fi, err := r.lfetch("/bar")
+	require.NoError(t, err)
 	assert.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink)
+
+	fi, err = r.lfetch("/baz")
+	require.NoError(t, err)
+	assert.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink)
+
+	content, err := getTestFile(p.cli, "/baz")
+	require.NoError(t, err)
+	assert.Equal(t, []byte("hello"), content)
+
 	checkRequestServerAllocator(t, p)
 }
 
-func TestRequestSymlinkFail(t *testing.T) {
+func TestRequestSymlinkLoop(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
+
 	err := p.cli.Symlink("/foo", "/bar")
-	assert.True(t, os.IsNotExist(err))
+	require.NoError(t, err)
+	err = p.cli.Symlink("/bar", "/baz")
+	require.NoError(t, err)
+	err = p.cli.Symlink("/baz", "/foo")
+	require.NoError(t, err)
+
+	// test should fail if we reach this point
+	timer := time.NewTimer(1 * time.Second)
+	defer timer.Stop()
+
+	var content []byte
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+
+		content, err = getTestFile(p.cli, "/bar")
+	}()
+
+	select {
+	case <-timer.C:
+		t.Fatal("symlink loop following timed out")
+		return // just to let the compiler be absolutely sure
+
+	case <-done:
+	}
+
+	assert.Error(t, err)
+	assert.Len(t, content, 0)
+
+	checkRequestServerAllocator(t, p)
+}
+
+func TestRequestSymlinkDanglingFiles(t *testing.T) {
+	p := clientRequestServerPair(t)
+	defer p.Close()
+
+	// dangling links are ok. We will use "/foo" later.
+	err := p.cli.Symlink("/foo", "/bar")
+	require.NoError(t, err)
+
+	// creating a symlink in a non-existant directory should fail.
+	err = p.cli.Symlink("/dangle", "/foo/bar")
+	require.Error(t, err)
+
+	// creating a symlink under a dangling symlink should fail.
+	err = p.cli.Symlink("/dangle", "/bar/bar")
+	require.Error(t, err)
+
+	// opening a dangling link without O_CREATE should fail with os.IsNotExist == true
+	_, err = p.cli.OpenFile("/bar", os.O_RDONLY)
+	require.True(t, os.IsNotExist(err))
+
+	// overwriting a symlink is not allowed.
+	err = p.cli.Symlink("/dangle", "/bar")
+	require.Error(t, err)
+
+	// double symlink
+	err = p.cli.Symlink("/bar", "/baz")
+	require.NoError(t, err)
+
+	// opening a dangling link with O_CREATE should work.
+	_, err = putTestFile(p.cli, "/baz", "hello")
+	require.NoError(t, err)
+
+	// dangling link creation should create the target file itself.
+	content, err := getTestFile(p.cli, "/foo")
+	require.NoError(t, err)
+	assert.Equal(t, []byte("hello"), content)
+
+	// creating a symlink under a non-directory file should fail.
+	err = p.cli.Symlink("/dangle", "/foo/bar")
+	assert.Error(t, err)
+
+	checkRequestServerAllocator(t, p)
+}
+
+func TestRequestSymlinkDanglingDirectories(t *testing.T) {
+	p := clientRequestServerPair(t)
+	defer p.Close()
+
+	// dangling links are ok. We will use "/foo" later.
+	err := p.cli.Symlink("/foo", "/bar")
+	require.NoError(t, err)
+
+	// reading from a dangling symlink should fail.
+	_, err = p.cli.ReadDir("/bar")
+	require.True(t, os.IsNotExist(err))
+
+	// making a directory on a dangling symlink SHOULD NOT work.
+	err = p.cli.Mkdir("/bar")
+	require.Error(t, err)
+
+	// ok, now make directory, so we can test make files through the symlink.
+	err = p.cli.Mkdir("/foo")
+	require.NoError(t, err)
+
+	// should be able to make a file in that symlinked directory.
+	_, err = putTestFile(p.cli, "/bar/baz", "hello")
+	require.NoError(t, err)
+
+	// dangling directory creation should create the target directory itself.
+	content, err := getTestFile(p.cli, "/foo/baz")
+	assert.NoError(t, err)
+	assert.Equal(t, []byte("hello"), content)
+
 	checkRequestServerAllocator(t, p)
 }
 
@@ -468,11 +681,11 @@ func TestRequestReadlink(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	_, err := putTestFile(p.cli, "/foo", "hello")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	err = p.cli.Symlink("/foo", "/bar")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	rl, err := p.cli.ReadLink("/bar")
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	assert.Equal(t, "foo", rl)
 	checkRequestServerAllocator(t, p)
 }
@@ -494,8 +707,8 @@ func TestRequestReaddir(t *testing.T) {
 	_, err = p.cli.ReadDir("/does_not_exist")
 	assert.Equal(t, os.ErrNotExist, err)
 	di, err := p.cli.ReadDir("/")
-	assert.Nil(t, err)
-	assert.Len(t, di, 100)
+	require.NoError(t, err)
+	require.Len(t, di, 100)
 	names := []string{di[18].Name(), di[81].Name()}
 	assert.Equal(t, []string{"foo_18", "foo_81"}, names)
 	checkRequestServerAllocator(t, p)