Repository: pkg/sftp
Pull request #373 (2020-08-20 09:13:32)

request server: add support for SSH_FXP_FSETSTAT
we need to add a case for this packet inside the packet worker otherwise
it will be handled in hasHandle case and it will become a "Put" request.

Client side if a Truncate request is called on the open file we should
send a FSETSTAT packet, the request is on the handle, and not a SETSTAT
packet that should be used for paths and not for handle.
We should probably do the same for Chmod/Chtimes/Stat too.

This patch fix file corruption using `sshfs`. Simply create a text file using sshfs and then delete some lines, the file will be corrupted.

See the truncate issue reported [here](https://github.com/drakkan/sftpgo/issues/155) for more details

puellanivis: Hm… interesting. Is this something really critical that we should address before releasing 1.12? File corrupt _does_ seems like a showstopper.
drakkan: > Hm… interesting. Is this something really critical that we should address before releasing 1.12? File corrupt _does_ seems like a showstopper.

It depends on what the client do. We will have corrupted files if the client use `SSH_FXP_FSETSTAT` to truncate the file. But we have corrupted files also if the client read and write to the same file, see #374

Probably we should also add an optional `Truncate` interface and call it  on the aleady opened file, for example in SFTPGo I have to do something like [this](https://github.com/drakkan/sftpgo/blob/master/common/connection.go#L494) because if a issue an `os.Truncate` on an already opened file I get errors on some OS (for example on Windows). Anyway I can handle this in SFTPGo. Please let me know what do you think about, thank you

Patch:
diff --git a/.travis.yml b/.travis.yml
index e949028..1cbefce 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -4,8 +4,8 @@ go_import_path: github.com/pkg/sftp
 # current and previous stable releases, plus tip
 # remember to exclude previous and tip for macs below
 go:
-  - 1.13.x
   - 1.14.x
+  - 1.15.x
   - tip
 
 os:
@@ -15,7 +15,7 @@ os:
 matrix:
   exclude:
     - os: osx
-      go: 1.13.x
+      go: 1.14.x
     - os: osx
       go: tip
 
diff --git a/client.go b/client.go
index 28ebb21..4359e78 100644
--- a/client.go
+++ b/client.go
@@ -432,6 +432,25 @@ func (c *Client) Symlink(oldname, newname string) error {
 	}
 }
 
+func (c *Client) setfstat(handle string, flags uint32, attrs interface{}) error {
+	id := c.nextID()
+	typ, data, err := c.sendPacket(sshFxpFsetstatPacket{
+		ID:     id,
+		Handle: handle,
+		Flags:  flags,
+		Attrs:  attrs,
+	})
+	if err != nil {
+		return err
+	}
+	switch typ {
+	case sshFxpStatus:
+		return normaliseError(unmarshalStatus(id, data))
+	default:
+		return unimplementedPacketErr(typ)
+	}
+}
+
 // setstat is a convience wrapper to allow for changing of various parts of the file descriptor.
 func (c *Client) setstat(path string, flags uint32, attrs interface{}) error {
 	id := c.nextID()
@@ -817,7 +836,7 @@ type File struct {
 	path   string
 	handle string
 
-	mu sync.Mutex
+	mu     sync.Mutex
 	offset uint64 // current offset within remote file
 }
 
@@ -845,13 +864,13 @@ func (f *File) Read(b []byte) (int, error) {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 
-	r, err := f.ReadAt(b, int64( f.offset ))
+	r, err := f.ReadAt(b, int64(f.offset))
 	f.offset += uint64(r)
 	return r, err
 }
 
-// ReadAt reads up to len(b) byte from the File at a given offset `off`. It returns 
-// the number of bytes read and an error, if any. ReadAt follows io.ReaderAt semantics, 
+// ReadAt reads up to len(b) byte from the File at a given offset `off`. It returns
+// the number of bytes read and an error, if any. ReadAt follows io.ReaderAt semantics,
 // so the file offset is not altered during the read.
 func (f *File) ReadAt(b []byte, off int64) (n int, err error) {
 	// Split the read into multiple maxPacket sized concurrent reads
@@ -860,7 +879,7 @@ func (f *File) ReadAt(b []byte, off int64) (n int, err error) {
 	// overlapping round trip times.
 	inFlight := 0
 	desiredInFlight := 1
-	offset := uint64( off )
+	offset := uint64(off)
 	// maxConcurrentRequests buffer to deal with broadcastErr() floods
 	// also must have a buffer of max value of (desiredInFlight - inFlight)
 	ch := make(chan result, f.c.maxConcurrentRequests+1)
@@ -1280,8 +1299,9 @@ func (f *File) Chmod(mode os.FileMode) error {
 // that if the size is less than its current size it will be truncated to fit,
 // the SFTP protocol does not specify what behavior the server should do when setting
 // size greater than the current size.
+// We send a SSH_FXP_FSETSTAT here since we have a file handle
 func (f *File) Truncate(size int64) error {
-	return f.c.Truncate(f.path, size)
+	return f.c.setfstat(f.handle, sshFileXferAttrSize, uint64(size))
 }
 
 func min(a, b int) int {
diff --git a/request-example.go b/request-example.go
index 9f6ae0e..012c6ae 100644
--- a/request-example.go
+++ b/request-example.go
@@ -78,6 +78,13 @@ func (fs *root) Filecmd(r *Request) error {
 	defer fs.filesLock.Unlock()
 	switch r.Method {
 	case "Setstat":
+		file, err := fs.fetch(r.Filepath)
+		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)
@@ -302,6 +309,18 @@ func (f *memFile) WriteAt(p []byte, off int64) (int, error) {
 	return len(p), nil
 }
 
+func (f *memFile) Truncate(size int64) error {
+	f.contentLock.Lock()
+	defer f.contentLock.Unlock()
+	grow := size - int64(len(f.content))
+	if grow <= 0 {
+		f.content = f.content[:size]
+	} else {
+		f.content = append(f.content, make([]byte, grow)...)
+	}
+	return nil
+}
+
 func (f *memFile) TransferError(err error) {
 	f.transferError = err
 }
diff --git a/request-server.go b/request-server.go
index e9974d5..05dfa6d 100644
--- a/request-server.go
+++ b/request-server.go
@@ -214,6 +214,15 @@ func (rs *RequestServer) packetWorker(
 				request = NewRequest("Stat", request.Filepath)
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
+		case *sshFxpFsetstatPacket:
+			handle := pkt.getHandle()
+			request, ok := rs.getRequest(handle)
+			if !ok {
+				rpkt = statusFromError(pkt, syscall.EBADF)
+			} else {
+				request = NewRequest("Setstat", request.Filepath)
+				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
+			}
 		case *sshFxpExtendedPacketPosixRename:
 			request := NewRequest("Rename", pkt.Oldpath)
 			request.Target = pkt.Newpath


Test patch:
diff --git a/request-server_test.go b/request-server_test.go
index 370156f..991f36e 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var _ = fmt.Print
@@ -345,6 +346,37 @@ func TestRequestFstat(t *testing.T) {
 	checkRequestServerAllocator(t, p)
 }
 
+func TestRequestFsetstat(t *testing.T) {
+	p := clientRequestServerPair(t)
+	defer p.Close()
+	_, err := putTestFile(p.cli, "/foo", "hello")
+	require.NoError(t, err)
+	fp, err := p.cli.OpenFile("/foo", os.O_WRONLY)
+	require.NoError(t, err)
+	err = fp.Truncate(2)
+	fi, err := fp.Stat()
+	require.NoError(t, err)
+	assert.Equal(t, fi.Name(), "foo")
+	assert.Equal(t, fi.Size(), int64(2))
+	err = fp.Truncate(5)
+	require.NoError(t, err)
+	fi, err = fp.Stat()
+	require.NoError(t, err)
+	assert.Equal(t, fi.Name(), "foo")
+	assert.Equal(t, fi.Size(), int64(5))
+	err = fp.Close()
+	assert.NoError(t, err)
+	rf, err := p.cli.Open("/foo")
+	assert.NoError(t, err)
+	defer rf.Close()
+	contents := make([]byte, 20)
+	n, err := rf.Read(contents)
+	assert.EqualError(t, err, io.EOF.Error())
+	assert.Equal(t, 5, n)
+	assert.Equal(t, []byte{'h', 'e', 0, 0, 0}, contents[0:n])
+	checkRequestServerAllocator(t, p)
+}
+
 func TestRequestStatFail(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()