From 1b6ccb52eab139cd836e01316c46252e8c143288 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Sep 2017 09:24:01 -0400 Subject: [PATCH 1/2] Log more information when bug8185 is about to trigger. My current theory is that this is just a marked circuit that hasn't closed yet, but let's gather more information in case that theory is wrong. Diagnostic for 8185. --- changes/8185_diagnostic | 4 ++++ src/or/relay.c | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changes/8185_diagnostic diff --git a/changes/8185_diagnostic b/changes/8185_diagnostic new file mode 100644 index 0000000000..b809c4f3ee --- /dev/null +++ b/changes/8185_diagnostic @@ -0,0 +1,4 @@ + o Minor features (logging diagnostic): + - Log more circuit information whenever we are about to try to + package a relay cell on a circuit with a nonexistent n_chan. + Attempt to diagnose bug 8185. diff --git a/src/or/relay.c b/src/or/relay.c index 2bfec342d3..ef4b1f423f 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -500,7 +500,15 @@ circuit_package_relay_cell(cell_t *cell, circuit_t *circ, chan = circ->n_chan; if (!chan) { log_warn(LD_BUG,"outgoing relay cell sent from %s:%d has n_chan==NULL." - " Dropping.", filename, lineno); + " Dropping. Circuit is in state %s (%d), and is " + "%smarked for close. (%s:%d, %d)", filename, lineno, + circuit_state_to_string(circ->state), circ->state, + circ->marked_for_close ? "" : "not ", + circ->marked_for_close_file?circ->marked_for_close_file:"", + circ->marked_for_close, circ->marked_for_close_reason); + if (CIRCUIT_IS_ORIGIN(circ)) { + circuit_log_path(LOG_WARN, LD_BUG, TO_ORIGIN_CIRCUIT(circ)); + } log_backtrace(LOG_WARN,LD_BUG,""); return 0; /* just drop it */ } From d256d4c0a6c6a258c4f6c6839dd53c7e304fa94f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Sep 2017 09:26:16 -0400 Subject: [PATCH 2/2] Don't package cells onto marked circuits. This caused a BUG log when we noticed that the circuit had no channel. The likeliest culprit for exposing that behavior is d769cab3e5097980, where we made circuit_mark_for_close() NULL out the n_chan and p_chan fields of the circuit. Fixes bug 8185; bugfix on 0.2.5.4-alpha, I think. --- changes/bug8185_025 | 6 ++++++ src/or/relay.c | 11 +++++++++++ 2 files changed, 17 insertions(+) create mode 100644 changes/bug8185_025 diff --git a/changes/bug8185_025 b/changes/bug8185_025 new file mode 100644 index 0000000000..1bfc12b1e4 --- /dev/null +++ b/changes/bug8185_025 @@ -0,0 +1,6 @@ + o Minor bugfixes (logging, relay shutdown, annoyance): + - When a circuit is marked for close, do not attempt to package any cells + for channels on that circuit. Previously, we would detect this + condition lower in the call stack, when we noticed that the circuit had + no attached channel, and log an annoying message. Fixes bug 8185; + bugfix on 0.2.5.4-alpha. diff --git a/src/or/relay.c b/src/or/relay.c index daf354c34c..48c823423a 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -390,6 +390,11 @@ circuit_package_relay_cell(cell_t *cell, circuit_t *circ, { channel_t *chan; /* where to send the cell */ + if (circ->marked_for_close) { + /* Circuit is marked; send nothing. */ + return 0; + } + if (cell_direction == CELL_DIRECTION_OUT) { crypt_path_t *thishop; /* counter for repeated crypts */ chan = circ->n_chan; @@ -703,6 +708,12 @@ connection_edge_send_command(edge_connection_t *fromconn, return -1; } + if (circ->marked_for_close) { + /* The circuit has been marked, but not freed yet. When it's freed, it + * will mark this connection for close. */ + return -1; + } + return relay_send_command_from_edge(fromconn->stream_id, circ, relay_command, payload, payload_len, cpath_layer);