remote vs local window discrepancy

[David Wierbowski]

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 :


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

It's not jumping out at me as to why the local_window counter seems to be
off by 4 bytes.  Hopefully it will make more sense to you :>).

In a case like this, how do we maintain compatibility when one side of the
SSH channel has a fix to this issue and the other side does not have the
fix?

Thanks

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/27/2010 09:17 PM
Subject:    Re: remote vs local window discrepancy



On Tue, 27 Jul 2010, David Wierbowski wrote:

> Damien,
>
> I tried the change below.  This appears to take care of the issue with
the
> remote_window being decremented by a value that is off by 4.
> Unfortunately, now the local_window is growing by a value of 4,  It seems
> that local_window is being decremented by 4 bytes less than
local_consumed.

Ok, please remove the previous diff and try this one. It changes the way
that local_consumed is calculated to be more simple and robust against
fiddling with lengths inside channel_handle_wrd()

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           28 Jul 2010 01:14:18 -0000
@@ -1632,13 +1632,14 @@ channel_handle_wfd(Channel *c, fd_set *r
 {
             struct termios tio;
             u_char *data = NULL, *buf;
-            u_int dlen;
+            u_int dlen, olen = 0;
             int len;

             /* Send buffered output data to the socket. */
             if (c->wfd != -1 &&
                 FD_ISSET(c->wfd, writeset) &&
                 buffer_len(&c->output) > 0) {
+                        olen = buffer_len(&c->output);
                         if (c->output_filter != NULL) {
                                     if ((buf = c->output_filter(c, &data,
&dlen)) == NULL) {
                                                 debug2("channel %d: filter
stops", c->self);
@@ -1657,7 +1658,6 @@ channel_handle_wfd(Channel *c, fd_set *r

                         if (c->datagram) {
                                     /* ignore truncated writes, datagrams
might get lost */
-                                    c->local_consumed += dlen + 4;
                                     len = write(c->wfd, buf, dlen);
                                     xfree(data);
                                     if (len < 0 && (errno == EINTR ||
errno == EAGAIN))
@@ -1669,7 +1669,7 @@ channel_handle_wfd(Channel *c, fd_set *r

chan_write_failed(c);
                                                 return -1;
                                     }
-                                    return 1;
+                                    goto out;
                         }

                         len = write(c->wfd, buf, dlen);
@@ -1703,10 +1703,10 @@ channel_handle_wfd(Channel *c, fd_set *r
                                     }
                         }
                         buffer_consume(&c->output, len);
-                        if (compat20 && len > 0) {
-                                    c->local_consumed += len;
-                        }
             }
+ out:
+            if (compat20 && olen > 0)
+                        c->local_consumed += olen - buffer_len(&c->
output);
             return 1;
 }

@@ -2150,6 +2150,14 @@ channel_output_poll(void)

                                                             data =
buffer_get_string(&c->input,
                                                                 &dlen);
+                                                            if (dlen > c->
remote_window ||
+                                                                dlen > c->
remote_maxpacket) {
+
debug("channel %d: datagram "
+
"too big for channel",
+
c->self);
+
xfree(data);
+
continue;
+                                                            }
                                                             packet_start
(SSH2_MSG_CHANNEL_DATA);
                                                             packet_put_int
(c->remote_id);

packet_put_string(data, dlen);