remote vs local window discrepancy

[David Wierbowski]

Damien,

I have verified that your fix works correctly in the tunnel case.  I have
not verified the non-tunnel case, but the logic looks fine to me.  It's
actually quite clever how you calculated the local_consumed value.

One related question.  The following check is in channel_check_window
(Channel *c):

    if (c->type == SSH_CHANNEL_OPEN &&
        !(c->flags & (CHAN_CLOSE_SENT|CHAN_CLOSE_RCVD)) &&
        ((c->local_window_max - c->local_window >
        c->local_maxpacket*3) ||
        c->local_window < c->local_window_max/2) &&
        c->local_consumed > 0) {

I was just curious why c->local_maxpacket*3 was chosen.  Is there any
significance to this value or was it just deemed to be a significantly
large enough value that an adjust should be sent assuming that data has
actually been consumed?

Thanks for you help.


Dave Wierbowski



From:       Damien Miller <djm at mindrot.org>
To:         David Wierbowski/Endicott/IBM at IBMUS
Cc:         openssh-unix-dev at mindrot.org
Date:       07/28/2010 09:43 PM
Subject:    Re: remote vs local window discrepancy



On Wed, 28 Jul 2010, David Wierbowski wrote:

> Damien,
>
> Your latest suggested fix worked exactly the same as  your previous fix
(at
> least in my environment).
>
> I believe both fixes calculate local_consumed such that it agrees with
the
> amount the remote side decrements remote_window.  I believe both fixes
> decrement local_window by a value that is 4 bytes less per packet than
the
> amount that local_consumed is incremented.
>
> I believe I should see a pattern of  local_consumed  being incremented as
> follows: 1508, 1508, 1508, 596 and local_window being decremented as
> follows:  1508, 1508, 1508, 596.
>
> What I am seeing is a pattern of  local_consumed  being incremented as
> follows: 1508, 1508, 1508, 596 and local_window being decremented as
> follows:  1504, 1504, 1504, 592.
>
> Just as a quick test I've made the following change in addition to your
fix
> and the counts seem to work as I would expect them (although I admit that
I
> did not check to see if remote_window was still as I expected).  In
> channel_input_data I added an extra 4 bytes to the amount that
local_window
> is decremented :

I think you change breaks window calculation for non-datagram channels,
please try this (on top of the other patch):

Index: channels.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/channels.c,v
retrieving revision 1.308
diff -u -p -r1.308 channels.c
--- channels.c           13 Jul 2010 23:13:16 -0000          1.308
+++ channels.c           29 Jul 2010 01:42:02 -0000
@@ -2235,7 +2243,7 @@ channel_input_data(int type, u_int32_t s
 {
             int id;
             char *data;
-            u_int data_len;
+            u_int data_len, win_len;
             Channel *c;

             /* Get the channel number and verify it. */
@@ -2251,6 +2259,9 @@ channel_input_data(int type, u_int32_t s

             /* Get the data. */
             data = packet_get_string_ptr(&data_len);
+            win_len = data_len;
+            if (c->datagram)
+                        win_len += 4;  /* string length header */

             /*
              * Ignore data for protocol > 1.3 if output end is no longer
open.
@@ -2261,23 +2272,23 @@ channel_input_data(int type, u_int32_t s
              */
             if (!compat13 && c->ostate != CHAN_OUTPUT_OPEN) {
                         if (compat20) {
-                                    c->local_window -= data_len;
-                                    c->local_consumed += data_len;
+                                    c->local_window -= win_len;
+                                    c->local_consumed += win_len;
                         }
                         return;
             }

             if (compat20) {
-                        if (data_len > c->local_maxpacket) {
+                        if (win_len > c->local_maxpacket) {
                                     logit("channel %d: rcvd big packet %d,
maxpack %d",
-                                        c->self, data_len, c->
local_maxpacket);
+                                        c->self, win_len, c->
local_maxpacket);
                         }
-                        if (data_len > c->local_window) {
+                        if (win_len > c->local_window) {
                                     logit("channel %d: rcvd too much data
%d, win %d",
-                                        c->self, data_len, c->
local_window);
+                                        c->self, win_len, c->
local_window);
                                     return;
                         }
-                        c->local_window -= data_len;
+                        c->local_window -= win_len;
             }
             if (c->datagram)
                         buffer_put_string(&c->output, data, data_len);