From 0397474ecf4110c00f72fb150ed12740f452c44e Mon Sep 17 00:00:00 2001
From: "Chris (ChrisJr404)" <11917633+ChrisJr404@users.noreply.github.com>
Date: Tue, 25 Aug 2026 19:07:12 -0400
Subject: [PATCH] fix: don't silently drop data on short reads in concurrent
 WriteTo

The concurrent File.WriteTo path (which io.Copy uses) dispatches reads at
offsets spaced chunkSize apart and advances the offset unconditionally. A
server is allowed to return fewer bytes than requested, which it does whenever
the client max packet size is larger than the server's, so every chunk kept
only its first short read and the rest was skipped. The copy then returned a
truncated file with a nil error.

Fill the remainder of a short chunk in the worker before handing it on, the
same way the sequential path already does via readChunkAt, so the pre-computed
offsets stay aligned. A genuine EOF ends the fill and is carried through as the
chunk's error. The full-read path is unchanged.

Fixes #658
---
 client.go      | 21 ++++++++++++++--
 client_test.go | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/client.go b/client.go
index c04f1afd..67cf5145 100644
--- a/client.go
+++ b/client.go
@@ -1513,7 +1513,6 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
 				var n int
 
 				s := <-readWork.res
-				resPool.Put(readWork.res)
 
 				err := s.err
 				if err == nil {
@@ -1528,8 +1527,24 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
 
 						} else {
 							l, data := unmarshalUint32(data)
-							b = pool.Get()[:l]
+							b = pool.Get()
 							n = copy(b, data[:l])
+
+							// The dispatcher already queued the next read at
+							// off+chunkSize, so a short read here (a server is
+							// allowed to return fewer bytes than requested, e.g.
+							// when it clamps to a smaller max packet size) would
+							// leave a hole. Fill the rest of the chunk before
+							// handing it on, the same way the sequential path
+							// does, or we silently drop the skipped bytes. A
+							// genuine EOF ends the fill and is carried as the
+							// chunk's error.
+							if n < chunkSize {
+								var m int
+								m, err = f.readChunkAt(readWork.res, b[n:], readWork.off+int64(n))
+								n += m
+							}
+
 							b = b[:n]
 						}
 
@@ -1538,6 +1553,8 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
 					}
 				}
 
+				resPool.Put(readWork.res)
+
 				writeWork := writeWork{
 					b:   b,
 					off: readWork.off,
diff --git a/client_test.go b/client_test.go
index dda8af2b..332a4875 100644
--- a/client_test.go
+++ b/client_test.go
@@ -198,3 +198,68 @@ func TestClientNoSid(t *testing.T) {
 		t.Fatal("expected ErrSSHFxConnectionLost, got", err)
 	}
 }
+
+// Issue #658: the concurrent File.WriteTo path (used by io.Copy) must not
+// silently drop data when the server returns short reads. A server is free to
+// return fewer bytes than asked for, which it will whenever the client's max
+// packet size is larger than the server's.
+func TestClientWriteToShortReads(t *testing.T) {
+	cr, sw := io.Pipe()
+	sr, cw := io.Pipe()
+
+	// The default server max packet size is 32768, so a bigger client packet
+	// size makes every read come back short.
+	server, err := NewServer(struct {
+		io.Reader
+		io.WriteCloser
+	}{sr, sw})
+	if err != nil {
+		t.Fatal(err)
+	}
+	go server.Serve()
+
+	client, err := NewClientPipe(cr, cw, MaxPacketUnchecked(128*1024))
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Close the client first (LIFO), so its receive loop sees the server go away.
+	defer client.Close()
+	defer server.Close()
+
+	// Bigger than the client packet size so WriteTo takes the concurrent path,
+	// and not a multiple of it so the last chunk is partial as well.
+	want := make([]byte, 5*128*1024+123)
+	for i := range want {
+		want[i] = byte(i)
+	}
+
+	tmp, err := os.CreateTemp("", "sftp-writeto-shortread")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(tmp.Name())
+	if _, err := tmp.Write(want); err != nil {
+		t.Fatal(err)
+	}
+	if err := tmp.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	f, err := client.Open(tmp.Name())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	var buf bytes.Buffer
+	n, err := f.WriteTo(&buf)
+	if err != nil {
+		t.Fatalf("WriteTo: %v", err)
+	}
+	if n != int64(len(want)) {
+		t.Errorf("WriteTo returned %d, want %d", n, len(want))
+	}
+	if !bytes.Equal(buf.Bytes(), want) {
+		t.Errorf("WriteTo produced %d bytes that do not match the source (want %d)", buf.Len(), len(want))
+	}
+}