From b62d379f9274c219615520d6805a4fa985970faf Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 11 Oct 2007 20:45:26 +0000 Subject: [PATCH] r15689@catbus: nickm | 2007-10-11 16:40:25 -0400 Fix bug 451. This was a nasty bug, so let's fix it twice: first, by banning recursive calls to connection_handle_write from connection_flushed_some; and second, by not calling connection_finished_flushing() on a closed connection. Backport candidate. svn:r11882 --- ChangeLog | 12 ++++++++++++ src/or/connection.c | 44 +++++++++++++++++++++++++++++++++++++------- src/or/or.h | 9 ++++++--- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/ChangeLog b/ChangeLog index 63b61b96e1..7d46feaff7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -35,6 +35,18 @@ Changes in version 0.2.0.8-alpha - 2007-10-12 string which usually wasn't there, once for every routerinfo we read, just scan lines forward until we find one we like. Bugfix on 0.2.0.1. + - When we add data to a write buffer in response to the data on that + write buffer getting low because of a flush, do not consider the newly + added data as a candidate for immediate flushing, but rather make it + wait until the next round of writing. Otherwise, we flush and refill + recursively, and a single greedy TLS connection can eat all of our + bandwidth. Bugfix on 0.1.2.7-alpha. + + o Major bugfixes (crashes): + - If a connection is shut down abruptly because of something that + happened inside connection_flushed_some(), do not call + connection_finished_flushing(). Should fix bug 451. Bugfix on + 0.1.2.7-alpha. o Minor features (v3 authority system): - Add more ways for tools to download the votes that lead to the diff --git a/src/or/connection.c b/src/or/connection.c index aeccb49667..fefef9e9bb 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -529,6 +529,11 @@ connection_about_to_close_connection(connection_t *conn) } } +/** Return true iff connection_close_immediate has been called on this + * connection */ +#define CONN_IS_CLOSED(c) \ + ((c)->linked ? ((c)->linked_conn_is_closed) : ((c)->s < 0)) + /** Close the underlying socket for conn, so we don't try to * flush it. Must be used in conjunction with (right before) * connection_mark_for_close(). @@ -537,7 +542,7 @@ void connection_close_immediate(connection_t *conn) { assert_connection_ok(conn,0); - if (conn->s < 0 && !conn->linked) { + if (CONN_IS_CLOSED(conn)) { log_err(LD_BUG,"Attempt to close already-closed connection."); tor_fragile_assert(); return; @@ -554,6 +559,8 @@ connection_close_immediate(connection_t *conn) if (conn->s >= 0) tor_close_socket(conn->s); conn->s = -1; + if (conn->linked) + conn->linked_conn_is_closed = 1; if (!connection_is_listener(conn)) { buf_clear(conn->outbuf); conn->outbuf_flushlen = 0; @@ -2046,6 +2053,11 @@ connection_handle_write(connection_t *conn, int force) if (conn->marked_for_close || conn->s < 0) return 0; /* do nothing */ + if (conn->in_flushed_some) { + log_warn(LD_BUG, "called recursively from inside conn->in_flushed_some()"); + return 0; + } + conn->timestamp_lastwritten = now; /* Sometimes, "writable" means "connected". */ @@ -2207,6 +2219,7 @@ void _connection_write_to_buf_impl(const char *string, size_t len, connection_t *conn, int zlib) { + /* XXXX This function really needs to return -1 on failure. */ int r; size_t old_datalen; if (!len) @@ -2248,9 +2261,18 @@ _connection_write_to_buf_impl(const char *string, size_t len, int extra = 0; conn->outbuf_flushlen += len; + /* Should we try flushing the outbuf now? */ + if (conn->in_flushed_some) { + /* Don't flush the outbuf when the reason we're writing more stuff is + * _because_ we flushed the outbuf. That's unfair. */ + return; + } + if (conn->type == CONN_TYPE_OR && conn->outbuf_flushlen-len < MIN_TLS_FLUSHLEN && conn->outbuf_flushlen >= MIN_TLS_FLUSHLEN) { + /* We just pushed outbuf_flushelen to MIN_TLS_FLUSHLEN or above; + * we can send out a full TLS frame now if we like. */ extra = conn->outbuf_flushlen - MIN_TLS_FLUSHLEN; conn->outbuf_flushlen = MIN_TLS_FLUSHLEN; } else if (conn->type == CONN_TYPE_CONTROL && @@ -2625,13 +2647,17 @@ connection_process_inbuf(connection_t *conn, int package_partial) static int connection_flushed_some(connection_t *conn) { + int r = 0; + tor_assert(!conn->in_flushed_some); + conn->in_flushed_some = 1; if (conn->type == CONN_TYPE_DIR && - conn->state == DIR_CONN_STATE_SERVER_WRITING) - return connection_dirserv_flushed_some(TO_DIR_CONN(conn)); - else if (conn->type == CONN_TYPE_OR) - return connection_or_flushed_some(TO_OR_CONN(conn)); - else - return 0; + conn->state == DIR_CONN_STATE_SERVER_WRITING) { + r = connection_dirserv_flushed_some(TO_DIR_CONN(conn)); + } else if (conn->type == CONN_TYPE_OR) { + r = connection_or_flushed_some(TO_OR_CONN(conn)); + } + conn->in_flushed_some = 0; + return r; } /** We just finished flushing bytes from conn-\>outbuf, and there @@ -2645,6 +2671,10 @@ connection_finished_flushing(connection_t *conn) { tor_assert(conn); + /* If the connection is don't try to do anything more here. */ + if (CONN_IS_CLOSED(conn)) + return 0; + // log_fn(LOG_DEBUG,"entered. Socket %u.", conn->s); switch (conn->type) { diff --git a/src/or/or.h b/src/or/or.h index e2c5c8a71c..9cd56c3d02 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -744,9 +744,6 @@ typedef struct connection_t { /* The next fields are all one-bit booleans. Some are only applicable to * connection subtypes, but we hold them here anyway, to save space. - * (Currently, they all fit into a single byte. If they ever need more than - * one byte, we can shave some bits off type, state, and purpose above, none - * of which is ever over 31.) */ unsigned read_blocked_on_bw:1; /**< Boolean: should we start reading again * once the bandwidth throttler allows it? */ @@ -769,6 +766,9 @@ typedef struct connection_t { /** For AP connections only. If 1, and we fail to reach the chosen exit, * stop requiring it. */ unsigned int chosen_exit_optional:1; + /** Set to 1 when we're inside connection_flushed_some to keep us from + * calling connection_handle_write() recursively. */ + unsigned int in_flushed_some:1; /* For linked connections: */ @@ -781,6 +781,9 @@ typedef struct connection_t { /** True iff we're currently able to read on the linked conn, and our * read_event should be made active with libevent. */ unsigned int active_on_link:1; + /** True iff we've called connection_close_immediate on this linked + * connection */ + unsigned int linked_conn_is_closed:1; int s; /**< Our socket; -1 if this connection is closed, or has no * socket. */