- djm@cvs.openbsd.org 2006/04/16 00:48:52

     [buffer.c buffer.h channels.c]
     Fix condition where we could exit with a fatal error when an input
     buffer became too large and the remote end had advertised a big window.
     The problem was a mismatch in the backoff math between the channels code
     and the buffer code, so make a buffer_check_alloc() function that the
     channels code can use to propsectivly check whether an incremental
     allocation will succeed.  bz #1131, debugged with the assistance of
     cove AT wildpackets.com; ok dtucker@ deraadt@

diff --git a/ChangeLog b/ChangeLog
index e48dfe0e628e9529fc4ff77a8ee7e473301e6416..e0d1e28a5cf875012fd68bf417618368a5d284be 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -17,6 +17,15 @@
      GSSAPI buffers shouldn't be nul-terminated, spotted in bugzilla #1066
      by dleonard AT vintela.com. use xasprintf() to simplify code while in
      there; "looks right" deraadt@
+   - djm@cvs.openbsd.org 2006/04/16 00:48:52
+     [buffer.c buffer.h channels.c]
+     Fix condition where we could exit with a fatal error when an input
+     buffer became too large and the remote end had advertised a big window.
+     The problem was a mismatch in the backoff math between the channels code
+     and the buffer code, so make a buffer_check_alloc() function that the
+     channels code can use to propsectivly check whether an incremental
+     allocation will succeed.  bz #1131, debugged with the assistance of
+     cove AT wildpackets.com; ok dtucker@ deraadt@
 
 20060421
  - (djm) [Makefile.in configure.ac session.c sshpty.c]

diff --git a/buffer.c b/buffer.c
index de404e602453eb05f450fab972bd00651ab5b515..ba718daf2214e024f29716bc0d2f6c9112a5a53c 100644 (file)
--- a/buffer.c
+++ b/buffer.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: buffer.c,v 1.26 2006/03/25 13:17:01 djm Exp $ */
+/* $OpenBSD: buffer.c,v 1.27 2006/04/16 00:48:52 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -18,6 +18,10 @@
 #include "buffer.h"
 #include "log.h"
 
+#define        BUFFER_MAX_CHUNK        0x100000
+#define        BUFFER_MAX_LEN          0xa00000
+#define        BUFFER_ALLOCSZ          0x008000
+
 /* Initializes the buffer structure. */
 
 void
@@ -66,6 +70,23 @@ buffer_append(Buffer *buffer, const void *data, u_int len)
        memcpy(p, data, len);
 }
 
+static int
+buffer_compact(Buffer *buffer)
+{
+       /*
+        * If the buffer is quite empty, but all data is at the end, move the
+        * data to the beginning.
+        */
+       if (buffer->offset > MIN(buffer->alloc, BUFFER_MAX_CHUNK)) {
+               memmove(buffer->buf, buffer->buf + buffer->offset,
+                       buffer->end - buffer->offset);
+               buffer->end -= buffer->offset;
+               buffer->offset = 0;
+               return (1);
+       }
+       return (0);
+}
+
 /*
  * Appends space to the buffer, expanding the buffer if necessary. This does
  * not actually copy the data into the buffer, but instead returns a pointer
@@ -93,20 +114,13 @@ restart:
                buffer->end += len;
                return p;
        }
-       /*
-        * If the buffer is quite empty, but all data is at the end, move the
-        * data to the beginning and retry.
-        */
-       if (buffer->offset > MIN(buffer->alloc, BUFFER_MAX_CHUNK)) {
-               memmove(buffer->buf, buffer->buf + buffer->offset,
-                       buffer->end - buffer->offset);
-               buffer->end -= buffer->offset;
-               buffer->offset = 0;
+
+       /* Compact data back to the start of the buffer if necessary */
+       if (buffer_compact(buffer))
                goto restart;
-       }
-       /* Increase the size of the buffer and retry. */
 
-       newlen = buffer->alloc + len + 32768;
+       /* Increase the size of the buffer and retry. */
+       newlen = roundup(buffer->alloc + len, BUFFER_ALLOCSZ);
        if (newlen > BUFFER_MAX_LEN)
                fatal("buffer_append_space: alloc %u not supported",
                    newlen);
@@ -116,6 +130,27 @@ restart:
        /* NOTREACHED */
 }
 
+/*
+ * Check whether an allocation of 'len' will fit in the buffer
+ * This must follow the same math as buffer_append_space
+ */
+int
+buffer_check_alloc(Buffer *buffer, u_int len)
+{
+       if (buffer->offset == buffer->end) {
+               buffer->offset = 0;
+               buffer->end = 0;
+       }
+ restart:
+       if (buffer->end + len < buffer->alloc)
+               return (1);
+       if (buffer_compact(buffer))
+               goto restart;
+       if (roundup(buffer->alloc + len, BUFFER_ALLOCSZ) <= BUFFER_MAX_LEN)
+               return (1);
+       return (0);
+}
+
 /* Returns the number of bytes of data in the buffer. */
 
 u_int

diff --git a/buffer.h b/buffer.h
index abdaea3493d9bbf25e40ea204b9905f57b7e0226..43414ae99e5555b09bace5a4c22e3820b3dcd7cc 100644 (file)
--- a/buffer.h
+++ b/buffer.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: buffer.h,v 1.14 2006/03/25 22:22:42 djm Exp $ */
+/* $OpenBSD: buffer.h,v 1.15 2006/04/16 00:48:52 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -23,9 +23,6 @@ typedef struct {
        u_int    end;           /* Offset of last byte containing data. */
 }       Buffer;
 
-#define        BUFFER_MAX_CHUNK        0x100000
-#define        BUFFER_MAX_LEN          0xa00000
-
 void    buffer_init(Buffer *);
 void    buffer_clear(Buffer *);
 void    buffer_free(Buffer *);
@@ -36,6 +33,8 @@ void  *buffer_ptr(Buffer *);
 void    buffer_append(Buffer *, const void *, u_int);
 void   *buffer_append_space(Buffer *, u_int);
 
+int     buffer_check_alloc(Buffer *, u_int);
+
 void    buffer_get(Buffer *, void *, u_int);
 
 void    buffer_consume(Buffer *, u_int);

diff --git a/channels.c b/channels.c
index fccb9098e43cbaba5979208fa65ef61fa2877e67..2fa997edc991642ba6acbf03fff2f3d9c0bb2798 100644 (file)
--- a/channels.c
+++ b/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.249 2006/03/30 09:41:25 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.250 2006/04/16 00:48:52 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -747,12 +747,10 @@ channel_pre_open(Channel *c, fd_set *readset, fd_set *writeset)
 {
        u_int limit = compat20 ? c->remote_window : packet_get_maxsize();
 
-       /* check buffer limits */
-       limit = MIN(limit, (BUFFER_MAX_LEN - BUFFER_MAX_CHUNK - CHAN_RBUF));
-
        if (c->istate == CHAN_INPUT_OPEN &&
            limit > 0 &&
-           buffer_len(&c->input) < limit)
+           buffer_len(&c->input) < limit &&
+           buffer_check_alloc(&c->input, CHAN_RBUF))
                FD_SET(c->rfd, readset);
        if (c->ostate == CHAN_OUTPUT_OPEN ||
            c->ostate == CHAN_OUTPUT_WAIT_DRAIN) {