pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.49k stars 379 forks source link

Large directory ReadDir ends up with connection lost error #591

Closed tpoxa closed 1 month ago

tpoxa commented 1 month ago

I am running self-hosted sftpgo. One of the directories has 2831 other directories. Here is the code I use to read files in the directory.

  conf := &ssh.ClientConfig{}
  conf.HostKeyCallback = ssh.InsecureIgnoreHostKey()

  conf.User = "ftp_user_1"
  conf.Auth = append(conf.Auth, ssh.Password("ftp_user_1"))

  hostname := fmt.Sprintf("%s:%s", "localhost", "2022")

  conn, err := ssh.Dial("tcp", hostname, conf)
  if err != nil {
    log.Fatal("dial ", err)
  }

  client, err := sftppkg.NewClient(conn)
  if err != nil {
    log.Fatal("new client", err)
  }
  defer client.Close()

  files, err := client.ReadDir("/docker/registry/v2/repositories/asdf/library/library/gitlab")
  if err != nil {
    log.Fatal("read dir ", err)
  }
  fmt.Println(len(files))

Error

[FATAL] [main.go:33]: read dir connection lost

Filezilla lists all the files in the directory without error.

Smaller directories with 100-300 objects are being read without problem.

Any help is very appreciated.

Thanks!

puellanivis commented 1 month ago

In the if err != nil block where you’re hitting this issue, could you add a:

if errors.Is(err, sftp.ErrSSHFxConnectionLost) {
    log.Print("connection loss:", client.Wait())    
}

This should print the underlying error that caused the connection loss.

tpoxa commented 1 month ago

sure @puellanivis it is

drivers/sftp/cmd/main.go:35]: connection lost:packet too long
tpoxa commented 1 month ago

when I changed the maxMsgLength in my local fork its actually start working...

drakkan commented 1 month ago

I did a quick test and I cannot reproduce using both SFTPGo 2.5.x and 2.6.x and a directory with about 40000 files and 3000 directories

$ go run main.go
43002

If I print the received packet length in recvPacket I see this:

$ go run main.go
packet length 33
packet length 10
packet length 113958
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 114009
packet length 109805
packet length 112005
packet length 112009
packet length 345
packet length 20
packet length 17
43002

can you please share the received length in your case? Thank you

puellanivis commented 1 month ago

Huh… with such a long beginning file path, and I imagine large number of <hash>.git files (especially with the longname field being set), I bet, it’s possible that the server isn’t ensuring that it doesn’t overflow maximum packet size. But with smaller filenames it’s far less likely to throw in an over-large packet.

🤔 I might consider thinking about accepting one-off overlarge packets if they’re not drastically out of size.

drakkan commented 1 month ago

I can confirm, using filenames like this ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss01999.jpg I can reproduce the issue.

go run main.go
packet length 33
packet length 10
packet length 281454
panic: connection lost

The interesting thing is that sftp CLI fails in the same way

sftp -P 2022 a@127.0.0.1
(a@127.0.0.1) Password: 
Connected to 127.0.0.1.
sftp> ls
Received message too long 281454

FileZilla works

drakkan commented 1 month ago

This is a behaviour change in SFTPGo v2.6.0 to improve speed for listing directories with million of files. I think it should be fixed in SFTPGo and not here. It works if you use SFTPGo 2.5.x

puellanivis commented 1 month ago

Agree. Especially since the openssh sftp fails the same way.