Properly handle io.EOF error conditions when reading #554
Conversation
If the connection provided to sftp.NewServer is closed, Serve returns the io.EOF error verbatim from io.Reader.Read. This is an odd error since this is an expected situation, so we manually ignore io.EOF. This is somewhat buggy since the sftp package itself incorrectly reports io.EOF in cases where it should actually be reporting io.ErrUnexpectedEOF. See pkg/sftp#554 which patches Serve to return nil on clean closes and fixes buggy uses of io.ReadFull. Fixes #8592 Signed-off-by: Joe Tsai <joetsai@digital-static.net>
If the connection provided to sftp.NewServer is closed, Serve returns the io.EOF error verbatim from io.Reader.Read. This is an odd error since this is an expected situation, so we manually ignore io.EOF. This is somewhat buggy since the sftp package itself incorrectly reports io.EOF in cases where it should actually be reporting io.ErrUnexpectedEOF. See pkg/sftp#554 which patches Serve to return nil on clean closes and fixes buggy uses of io.ReadFull. Fixes #8592 Signed-off-by: Joe Tsai <joetsai@digital-static.net>
If the connection provided to sftp.NewServer is closed, Serve returns the io.EOF error verbatim from io.Reader.Read. This is an odd error since this is an expected situation, so we manually ignore io.EOF. This is somewhat buggy since the sftp package itself incorrectly reports io.EOF in cases where it should actually be reporting io.ErrUnexpectedEOF. See pkg/sftp#554 which patches Serve to return nil on clean closes and fixes buggy uses of io.ReadFull. Fixes #8592 Signed-off-by: Joe Tsai <joetsai@digital-static.net> (cherry picked from commit bb4b35e)
If the connection provided to sftp.NewServer is closed, Serve returns the io.EOF error verbatim from io.Reader.Read. This is an odd error since this is an expected situation, so we manually ignore io.EOF. This is somewhat buggy since the sftp package itself incorrectly reports io.EOF in cases where it should actually be reporting io.ErrUnexpectedEOF. See pkg/sftp#554 which patches Serve to return nil on clean closes and fixes buggy uses of io.ReadFull. Fixes #8592 Signed-off-by: Joe Tsai <joetsai@digital-static.net> (cherry picked from commit bb4b35e)
There was a problem hiding this comment.
While looking for other cases of code that might also be affected the same way: Hm… we’re not bubbling up the error in https://github.com/pkg/sftp/blob/v1.13.5/request-server.go#L175-L177 RequestServer.Serve … and actually, it looks like there’s a “bug” in that if packetWorker ever returns err == nil we would close the connection, even though we should. (Considering there would no longer be any goroutine watching the connection!)
Would you mind wrapping that into this PR as well?
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.
|
Thanks for the review.
I'm not sure I follow. Right now, The code you pointed out certainly seems fishy, but I think it's sufficiently different that it might belong in a different PR. |
|
😰 Ack, you’re right, it does only ever return I think we should remove the check, and always call I think it might need quite a bit of logic examination and correction, which as you say, might be better in a separate PR. I’m just a little wary of letting it stand there in this likely broken state… 🤔 |
|
OK, let me know if you want to pick up the RequestServer changes, but since it looks like the behavior won’t change despite the changes you’re making, if you don’t want to pick them up, I’ll just go ahead and merge anyways. |
|
I'd rather keep the PR targetted to just |
If the connection provided to sftp.NewServer is closed, Serve returns the io.EOF error verbatim from io.Reader.Read. This is an odd error since this is an expected situation, so we manually ignore io.EOF. This is somewhat buggy since the sftp package itself incorrectly reports io.EOF in cases where it should actually be reporting io.ErrUnexpectedEOF. See pkg/sftp#554 which patches Serve to return nil on clean closes and fixes buggy uses of io.ReadFull. Fixes tailscale#8592 Signed-off-by: Joe Tsai <joetsai@digital-static.net> Signed-off-by: Alex Paguis <alex@windscribe.com>
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.