- 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 360cf61..d3abc2b 100644
--- 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]
@@ -4528,4 +4537,4 @@
- (djm) Trim deprecated options from INSTALL. Mention UsePAM
- (djm) Fix quote handling in sftp; Patch from admorten AT umich.edu
-$Id: ChangeLog,v 1.4306 2006/04/23 02:05:46 djm Exp $
+$Id: ChangeLog,v 1.4307 2006/04/23 02:06:03 djm Exp $
diff --git a/buffer.c b/buffer.c
index de404e6..ba718da 100644
--- 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 @@
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 @@
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;
- goto restart;
- }
- /* Increase the size of the buffer and retry. */
- newlen = buffer->alloc + len + 32768;
+ /* 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 = roundup(buffer->alloc + len, BUFFER_ALLOCSZ);
if (newlen > BUFFER_MAX_LEN)
fatal("buffer_append_space: alloc %u not supported",
newlen);
@@ -116,6 +130,27 @@
/* 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 abdaea3..43414ae 100644
--- 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 @@
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_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 fccb909..2fa997e 100644
--- 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 @@
{
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) {