Properly handle io.EOF error conditions when reading

[dsnet]

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.

[dsnet]

Thanks for the review.

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!)

I'm not sure I follow. Right now, packetWorker only ever returns nil, so the err != nil check in RequestServer.Serve never gets executed. Are you saying that we should always call rs.conn.Close() regardless of the error condition?

The code you pointed out certainly seems fishy, but I think it's sufficiently different that it might belong in a different PR.

[puellanivis]

😰 Ack, you’re right, it does only ever return nil. (I was after all checking on my phone on holiday, so my brain wasn’t entirely on.)

I think we should remove the check, and always call rs.conn.Close() but it looks like this is called in https://github.com/pkg/sftp/blob/master/request-server.go#L150 which defers a close of the channel that happens before the range on that channel in packetWorker, meaning that rs.conn.Close() should never need to execute in the first place. 😩 Except that rs.conn.Close() is only ever called if there is an error in the makePacket function, not in the actual reading… so, I’m not sure either of them ever actually execute. 😔

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… 🤔

[puellanivis]

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.

[dsnet]

I'd rather keep the PR targetted to just io.ReadFull usages, if that's alright with you.