Channel.window_size can overflow on add

[jgrund]

Consider the following field:

https://github.com/warp-tech/russh/blob/49bf9599637d492f0aa17377b9c66cf953cb0824/russh/src/channels.rs#L113

This field is meant to indicate current window_size on the Channel.

However, I've been able to hit a case where the u32 overflows on add, which doesn't seem correct. Here's the scenario:

1. Push a large amount of data to an OpenSSH server
2. Enter the send_data loop of the channel
3. Eventually the window_size will become 0. When it does, we enter this loop to wait for an adjustment and reset the window_size.
4. This continues until the data has all been pushed.
5. After data returns, call Channel.wait in a loop waiting for ChannelMsg::Data to come through from the server.

The issue I am seeing is that OpenSSH has sent a large amount of ChannelMsg::WindowAdjusted messages during step 4.
send_data, does not attempt to process these, it only takes the first one it finds and leaves the rest in the channel.
While waiting for ChannelMsg::Data in step 5, the remaining ChannelMsg::WindowAdjusted are read out of the channel first.

Each CHANNEL_WINDOW_ADJUST coming from the server first goes through here:

https://github.com/warp-tech/russh/blob/49bf9599637d492f0aa17377b9c66cf953cb0824/russh/src/client/encrypted.rs#L489-L505

This code takes the existing Channel.recipient_window_size and adds the new adjustment amount to it. This resulting value is then passed into window_adjusted where it is used to build a new ChannelMsg::WindowAdjusted here:

https://github.com/warp-tech/russh/blob/49bf9599637d492f0aa17377b9c66cf953cb0824/russh/src/client/mod.rs#L1450-L1465

Finally, the wait method handles these messages by adding the entire Channel.recipient_window_size to the existing Channel.window_size here:

https://github.com/warp-tech/russh/blob/49bf9599637d492f0aa17377b9c66cf953cb0824/russh/src/channels.rs#L347-L356

Repeatedly adding the Channel.recipient_window_size to the Channel.window_size can overflow the u32 and cause a panic (in debug mode), or an two's complement wrapping (in release mode). I'm wondering if instead of self.window_size += new_size, we should do self.window_size = new_size since we are really passing in the value of Channel.recipient_window_size instead of some amount of byte adjustment from CHANNEL_WINDOW_ADJUST.

[jgrund]

Btw, I can view this issue when running with a single-threaded runtime. I think multithreading masks the issue.

[Eugeny]

You're absolutely correct - it was supposed to be an assignment!

[jgrund]

Thanks!

I've been looking through this a bit and I think there is at least another issue:

ChannelMsg::WindowAdjusted is pushed here:

https://github.com/warp-tech/russh/blob/f08f749b8a49e0fbef8c4baf6f8d7731ff1ac96c/russh/src/client/mod.rs#L1461

And only drained in these places:

https://github.com/warp-tech/russh/blob/f08f749b8a49e0fbef8c4baf6f8d7731ff1ac96c/russh/src/channels.rs#L291-L295

https://github.com/warp-tech/russh/blob/f08f749b8a49e0fbef8c4baf6f8d7731ff1ac96c/russh/src/channels.rs#L348-L355

In both of these spots, we drain a single ChannelMsg::WindowAdjusted message and return.

In an OpenSSH server, SSH_MSG_CHANNEL_WINDOW_ADJUST messages start getting pushed when available window size is half the total window size.

Due to this, many (hundreds to thousands depending on file size) of ChannelMsg::WindowAdjusted end up being queued and potentially not drained.

Since we only care about the last ChannelMsg::WindowAdjusted (due to it just being an assignment to window_size), it seems like we should demux these from other ChannelMsg variants and perhaps place them in their own tokio::sync::watch channel. This way we don't store many unnecessary messages when we only care about the last one.

[Eugeny]

Not sure I understand this correctly - almost always, the WINDOW_ADJUST messages are passed down to the Channel.

Only in the first case, we're unconditionally waiting for a new window size when it's 0 (it might make sense to push this one to the Channel ^{[citation needed]}). In the second case, it's just looked at and is still passed through to the Channel.

It's important to note that looping over Channel::wait is actually a requirement - if nothing is awaiting messages from a channel's queue, the session will just lock up very soon as it's using a bounded channel.

[jgrund]

It's important to note that looping over Channel::wait is actually a requirement - if nothing is awaiting messages from a channel's queue, the session will just lock up very soon as it's using a bounded channel.

Right, this is basically my point (though it is using an unbounded channel rather than a bounded one).

Refer to the following example:

https://github.com/warp-tech/russh/blob/f08f749b8a49e0fbef8c4baf6f8d7731ff1ac96c/russh/examples/client.rs#L55-L69

Channel::data is called first and waits until all data is sent. It's during this part that we may buffer many ChannelMsg::WindowAdjusted messages, as we only drain a single one at a time when window_size reaches 0.

Channel::wait will eventually drain the rest after Channel::data completes, but at this point there could be a large number of ChannelMsg::WindowAdjusted messages buffered in the channel depending on the data size being transferred and the behavior of the server during transfer.

[Eugeny]

Right - I think I finally get it - however the WindowAdjusted messages are not special here - anything else (e.g. incoming Data messages) would have to be queued up as well until the write completes - since a Channel can only either be read or written at any given time.

Do you think there's any value at all in exposing the WindowAdjusted messages to the user given that they're only handled internally anyway? If not, we could just remove ChannelMsg::WindowAdjusted as an immediate relief for this one specirfic case.