diff --git a/client.go b/client.go index c04f1afd..16da662b 100644 --- a/client.go +++ b/client.go @@ -1559,12 +1559,23 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) { // Reduce: serialize the results from the reads into sequential writes. cur := writeCh + shortRead := false for { packet, ok := <-cur if !ok { return written, errors.New("sftp.File.WriteTo: unexpectedly closed channel") } + // The reads are dispatched at fixed offsets (off, off+chunkSize, ...), so a + // server returning fewer bytes than requested mid-stream leaves a gap that the + // following chunk cannot fill. If a short read is followed by another chunk that + // still has data, the stream can no longer be reassembled, so fail loudly rather + // than silently drop the skipped bytes. A short final chunk is fine: it is + // followed only by the EOF packet, which carries no data. + if shortRead && len(packet.b) > 0 { + return written, errors.New("sftp: server returned a short read mid-stream, cannot reassemble concurrent WriteTo") + } + // Because writes are serialized, this will always be the last successfully read byte. f.offset = packet.off + int64(len(packet.b)) @@ -1584,6 +1595,10 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) { return written, packet.err } + if len(packet.b) < chunkSize { + shortRead = true + } + pool.Put(packet.b) cur = packet.next } diff --git a/client_test.go b/client_test.go index dda8af2b..1fa2a730 100644 --- a/client_test.go +++ b/client_test.go @@ -198,3 +198,70 @@ 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. Because the concurrent path +// dispatches reads at fixed offsets, it cannot reassemble the stream across a +// mid-stream short read, so it must fail loudly rather than return a truncated +// copy that looks successful. +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 succeeded but should have reported the short read; wrote %d of %d bytes", n, len(want)) + } + // Whatever was written must be a correct prefix of the source: we may stop + // early, but we must never emit misaligned or skipped bytes. + if !bytes.Equal(buf.Bytes(), want[:buf.Len()]) { + t.Errorf("WriteTo produced %d bytes that are not a prefix of the source", buf.Len()) + } +}