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

@@ -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

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 <b>conn</b>, 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)
+      conn->state == DIR_CONN_STATE_SERVER_WRITING) {
-    return connection_dirserv_flushed_some(TO_DIR_CONN(conn));
+    r = connection_dirserv_flushed_some(TO_DIR_CONN(conn));
-  else if (conn->type == CONN_TYPE_OR)
+  } else if (conn->type == CONN_TYPE_OR) {
-    return connection_or_flushed_some(TO_OR_CONN(conn));
+    r = connection_or_flushed_some(TO_OR_CONN(conn));
-  else
+  }
-    return 0;
+  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) {

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. */