[tpop3d-discuss] ssl Bug
Chris Lightfoot
chris at ex-parrot.com
Wed, 5 Nov 2003 18:38:50 +0000
--DocE+STaALJfprDB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Can you try the attached patch (not yet in CVS). I believe
I've found the problem.
tpop3d uses ring buffers to avoid blocking reads and
writes. This is fine for reading, and for writing over an
unsecured TCP connection. But under TLS, when a call to
SSL_write (analogous to write(2)) returns a code
indicating that the write would block, it's necessary that
the next call to SSL_write is passed exactly the same
buffer, or an extended buffer.
The bug occurs when a call to SSL_write would block, and
the ring buffer is arranged so that the data to write
would get split across the end of the ring buffer:
<-------- A -------->
XXXXXXXXXXXXXXXXXXXXX <-- original data to write
try immediate write:
SSL_write(..., buf, A) fails, would block
save contents of buffer in ring buffer
<--- C --> <---- B ---->
,--------------------------------------------.
|XXXXXXXXX XXXXXXXXXXXX|
`--------------------------------------------'
^ ^ beginning of data
SSL_write(..., ringbuf + ..., B)
fails, bad write retry
The attached patch makes the data in the ring buffer
contiguous. I haven't tested it much but it seems to do
the right thing.
--
``It is certainly going to deter the casual bomber.''
(BBC correspondent Frank Gardner on the emplacement
of concrete blocks around the Palace of Westminster)
--DocE+STaALJfprDB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="tls-fix-candidate.patch"
Index: buffer.c
===================================================================
RCS file: /home/chris/vcvs/repos/tpop3d/buffer.c,v
retrieving revision 1.6
diff -u -r1.6 buffer.c
--- buffer.c 6 Jan 2003 19:01:17 -0000 1.6
+++ buffer.c 5 Nov 2003 18:30:14 -0000
@@ -12,7 +12,9 @@
#include <sys/types.h>
#include <assert.h>
+#include <limits.h>
#include <stdlib.h>
+#include <string.h>
#include "buffer.h"
#include "util.h"
@@ -34,6 +36,38 @@
xfree(B);
}
+/* buffer_make_contiguous BUFFER
+ * Makes the available data in BUFFER contiguous, so that it can be returned
+ * by a single call to buffer_get_consume_ptr. */
+void buffer_make_contiguous(buffer B) {
+ size_t a;
+ char *newbuf;
+
+ assert(B);
+
+ a = buffer_available(B);
+ if (!a || (size_t)B->get + a <= B->len)
+ /* Nothing to do. */
+ return;
+
+ /*
+ * v (get + a) % len
+ * ,---------------------------------------------.
+ * |XXXXXXXXXXXXXXXXX| |XXXXXXXXXXX|
+ * | (get + a) % len | | len - get |
+ * |XXXXXXXXXXXXXXXXX| |XXXXXXXXXXX|
+ * `---------------------------------------------'
+ * ^ 0 ^ get
+ */
+ newbuf = xmalloc(B->len);
+ memcpy(newbuf, B->buf + B->get, B->len - B->get);
+ memcpy(newbuf + B->len - B->get, B->buf, (B->get + a) % B->len);
+ xfree(B->buf);
+ B->buf = newbuf;
+ B->get = 0;
+ B->put = a;
+}
+
/* buffer_get_consume_ptr BUFFER SLEN
* Consume some available data in BUFFER, returning a pointer to the data and
* recording the number of bytes consumed in SLEN. This may be less than the
@@ -50,7 +84,7 @@
*slen = 0;
return NULL;
}
- if (B->get + a > B->len)
+ if ((size_t)B->get + a > B->len)
a = B->len - B->get;
p = B->buf + B->get;
*slen = a;
@@ -101,27 +135,30 @@
* This uses a Boyer-Moore search, but we can't just reuse memstr because we
* may have to search across the end of the buffer. */
char *buffer_consume_to_mark(buffer B, const char *mark, const size_t mlen, char *str, size_t *slen) {
- int skip[256], k;
- size_t a;
+ size_t skip[256], a;
+ int k;
assert(B);
+ assert(mlen > 0 && mlen <= (size_t)INT_MAX);
if ((a = buffer_available(B)) < mlen) return NULL;
+ assert(a <= (size_t)INT_MAX);
+
/* Oh dear. Should special-case the mlen == 1 case, since it's the only
* one we use.... */
for (k = 0; k < 256; ++k) skip[k] = mlen;
- for (k = 0; k < mlen - 1; ++k) skip[(unsigned char)mark[k]] = mlen - k - 1;
+ for (k = 0; k < (int)mlen - 1; ++k) skip[(unsigned char)mark[k]] = mlen - k - 1;
- for (k = mlen - 1; k < a; k += skip[(unsigned char)mark[k]]) {
+ for (k = (int)mlen - 1; k < (int)a; k += skip[(unsigned char)mark[k]]) {
int i, j;
- for (j = mlen - 1, i = k; j >= 0 && B->buf[(B->get + i) % B->len] == mark[j]; j--) i--;
+ for (j = (int)mlen - 1, i = k; j >= 0 && B->buf[(B->get + i) % B->len] == mark[j]; j--) i--;
if (j == -1) {
/* Have found the mark at location i + 1. */
i += 1 + mlen; /* account for mark and terminating null */
- if (!str || *slen < i + 1)
- str = xrealloc(str, i + 1);
- *slen = i + 1;
+ if (!str || *slen < (size_t)i + 1)
+ str = xrealloc(str, (size_t)i + 1);
+ *slen = (size_t)i + 1;
for (j = 0; j < i; ++j)
str[j] = B->buf[(B->get + j) % B->len];
str[j] = 0;
@@ -150,7 +187,7 @@
B->buf = newbuf;
B->len = newlen;
B->get = 0;
- B->put = a;
+ B->put = (off_t)a;
}
}
Index: buffer.h
===================================================================
RCS file: /home/chris/vcvs/repos/tpop3d/buffer.h,v
retrieving revision 1.2
diff -u -r1.2 buffer.h
--- buffer.h 10 Nov 2002 02:34:27 -0000 1.2
+++ buffer.h 5 Nov 2003 18:30:14 -0000
@@ -21,11 +21,12 @@
/* buffer_available BUFFER
* Return the number of bytes of data available to consume from BUFFER. This
* is a macro which may evaluate BUFFER more than once. */
-#define buffer_available(B) (((B)->put + (B)->len - (B)->get) % (B)->len)
+#define buffer_available(B) ((size_t)(((B)->put + (B)->len - (B)->get) % (B)->len))
/* buffer.c */
buffer buffer_new(const size_t len);
void buffer_delete(buffer B);
+void buffer_make_contiguous(buffer B);
char *buffer_get_consume_ptr(buffer B, size_t *slen);
void buffer_consume_bytes(buffer B, const size_t num);
char *buffer_consume_all(buffer B, char *str, size_t *slen);
Index: ioabs_tls.c
===================================================================
RCS file: /home/chris/vcvs/repos/tpop3d/ioabs_tls.c,v
retrieving revision 1.12
diff -u -r1.12 ioabs_tls.c
--- ioabs_tls.c 30 Dec 2002 16:22:45 -0000 1.12
+++ ioabs_tls.c 5 Nov 2003 18:30:14 -0000
@@ -152,6 +152,23 @@
return IOABS_ERROR;
}
+static void dump(const char *s, size_t l) {
+ const char *p;
+ fprintf(stderr, ">>>");
+ for (p = s; p < s + l; ++p) {
+ if (*p < 32)
+ switch(*p) {
+ case '\t': fprintf(stderr, "\\t"); break;
+ case '\r': fprintf(stderr, "\\r"); break;
+ case '\n': fprintf(stderr, "\\n"); break;
+ default: fprintf(stderr, "\\x%02x", *p);
+ }
+ else fprintf(stderr, "%c", (int)*p);
+ }
+ fprintf(stderr, "<<<\n");
+}
+
+
/* ioabs_tls_immediate_write CONNECTION BUFFER COUNT
* Attempt to write COUNT bytes from BUFFER to CONNECTION. Returns the number
* of bytes written on success, IOABS_WOULDBLOCK if the write would block, and
@@ -160,8 +177,8 @@
static ssize_t ioabs_tls_immediate_write(connection c, const void *buf, size_t count) {
int n, e;
struct ioabs_tls *io;
- io = (struct ioabs_tls*)c->io;
+ io = (struct ioabs_tls*)c->io;
if (count == 0) return 0; /* otherwise can't distinguish this case... */
if (io->read_blocked_on_write) return IOABS_WOULDBLOCK;
@@ -176,6 +193,7 @@
}
e = ERR_get_error();
+
switch (SSL_get_error(io->ssl, n)) {
case SSL_ERROR_WANT_READ:
io->write_blocked_on_read = 1;
@@ -228,7 +246,7 @@
* Post-select handling for TLS, with its complicated logic. */
static int ioabs_tls_post_select(connection c, fd_set *readfds, fd_set *writefds, fd_set *exceptfds) {
int ret = 0, R = 0;
- ssize_t n;
+ ssize_t n, wtotal;
int canread, canwrite;
struct ioabs_tls *io;
io = (struct ioabs_tls*)c->io;
@@ -305,9 +323,11 @@
}
/* Write from the buffer to the connection, if necessary. */
- if (((!io->write_blocked_on_read && !io->read_blocked_on_write && canwrite) || (io->write_blocked_on_read && canread)) && buffer_available(c->wrb) > 0) {
+ if (((!io->write_blocked_on_read && !io->read_blocked_on_write && canwrite)
+ || (io->write_blocked_on_read && canread))
+ && (wtotal = buffer_available(c->wrb)) > 0) {
io->write_blocked_on_read = 0;
- n = 1;
+ buffer_make_contiguous(c->wrb);
do {
char *w;
size_t wlen;
--DocE+STaALJfprDB--