Repository: pkg/sftp (go)

Issue:
Request server: allow to open a file for both reading and writing
Hi,

based on the SFTP specs opening a file for both reading and writing is allowed

```
SSH_FXF_READ
      Open the file for reading.

SSH_FXF_WRITE
      Open the file for writing.  If both this and SSH_FXF_READ are
      specified, the file is opened for both reading and writing.
```

pkg/sftp currently does not support this mode. I think this is the reason for this [issue](https://github.com/drakkan/sftpgo/issues/155#issuecomment-676798820). 

Here are the OpenSSH logs when opening a file using sshfs-win:

```
ago 20 16:29:10 p1 sftp-server[9254]: debug3: request 168: open flags 3
ago 20 16:29:10 p1 sftp-server[9254]: open "/tmp/a/Nuovo documento di testo (2).txt" flags READ,WRITE mode 00
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 168: sent handle handle 2
ago 20 16:29:10 p1 sftp-server[9254]: debug3: request 169: lstat
ago 20 16:29:10 p1 sftp-server[9254]: lstat name "/tmp/a/Nuovo documento di testo (2).txt"
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 169: sent attrib have 0xf
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 170: write "/tmp/a/Nuovo documento di testo (2).txt" (handle 2) off 0 len 15
ago 20 16:29:10 p1 sftp-server[9254]: debug3: request 170: sent status 0
ago 20 16:29:10 p1 sftp-server[9254]: sent status Success
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 171: fstat "/tmp/a/Nuovo documento di testo (2).txt" (handle 2)
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 171: sent attrib have 0xf
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 172: fsetstat handle 2
ago 20 16:29:10 p1 sftp-server[9254]: set "/tmp/a/Nuovo documento di testo (2).txt" size 15
ago 20 16:29:10 p1 sftp-server[9254]: debug3: request 172: sent status 0
ago 20 16:29:10 p1 sftp-server[9254]: sent status Success
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 173: fstat "/tmp/a/Nuovo documento di testo (2).txt" (handle 2)
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 173: sent attrib have 0xf
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 174: read "/tmp/a/Nuovo documento di testo (2).txt" (handle 2) off 0 len 4096
ago 20 16:29:10 p1 sftp-server[9254]: debug1: request 174: sent data len 15
ago 20 16:29:10 p1 sftp-server[9254]: debug3: request 175: close handle 2
ago 20 16:29:10 p1 sftp-server[9254]: close "/tmp/a/Nuovo documento di testo (2).txt" bytes read 15 written 15
```

as you can see OpenSSH uses the same handle for both reads and writes.

How do you suggest to support this mode in pkg/sftp? Is ok a new request method, for example "PutGet" mapped to a new interface, for example Filereadwrite that returns both an io.ReaderAt and an io.WriterAt? Do you have other suggestions? Thank you

Pull request:
request-server: implement Open method
add an optional interface to FileWriter to allow to open a file for both
writing and reading

Fixes #374

I noticed that an `Open` method is documented but not implemented, see [here](https://github.com/pkg/sftp/blob/master/request-interfaces.go#L30). I implemented this method. The change is backward compatible and implementing the new optional Filewriter interface the request server can handle both reading and writing to the same file fixing the corruption issue reported in #374

I know that we want something different for v2 and so I'll not push this change myself.  

However I cannot wait until we implement v2 to fix a file corruption issue in SFTPGo, so if we don't agree to merge this change I'll maintain it in my fork until we have an alternative, thank you

Patch:
diff --git a/request-example.go b/request-example.go
index 012c6ae..333eba6 100644
--- a/request-example.go
+++ b/request-example.go
@@ -9,7 +9,7 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path/filepath"
+	"path"
 	"sort"
 	"strings"
 	"sync"
@@ -47,7 +47,7 @@ func (fs *root) Fileread(r *Request) (io.ReaderAt, error) {
 	return file.ReaderAt()
 }
 
-func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
+func (fs *root) getFileForWrite(r *Request) (*memFile, error) {
 	if fs.mockErr != nil {
 		return nil, fs.mockErr
 	}
@@ -56,7 +56,7 @@ func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
 	defer fs.filesLock.Unlock()
 	file, err := fs.fetch(r.Filepath)
 	if err == os.ErrNotExist {
-		dir, err := fs.fetch(filepath.Dir(r.Filepath))
+		dir, err := fs.fetch(path.Dir(r.Filepath))
 		if err != nil {
 			return nil, err
 		}
@@ -66,9 +66,21 @@ func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
 		file = newMemFile(r.Filepath, false)
 		fs.files[r.Filepath] = file
 	}
+	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)
+}
+
 func (fs *root) Filecmd(r *Request) error {
 	if fs.mockErr != nil {
 		return fs.mockErr
@@ -109,7 +121,7 @@ func (fs *root) Filecmd(r *Request) error {
 			}
 		}
 	case "Rmdir", "Remove":
-		file, err := fs.fetch(filepath.Dir(r.Filepath))
+		file, err := fs.fetch(path.Dir(r.Filepath))
 		if err != nil {
 			return err
 		}
@@ -129,7 +141,7 @@ func (fs *root) Filecmd(r *Request) error {
 		delete(fs.files, r.Filepath)
 
 	case "Mkdir":
-		_, err := fs.fetch(filepath.Dir(r.Filepath))
+		_, err := fs.fetch(path.Dir(r.Filepath))
 		if err != nil {
 			return err
 		}
@@ -190,7 +202,7 @@ func (fs *root) Filelist(r *Request) (ListerAt, error) {
 		}
 		orderedNames := []string{}
 		for fn := range fs.files {
-			if filepath.Dir(fn) == r.Filepath {
+			if path.Dir(fn) == r.Filepath {
 				orderedNames = append(orderedNames, fn)
 			}
 		}
@@ -261,7 +273,7 @@ func newMemFile(name string, isdir bool) *memFile {
 }
 
 // Have memFile fulfill os.FileInfo interface
-func (f *memFile) Name() string { return filepath.Base(f.name) }
+func (f *memFile) Name() string { return path.Base(f.name) }
 func (f *memFile) Size() int64  { return int64(len(f.content)) }
 func (f *memFile) Mode() os.FileMode {
 	ret := os.FileMode(0644)
@@ -309,6 +321,13 @@ func (f *memFile) WriteAt(p []byte, off int64) (int, error) {
 	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)
+}
+
 func (f *memFile) Truncate(size int64) error {
 	f.contentLock.Lock()
 	defer f.contentLock.Unlock()
diff --git a/request-interfaces.go b/request-interfaces.go
index dd224cd..1e85eb1 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -5,6 +5,13 @@ import (
 	"os"
 )
 
+// WriterAtReaderAt defines the interface to return when a file is to
+// be opened for reading and writing
+type WriterAtReaderAt interface {
+	io.WriterAt
+	io.ReaderAt
+}
+
 // Interfaces are differentiated based on required returned values.
 // All input arguments are to be pulled from Request (the only arg).
 
@@ -32,6 +39,16 @@ type FileWriter interface {
 	Filewrite(*Request) (io.WriterAt, error)
 }
 
+// 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
+type OpenFileWriter interface {
+	FileWriter
+	OpenFile(*Request) (WriterAtReaderAt, error)
+}
+
 // FileCmder should return an error
 // Note in cases of an error, the error text will be sent to the client.
 // Called for Methods: Setstat, Rename, Rmdir, Mkdir, Link, Symlink, Remove
diff --git a/request.go b/request.go
index df2d037..fa013f8 100644
--- a/request.go
+++ b/request.go
@@ -34,10 +34,11 @@ type Request struct {
 
 type state struct {
 	*sync.RWMutex
-	writerAt io.WriterAt
-	readerAt io.ReaderAt
-	listerAt ListerAt
-	lsoffset int64
+	writerAt       io.WriterAt
+	readerAt       io.ReaderAt
+	writerReaderAt WriterAtReaderAt
+	listerAt       ListerAt
+	lsoffset       int64
 }
 
 // New Request initialized based on packet data
@@ -142,6 +143,7 @@ func (r *Request) close() error {
 	r.state.RLock()
 	wr := r.state.writerAt
 	rd := r.state.readerAt
+	rw := r.state.writerReaderAt
 	r.state.RUnlock()
 
 	var err error
@@ -155,6 +157,14 @@ func (r *Request) close() error {
 		}
 	}
 
+	if c, ok := rw.(io.Closer); ok {
+		if err2 := c.Close(); err == nil {
+			// update error if it is still nil
+			err = err2
+			r.state.writerReaderAt = nil
+		}
+	}
+
 	if c, ok := rd.(io.Closer); ok {
 		if err2 := c.Close(); err == nil {
 			// update error if it is still nil
@@ -165,7 +175,7 @@ func (r *Request) close() error {
 	return err
 }
 
-// Close reader/writer if possible
+// Notify transfer error if any
 func (r *Request) transferError(err error) {
 	if err == nil {
 		return
@@ -174,12 +184,17 @@ func (r *Request) transferError(err error) {
 	r.state.RLock()
 	wr := r.state.writerAt
 	rd := r.state.readerAt
+	rw := r.state.writerReaderAt
 	r.state.RUnlock()
 
 	if t, ok := wr.(TransferError); ok {
 		t.TransferError(err)
 	}
 
+	if t, ok := rw.(TransferError); ok {
+		t.TransferError(err)
+	}
+
 	if t, ok := rd.(TransferError); ok {
 		t.TransferError(err)
 	}
@@ -192,6 +207,8 @@ func (r *Request) call(handlers Handlers, pkt requestPacket, alloc *allocator, o
 		return fileget(handlers.FileGet, r, pkt, alloc, orderID)
 	case "Put":
 		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":
 		return filecmd(handlers.FileCmd, r, pkt)
 	case "List":
@@ -207,26 +224,42 @@ func (r *Request) call(handlers Handlers, pkt requestPacket, alloc *allocator, o
 // Additional initialization for Open packets
 func (r *Request) open(h Handlers, pkt requestPacket) responsePacket {
 	flags := r.Pflags()
-	var err error
+
 	switch {
 	case flags.Write, flags.Append, flags.Creat, flags.Trunc:
+		if flags.Read {
+			if openFileWriter, ok := h.FilePut.(OpenFileWriter); ok {
+				r.Method = "Open"
+				rw, err := openFileWriter.OpenFile(r)
+				if err != nil {
+					return statusFromError(pkt, err)
+				}
+				r.state.writerReaderAt = rw
+				return &sshFxpHandlePacket{ID: pkt.id(), Handle: r.handle}
+			}
+		}
+
 		r.Method = "Put"
-		r.state.writerAt, err = h.FilePut.Filewrite(r)
+		wr, err := h.FilePut.Filewrite(r)
+		if err != nil {
+			return statusFromError(pkt, err)
+		}
+		r.state.writerAt = wr
 	case flags.Read:
 		r.Method = "Get"
-		r.state.readerAt, err = h.FileGet.Fileread(r)
+		rd, err := h.FileGet.Fileread(r)
+		if err != nil {
+			return statusFromError(pkt, err)
+		}
+		r.state.readerAt = rd
 	default:
 		return statusFromError(pkt, errors.New("bad file flags"))
 	}
-	if err != nil {
-		return statusFromError(pkt, err)
-	}
 	return &sshFxpHandlePacket{ID: pkt.id(), Handle: r.handle}
 }
 func (r *Request) opendir(h Handlers, pkt requestPacket) responsePacket {
-	var err error
 	r.Method = "List"
-	r.state.listerAt, err = h.FileList.Filelist(r)
+	la, err := h.FileList.Filelist(r)
 	if err != nil {
 		switch err.(type) {
 		case syscall.Errno:
@@ -234,12 +267,12 @@ func (r *Request) opendir(h Handlers, pkt requestPacket) responsePacket {
 		}
 		return statusFromError(pkt, err)
 	}
+	r.state.listerAt = la
 	return &sshFxpHandlePacket{ID: pkt.id(), Handle: r.handle}
 }
 
 // wrap FileReader handler
 func fileget(h FileReader, r *Request, pkt requestPacket, alloc *allocator, orderID uint32) responsePacket {
-	//fmt.Println("fileget", r)
 	r.state.RLock()
 	reader := r.state.readerAt
 	r.state.RUnlock()
@@ -249,7 +282,7 @@ func fileget(h FileReader, r *Request, pkt requestPacket, alloc *allocator, orde
 
 	data, offset, _ := packetData(pkt, alloc, orderID)
 	n, err := reader.ReadAt(data, offset)
-	// only return EOF erro if no data left to read
+	// only return EOF error if no data left to read
 	if err != nil && (err != io.EOF || n == 0) {
 		return statusFromError(pkt, err)
 	}
@@ -262,7 +295,6 @@ func fileget(h FileReader, r *Request, pkt requestPacket, alloc *allocator, orde
 
 // wrap FileWriter handler
 func fileput(h FileWriter, r *Request, pkt requestPacket, alloc *allocator, orderID uint32) responsePacket {
-	//fmt.Println("fileput", r)
 	r.state.RLock()
 	writer := r.state.writerAt
 	r.state.RUnlock()
@@ -275,6 +307,36 @@ func fileput(h FileWriter, r *Request, pkt requestPacket, alloc *allocator, orde
 	return statusFromError(pkt, err)
 }
 
+// wrap OpenFileWriter handler
+func fileputget(h FileWriter, r *Request, pkt requestPacket, alloc *allocator, orderID uint32) responsePacket {
+	r.state.RLock()
+	writerReader := r.state.writerReaderAt
+	r.state.RUnlock()
+	if writerReader == nil {
+		return statusFromError(pkt, errors.New("unexpected write and read packet"))
+	}
+	switch pkt.(type) {
+	case *sshFxpReadPacket:
+		data, offset, _ := packetData(pkt, alloc, orderID)
+		n, err := writerReader.ReadAt(data, offset)
+		// only return EOF error if no data left to read
+		if err != nil && (err != io.EOF || n == 0) {
+			return statusFromError(pkt, err)
+		}
+		return &sshFxpDataPacket{
+			ID:     pkt.id(),
+			Length: uint32(n),
+			Data:   data[:n],
+		}
+	case *sshFxpWritePacket:
+		data, offset, _ := packetData(pkt, alloc, orderID)
+		_, err := writerReader.WriteAt(data, offset)
+		return statusFromError(pkt, err)
+	default:
+		return statusFromError(pkt, errors.New("unexpected packet type for read or write"))
+	}
+}
+
 // file data for additional read/write packets
 func packetData(p requestPacket, alloc *allocator, orderID uint32) (data []byte, offset int64, length uint32) {
 	switch p := p.(type) {


Test patch:
diff --git a/request-server_test.go b/request-server_test.go
index 991f36e..33ecf6a 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -80,7 +80,7 @@ func TestRequestSplitWrite(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 	w, err := p.cli.Create("/foo")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	p.cli.maxPacket = 3 // force it to send in small chunks
 	contents := "one two three four five six seven eight nine ten"
 	w.Write([]byte(contents))
@@ -228,6 +228,25 @@ func TestRequestCreate(t *testing.T) {
 	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)
+
+	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 TestRequestMkdir(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()