Merge branch 'check_for_orconn_on_close_squashed' of ssh://git-rw.torproject.org/user/andrea/tor

changes/check_for_orconn_on_close

@@ -0,0 +1,4 @@
+  o Minor bugfixes:
+    - Check for closing an or_connection_t without going through correct
+      channel functions; emit a warning and then call
+      connection_or_close_for_error() so we don't assert as in 7212 and 7267.

src/or/connection.c

@@ -688,6 +688,41 @@ connection_mark_for_close_(connection_t *conn, int line, const char *file)
   tor_assert(line < 1<<16); /* marked_for_close can only fit a uint16_t. */
   tor_assert(file);
 
+  if (conn->type == CONN_TYPE_OR) {
+    /*
+     * An or_connection should have been closed through one of the channel-
+     * aware functions in connection_or.c.  We'll assume this is an error
+     * close and do that, and log a bug warning.
+     */
+    log_warn(LD_CHANNEL | LD_BUG,
+             "Something tried to close an or_connection_t without going "
+             "through channels at %s:%d",
+             file, line);
+    connection_or_close_for_error(TO_OR_CONN(conn), 0);
+  } else {
+    /* Pass it down to the real function */
+    connection_mark_for_close_internal_(conn, line, file);
+  }
+}
+
+/** Mark <b>conn</b> to be closed next time we loop through
+ * conn_close_if_marked() in main.c; the _internal version bypasses the
+ * CONN_TYPE_OR checks; this should be called when you either are sure that
+ * if this is an or_connection_t the controlling channel has been notified
+ * (e.g. with connection_or_notify_error()), or you actually are the
+ * connection_or_close_for_error() or connection_or_close_normally function.
+ * For all other cases, use connection_mark_and_flush() instead, which
+ * checks for or_connection_t properly, instead.  See below.
+ */
+void
+connection_mark_for_close_internal_(connection_t *conn,
+                                    int line, const char *file)
+{
+  assert_connection_ok(conn,0);
+  tor_assert(line);
+  tor_assert(line < 1<<16); /* marked_for_close can only fit a uint16_t. */
+  tor_assert(file);
+
   if (conn->marked_for_close) {
     log(LOG_WARN,LD_BUG,"Duplicate call to connection_mark_for_close at %s:%d"
         " (first at %s:%d)", file, line, conn->marked_for_close_file,
@@ -702,7 +737,8 @@ connection_mark_for_close_(connection_t *conn, int line, const char *file)
      * this so we can find things that call this wrongly when the asserts hit.
      */
     log_debug(LD_CHANNEL,
-              "Calling connection_mark_for_close on an OR conn at %s:%d",
+              "Calling connection_mark_for_close_internal_() on an OR conn "
+              "at %s:%d",
               file, line);
   }
 
@@ -2727,7 +2763,11 @@ connection_handle_read_impl(connection_t *conn)
       }
     }
     connection_close_immediate(conn); /* Don't flush; connection is dead. */
-    connection_mark_for_close(conn);
+    /*
+     * This can bypass normal channel checking since we did
+     * connection_or_notify_error() above.
+     */
+    connection_mark_for_close_internal(conn);
     return -1;
   }
   n_read += buf_datalen(conn->inbuf) - before;
@@ -3243,7 +3283,11 @@ connection_handle_write_impl(connection_t *conn, int force)
                                      tor_socket_strerror(e));
 
         connection_close_immediate(conn);
-        connection_mark_for_close(conn);
+        /*
+         * This can bypass normal channel checking since we did
+         * connection_or_notify_error() above.
+         */
+        connection_mark_for_close_internal(conn);
         return -1;
       } else {
         return 0; /* no change, see if next time is better */
@@ -3270,7 +3314,11 @@ connection_handle_write_impl(connection_t *conn, int force)
                                    "TLS error in connection_tls_"
                                    "continue_handshake()");
         connection_close_immediate(conn);
-        connection_mark_for_close(conn);
+        /*
+         * This can bypass normal channel checking since we did
+         * connection_or_notify_error() above.
+         */
+        connection_mark_for_close_internal(conn);
         return -1;
       }
       return 0;
@@ -3300,7 +3348,11 @@ connection_handle_write_impl(connection_t *conn, int force)
                                      "TLS error in during flush" :
                                      "TLS closed during flush");
         connection_close_immediate(conn);
-        connection_mark_for_close(conn);
+        /*
+         * This can bypass normal channel checking since we did
+         * connection_or_notify_error() above.
+         */
+        connection_mark_for_close_internal(conn);
         return -1;
       case TOR_TLS_WANTWRITE:
         log_debug(LD_NET,"wanted write.");
@@ -3365,7 +3417,11 @@ connection_handle_write_impl(connection_t *conn, int force)
                                    "connection_flushed_some()");
       }
 
-      connection_mark_for_close(conn);
+      /*
+       * This can bypass normal channel checking since we did
+       * connection_or_notify_error() above.
+       */
+      connection_mark_for_close_internal(conn);
     }
   }
 

src/or/connection.h

@@ -31,21 +31,53 @@ void connection_free(connection_t *conn);
 void connection_free_all(void);
 void connection_about_to_close_connection(connection_t *conn);
 void connection_close_immediate(connection_t *conn);
-void connection_mark_for_close_(connection_t *conn,int line, const char *file);
+void connection_mark_for_close_(connection_t *conn,
+                                int line, const char *file);
+void connection_mark_for_close_internal_(connection_t *conn,
+                                         int line, const char *file);
 
 #define connection_mark_for_close(c) \
   connection_mark_for_close_((c), __LINE__, SHORT_FILE__)
+#define connection_mark_for_close_internal(c) \
+  connection_mark_for_close_internal_((c), __LINE__, SHORT_FILE__)
+
+/**
+ * Mark 'c' for close, but try to hold it open until all the data is written.
+ * Use the _internal versions of connection_mark_for_close; this should be
+ * called when you either are sure that if this is an or_connection_t the
+ * controlling channel has been notified (e.g. with
+ * connection_or_notify_error()), or you actually are the
+ * connection_or_close_for_error() or connection_or_close_normally function.
+ * For all other cases, use connection_mark_and_flush() instead, which
+ * checks for or_connection_t properly, instead.  See below.
+ */
+#define connection_mark_and_flush_internal_(c,line,file)                  \
+  do {                                                                    \
+    connection_t *tmp_conn_ = (c);                                        \
+    connection_mark_for_close_internal_(tmp_conn_, (line), (file));       \
+    tmp_conn_->hold_open_until_flushed = 1;                               \
+    IF_HAS_BUFFEREVENT(tmp_conn_,                                         \
+                       connection_start_writing(tmp_conn_));              \
+  } while (0)
+
+#define connection_mark_and_flush_internal(c)            \
+  connection_mark_and_flush_internal_((c), __LINE__, SHORT_FILE__)
 
 /**
  * Mark 'c' for close, but try to hold it open until all the data is written.
  */
-#define connection_mark_and_flush_(c,line,file)                         \
+#define connection_mark_and_flush_(c,line,file)                           \
-  do {                                                                  \
+  do {                                                                    \
-    connection_t *tmp_conn_ = (c);                                      \
+    connection_t *tmp_conn_ = (c);                                        \
-    connection_mark_for_close_(tmp_conn_, (line), (file));              \
+    if (tmp_conn_->type == CONN_TYPE_OR) {                                \
-    tmp_conn_->hold_open_until_flushed = 1;                             \
+      log_warn(LD_CHANNEL | LD_BUG,                                       \
-    IF_HAS_BUFFEREVENT(tmp_conn_,                                       \
+               "Something tried to close (and flush) an or_connection_t"  \
-                       connection_start_writing(tmp_conn_));            \
+               " without going through channels at %s:%d",                \
+               file, line);                                               \
+      connection_or_close_for_error(TO_OR_CONN(tmp_conn_), 1);            \
+    } else {                                                              \
+      connection_mark_and_flush_internal_(c, line, file);                 \
+    }                                                                     \
   } while (0)
 
 #define connection_mark_and_flush(c)            \

src/or/connection_or.c

@@ -1143,8 +1143,8 @@ connection_or_close_normally(or_connection_t *orconn, int flush)
   channel_t *chan = NULL;
 
   tor_assert(orconn);
-  if (flush) connection_mark_and_flush(TO_CONN(orconn));
+  if (flush) connection_mark_and_flush_internal(TO_CONN(orconn));
-  else connection_mark_for_close(TO_CONN(orconn));
+  else connection_mark_for_close_internal(TO_CONN(orconn));
   if (orconn->chan) {
     chan = TLS_CHAN_TO_BASE(orconn->chan);
     /* Don't transition if we're already in closing, closed or error */
@@ -1166,8 +1166,8 @@ connection_or_close_for_error(or_connection_t *orconn, int flush)
   channel_t *chan = NULL;
 
   tor_assert(orconn);
-  if (flush) connection_mark_and_flush(TO_CONN(orconn));
+  if (flush) connection_mark_and_flush_internal(TO_CONN(orconn));
-  else connection_mark_for_close(TO_CONN(orconn));
+  else connection_mark_for_close_internal(TO_CONN(orconn));
   if (orconn->chan) {
     chan = TLS_CHAN_TO_BASE(orconn->chan);
     /* Don't transition if we're already in closing, closed or error */