Previously, the Server.Serve method would never return nil,
because the infinite for-loop handling request packets would
only break if reading a packet reported an error.
A common termination condition is when the underlying connection
is closed and recvPacket returns io.EOF.
In which case Serve should ignore io.EOF and
treat it as a normal shutdown.
However, this means that recvPacket must correctly handle io.EOF
such that it never reports io.EOF if a packet is partially read.
There are two calls to io.ReadFull in recvPacket.
The first call correctly forwards an io.EOF error
if no additional bytes of the next packet are read.
However, the second call incorrectly forwards io.EOF
when no bytes of the payload could be read.
This is incorrect since we already read the length and
should convert the io.EOF into an io.ErrUnexpectedEOF.
after processing a packet we keep in memory the allocated slices and we reuse
them for new packets.
Slices are allocated in:
- recvPacket
- when we receive a sshFxpReadPacket (downloads)
The allocated slices have a fixed size = maxMsgLength.
Allocated slices are referenced to the request order id and are marked for reuse
after a request is served in maybeSendPackets.
The allocator is added to the packetManager struct and it is cleaned at the end
of the Serve() function.
This allocation mode is optional and disabled by default
Need to prevent Close from being called at the same time. The lock
already existed, but was only used in one place where it should be
applied generally.
It is possible for dispatchRequest and broadcastErr to both send a
result down the inflight[] channel which has a buffer of 1, so would
work for one of them then block on the other. If broadcastErr went
first, then dispatchRequest would block causing the client to hang.
After commit 506f3a7 which removed clientConn mutex around
conn.sendPacket, it is now possible for conn.Close and conn.Write to
race (https://travis-ci.org/pkg/sftp/jobs/219048782#L1838). The problem
is really hard to reproduce, but I believe this patch fixes it.
Now clientConn.Mutex protects 'inflight' map only, and conn.Mutex
serializes connection writes/close.
Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
Client + Server might deadlock while being connected via io.Pipe with
high request concurrency. The scenario is as follows:
- Server is trying to send reply to a previous client's packet and is
waiting on Pipe's condvar for the client to read it:
3 0x0000000000460e39 in sync.(*Cond).Wait
at /usr/lib/go/src/sync/cond.go:57
4 0x0000000000464519 in io.(*pipe).write
at /usr/lib/go/src/io/pipe.go:90
5 0x0000000000464a4c in io.(*PipeWriter).Write
at /usr/lib/go/src/io/pipe.go:157
6 0x00000000007a0b66 in go.(*struct { io.Reader; io.WriteCloser }).Write
at <autogenerated>:246
7 0x0000000000790cf6 in github.com/pkg/sftp.(*conn).Write
at <autogenerated>:3
8 0x000000000073a68d in github.com/pkg/sftp.sendPacket
at ./packet.go:130
9 0x000000000073338c in github.com/pkg/sftp.(*conn).sendPacket
at ./conn.go:31
10 0x00000000007374b4 in github.com/pkg/sftp.(*packetManager).maybeSendPackets
at ./packet-manager.go:87
- Client is sending a new packet and is waiting on Pipe's condvar for
the server to read it, while holding clientConn mutex:
5 0x0000000000464a4c in io.(*PipeWriter).Write
at /usr/lib/go/src/io/pipe.go:157
6 0x0000000000790cf6 in github.com/pkg/sftp.(*conn).Write
at <autogenerated>:3
7 0x000000000073a68d in github.com/pkg/sftp.sendPacket
at ./packet.go:130
8 0x000000000073338c in github.com/pkg/sftp.(*conn).sendPacket
at ./conn.go:31
9 0x0000000000733bfb in github.com/pkg/sftp.(*clientConn).dispatchRequest
at ./conn.go:105
10 0x0000000000733a0b in github.com/pkg/sftp.(*clientConn).sendPacket
at ./conn.go:97
- Client's receive loop is processing a reply from the server and is
trying to acquire clientConn mutex:
4 0x000000000046108d in sync.(*Mutex).Lock
at /usr/lib/go/src/sync/mutex.go:87
5 0x00000000007336cb in github.com/pkg/sftp.(*clientConn).recv
at ./conn.go:69
6 0x000000000073350d in github.com/pkg/sftp.(*clientConn).loop
at ./conn.go:49
7 0x000000000045ab51 in runtime.goexit
at /usr/lib/go/src/runtime/asm_amd64.s:2197
In this scenario neither server nor client can progress. Fix this by
releasing clienConn mutex *before* trying to send the packet.
Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
github issue #145
A deadlock will occur in cases when using client.Write or client.ReadFrom,
it has 2 network issues in a row, and it is starting a new loop to send
more concurrent packets.
Reading and Writing are less coupled in the sftp client and server than
they may be over a traditional ReadWriter. This mirrors the full duplex
/ half close nature of a TCP connection.
In essence conn.Reader cannot be closed locally, it can only close in
response to the remote side using conn.WriteCloser.Close(), or a network
error, which is effectively the same thing.
Make this behaviour super clear by no longer smooshing the type into a
ReadWriteCloser.
Cassondra Foesch
Erik Unger
Joe Tsai
Sebastien Rosset (serosset)
Nicola Murino
greatroar
John Eikenberry
Taizeng Wu
Pavel Borzenkov
Dave Cheney