tomaswolf commented on PR #259: URL: https://github.com/apache/mina-sshd/pull/259#issuecomment-1293505344
> it's possible that we buffer a lot data if the peer (by mistake or malicious intent) ignores our window and just keeps on sending data A simple case of this is in TCP/IP port forwarding. Previous code would read from the channel, then write to the socket via Nio2Session.writeBuffer(), and call Window.consumeAndCheck() once the buffer _was_ written. (Nio2Session.writeBuffer() queues up a buffer for writing; it may get written only later, asynchronously.) Now consider the simple case where whether is connected on that socket doesn't read. (Let's ignore the write timeout for the moment.) If the peer just sends data, we will queue up all that data in Nio2Session without ever decrementing our local window. With a peer that does respect our window (decrements its remote window properly, and doesn't send any more data when it hits zero) we may end up with a full channel window buffered in Nio2Session. With a peer that doesn't respect our window, we'll just keep on buffering until OOME. Nio2Session does have a write timeout; its default value is 30 seconds. So we'd buffer as much data as the peer sends within that time, before we'd get a time-out on a socket write, which would eventually close the channel. Depending on throughput on the tunnel that may still be a lot of data, and an OOME is still possible, especially if this happens on several channels concurrently. This PR changes this: we decrement our local window right away when we receive data from the channel. Additionally, we also check that we didn't exhaust our local window and if we just did, we close. The peer evidently didn't respect our window. [RFC 4254](https://www.rfc-editor.org/rfc/rfc4254#section-5.2) is unclear about this case. It does not mandate shutting down, but it does say > Both parties MAY ignore all extra data sent after the allowed window is empty. [RFC 8308](https://www.rfc-editor.org/rfc/rfc8308#section-3.3.1) does point out that a disconnection may occur. Personally I prefer that over dropping some packets, which may have unknown effects that might even go unnoticed. OpenSSH drops such packets, see [channels.c](https://github.com/openssh/openssh-portable/blob/445363433ba/channels.c#L3246), lines 3246 and 3302. Checking the local window size and sending back a window adjustment is still done as before once the received data has been handled or forwarded. This implements SSH flow control even if the peer chooses to ignore it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
