From 8124398835097c3edfe0200a307092b71acfe0a0 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 9 Nov 2012 14:06:54 -0800 Subject: [PATCH 1/3] Check for orconns in connection_mark_for_close and connection_mark_and_flush, and pass the call through channel_close_for_error with a warning to avoid asserts --- src/or/connection.c | 38 ++++++++++++++++++++++++++++++++- src/or/connection.h | 48 +++++++++++++++++++++++++++++++++++------- src/or/connection_or.c | 8 +++---- 3 files changed, 81 insertions(+), 13 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 1fbce418a4..5aac06ff15 100644 --- a/src/or/connection.c +++ b/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 conn 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); } diff --git a/src/or/connection.h b/src/or/connection.h index 6ed9e4a41f..b0c2a42838 100644 --- a/src/or/connection.h +++ b/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) \ - do { \ - connection_t *tmp_conn_ = (c); \ - connection_mark_for_close_(tmp_conn_, (line), (file)); \ - tmp_conn_->hold_open_until_flushed = 1; \ - IF_HAS_BUFFEREVENT(tmp_conn_, \ - connection_start_writing(tmp_conn_)); \ +#define connection_mark_and_flush_(c,line,file) \ + do { \ + connection_t *tmp_conn_ = (c); \ + if (tmp_conn_->type == CONN_TYPE_OR) { \ + log_warn(LD_CHANNEL | LD_BUG, \ + "Something tried to close (and flush) an or_connection_t" \ + " 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) \ diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 9cd56bb89c..1db2def848 100644 --- a/src/or/connection_or.c +++ b/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)); - else connection_mark_for_close(TO_CONN(orconn)); + if (flush) connection_mark_and_flush_internal(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)); - else connection_mark_for_close(TO_CONN(orconn)); + if (flush) connection_mark_and_flush_internal(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 */ From 99e82cab30efdacd464deec524ee0da3576e69d9 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 9 Nov 2012 14:19:45 -0800 Subject: [PATCH 2/3] Make everything in connection.c that uses connection_or_notify_error() also use connection_mark_and_close_internal() to avoid spurious warnings --- src/or/connection.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 5aac06ff15..e6755f4104 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -2763,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; @@ -3279,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 */ @@ -3306,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; @@ -3336,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."); @@ -3401,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); } } From fc1a9a13cf427ec7a319086c820408847fbc189b Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 10 Nov 2012 02:38:40 -0800 Subject: [PATCH 3/3] Add changes file for connection_mark_for_close()/connection_mark_and_flush() or_connection_t checking --- changes/check_for_orconn_on_close | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/check_for_orconn_on_close diff --git a/changes/check_for_orconn_on_close b/changes/check_for_orconn_on_close new file mode 100644 index 0000000000..4d76d5eb5c --- /dev/null +++ b/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.