From bf242ebe6c7e13cf30fedc006e8d25597f9e602d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 20 Nov 2017 15:46:51 -0500 Subject: relay: Remove dead code append_cell_to_circuit_queue() had code disabled from commit 2a95f3171681ee53c97ccba9d80f4454b462aaa7 This code is 4+ years old related to bug #9072 so if we ever want to revisit it, lets inspect/revert this commit. Signed-off-by: David Goulet --- src/or/relay.c | 100 --------------------------------------------------------- 1 file changed, 100 deletions(-) (limited to 'src/or/relay.c') diff --git a/src/or/relay.c b/src/or/relay.c index 09f70793d3..eb9d030802 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2858,20 +2858,6 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) return n_flushed; } -#if 0 -/** Indicate the current preferred cap for middle circuits; zero disables - * the cap. Right now it's just a constant, ORCIRC_MAX_MIDDLE_CELLS, but - * the logic in append_cell_to_circuit_queue() is written to be correct - * if we want to base it on a consensus param or something that might change - * in the future. - */ -static int -get_max_middle_cells(void) -{ - return ORCIRC_MAX_MIDDLE_CELLS; -} -#endif /* 0 */ - /** Add cell to the queue of circ writing to chan * transmitting in direction. */ void @@ -2882,10 +2868,6 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, or_circuit_t *orcirc = NULL; cell_queue_t *queue; int streams_blocked; -#if 0 - uint32_t tgt_max_middle_cells, p_len, n_len, tmp, hard_max_middle_cells; -#endif - int exitward; if (circ->marked_for_close) return; @@ -2900,88 +2882,6 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, streams_blocked = circ->streams_blocked_on_p_chan; } - /* - * Disabling this for now because of a possible guard discovery attack - */ -#if 0 - /* Are we a middle circuit about to exceed ORCIRC_MAX_MIDDLE_CELLS? */ - if ((circ->n_chan != NULL) && CIRCUIT_IS_ORCIRC(circ)) { - orcirc = TO_OR_CIRCUIT(circ); - if (orcirc->p_chan) { - /* We are a middle circuit if we have both n_chan and p_chan */ - /* We'll need to know the current preferred maximum */ - tgt_max_middle_cells = get_max_middle_cells(); - if (tgt_max_middle_cells > 0) { - /* Do we need to initialize middle_max_cells? */ - if (orcirc->max_middle_cells == 0) { - orcirc->max_middle_cells = tgt_max_middle_cells; - } else { - if (tgt_max_middle_cells > orcirc->max_middle_cells) { - /* If we want to increase the cap, we can do so right away */ - orcirc->max_middle_cells = tgt_max_middle_cells; - } else if (tgt_max_middle_cells < orcirc->max_middle_cells) { - /* - * If we're shrinking the cap, we can't shrink past either queue; - * compare tgt_max_middle_cells rather than tgt_max_middle_cells * - * ORCIRC_MAX_MIDDLE_KILL_THRESH so the queues don't shrink enough - * to generate spurious warnings, either. - */ - n_len = circ->n_chan_cells.n; - p_len = orcirc->p_chan_cells.n; - tmp = tgt_max_middle_cells; - if (tmp < n_len) tmp = n_len; - if (tmp < p_len) tmp = p_len; - orcirc->max_middle_cells = tmp; - } - /* else no change */ - } - } else { - /* tgt_max_middle_cells == 0 indicates we should disable the cap */ - orcirc->max_middle_cells = 0; - } - - /* Now we know orcirc->max_middle_cells is set correctly */ - if (orcirc->max_middle_cells > 0) { - hard_max_middle_cells = - (uint32_t)(((double)orcirc->max_middle_cells) * - ORCIRC_MAX_MIDDLE_KILL_THRESH); - - if ((unsigned)queue->n + 1 >= hard_max_middle_cells) { - /* Queueing this cell would put queue over the kill theshold */ - log_warn(LD_CIRC, - "Got a cell exceeding the hard cap of %u in the " - "%s direction on middle circ ID %u on chan ID " - U64_FORMAT "; killing the circuit.", - hard_max_middle_cells, - (direction == CELL_DIRECTION_OUT) ? "n" : "p", - (direction == CELL_DIRECTION_OUT) ? - circ->n_circ_id : orcirc->p_circ_id, - U64_PRINTF_ARG( - (direction == CELL_DIRECTION_OUT) ? - circ->n_chan->global_identifier : - orcirc->p_chan->global_identifier)); - circuit_mark_for_close(circ, END_CIRC_REASON_RESOURCELIMIT); - return; - } else if ((unsigned)queue->n + 1 == orcirc->max_middle_cells) { - /* Only use ==, not >= for this test so we don't spam the log */ - log_warn(LD_CIRC, - "While trying to queue a cell, reached the soft cap of %u " - "in the %s direction on middle circ ID %u " - "on chan ID " U64_FORMAT ".", - orcirc->max_middle_cells, - (direction == CELL_DIRECTION_OUT) ? "n" : "p", - (direction == CELL_DIRECTION_OUT) ? - circ->n_circ_id : orcirc->p_circ_id, - U64_PRINTF_ARG( - (direction == CELL_DIRECTION_OUT) ? - circ->n_chan->global_identifier : - orcirc->p_chan->global_identifier)); - } - } - } - } -#endif /* 0 */ - cell_queue_append_packed_copy(circ, queue, exitward, cell, chan->wide_circ_ids, 1); -- cgit v1.2.3-54-g00ecf From d165f0fd30bc9823152dad6769afab0afae2ea6d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 20 Nov 2017 15:53:25 -0500 Subject: relay: Improve comment in append_cell_to_circuit_queue() This function is part of the tor fast path so this commit adds more documentation to it as it is critical. Signed-off-by: David Goulet --- src/or/relay.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'src/or/relay.c') diff --git a/src/or/relay.c b/src/or/relay.c index eb9d030802..d6c103c146 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2859,7 +2859,12 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) } /** Add cell to the queue of circ writing to chan - * transmitting in direction. */ + * transmitting in direction. + * + * The given cell is copied over the circuit queue so the caller must + * cleanup the memory. + * + * This function is part of the fast path. */ void append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, cell_t *cell, cell_direction_t direction, @@ -2882,11 +2887,14 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, streams_blocked = circ->streams_blocked_on_p_chan; } + /* Very important that we copy to the circuit queue because all calls to + * this function use the stack for the cell memory. */ cell_queue_append_packed_copy(circ, queue, exitward, cell, chan->wide_circ_ids, 1); + /* Check and run the OOM if needed. */ if (PREDICT_UNLIKELY(cell_queues_check_size())) { - /* We ran the OOM handler */ + /* We ran the OOM handler which might have closed this circuit. */ if (circ->marked_for_close) return; } -- cgit v1.2.3-54-g00ecf From 56833bf4491ef774163bc263bb18fa66fb378292 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 21 Nov 2017 12:56:10 -0500 Subject: channel: Requeue cell to circuit if channnel failed If the channel layer failed to write a cell from the circuit queue, requeue it so it can be retried on the same channel later. Signed-off-by: David Goulet --- src/or/relay.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) (limited to 'src/or/relay.c') diff --git a/src/or/relay.c b/src/or/relay.c index d6c103c146..f36fc78cda 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2521,6 +2521,17 @@ cell_queue_pop(cell_queue_t *queue) return cell; } +/** Insert cell as the head of the queue. */ +static void +cell_insert_head(cell_queue_t *queue, packed_cell_t *cell) +{ + tor_assert(queue); + tor_assert(cell); + + TOR_SIMPLEQ_INSERT_HEAD(&queue->head, cell, next); + ++queue->n; +} + /** Return the total number of bytes used for each packed_cell in a queue. * Approximate. */ size_t @@ -2746,7 +2757,10 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) /* this code is duplicated from some of the logic below. Ugly! XXXX */ tor_assert(destroy_queue->n > 0); cell = cell_queue_pop(destroy_queue); - channel_write_packed_cell(chan, cell); + if (channel_write_packed_cell(chan, cell) < 0) { + cell_insert_head(destroy_queue, cell); + continue; + } /* Update the cmux destroy counter */ circuitmux_notify_xmit_destroy(cmux); cell = NULL; @@ -2823,7 +2837,12 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) DIRREQ_CIRC_QUEUE_FLUSHED); /* Now send the cell */ - channel_write_packed_cell(chan, cell); + if (channel_write_packed_cell(chan, cell) < 0) { + /* Unable to send the cell, put it back at the start of the circuit + * queue so we can retry. */ + cell_insert_head(queue, cell); + continue; + } cell = NULL; /* -- cgit v1.2.3-54-g00ecf From 6120efd771928fc958b552b9f5c3e09d949e92fe Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Dec 2017 15:41:09 -0500 Subject: chan: Do not re-queue after a fail cell write Couple things happen in this commit. First, we do not re-queue a cell back in the circuit queue if the write packed cell failed. Currently, it is close to impossible to have it failed but just in case, the channel is mark as closed and we move on. The second thing is that the channel_write_packed_cell() always took ownership of the cell whatever the outcome. This means, on success or failure, it needs to free it. It turns out that that we were using the wrong free function in one case and not freeing it in an other possible code path. So, this commit makes sure we only free it in one place that is at the very end of channel_write_packed_cell() which is the top layer of the channel abstraction. This makes also channel_tls_write_packed_cell_method() return a negative value on error. Two unit tests had to be fixed (quite trivial) due to a double free of the packed cell in the test since now we do free it in all cases correctly. Part of #23709 Signed-off-by: David Goulet --- src/or/channel.c | 24 +++++++++++++++++++----- src/or/channeltls.c | 11 +++++------ src/or/relay.c | 24 ++++++++---------------- src/test/test_channel.c | 2 -- 4 files changed, 32 insertions(+), 29 deletions(-) (limited to 'src/or/relay.c') diff --git a/src/or/channel.c b/src/or/channel.c index 94d7af47b5..9e79fc1a3f 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -1431,8 +1431,11 @@ channel_clear_remote_end(channel_t *chan) /** * Write to a channel the given packed cell. * - * Return 0 on success and the cell is freed so the caller MUST discard any - * reference to it. On error, -1 is returned and the cell is untouched. + * Return 0 on success or -1 on error. + * + * Two possible errors can happen. Either the channel is not opened or the + * lower layer (specialized channel) failed to write it. In both cases, it is + * the caller responsability to free the cell. */ static int write_packed_cell(channel_t *chan, packed_cell_t *cell) @@ -1483,10 +1486,15 @@ write_packed_cell(channel_t *chan, packed_cell_t *cell) * Write a packed cell to a channel using the write_cell() method. This is * called by the transport-independent code to deliver a packed cell to a * channel for transmission. + * + * Return 0 on success else a negative value. In both cases, the caller should + * not access the cell anymore, it is freed both on success and error. */ int channel_write_packed_cell(channel_t *chan, packed_cell_t *cell) { + int ret = -1; + tor_assert(chan); tor_assert(cell); @@ -1494,14 +1502,20 @@ channel_write_packed_cell(channel_t *chan, packed_cell_t *cell) log_debug(LD_CHANNEL, "Discarding %p on closing channel %p with " "global ID "U64_FORMAT, cell, chan, U64_PRINTF_ARG(chan->global_identifier)); - tor_free(cell); - return -1; + goto end; } log_debug(LD_CHANNEL, "Writing %p to channel %p with global ID " U64_FORMAT, cell, chan, U64_PRINTF_ARG(chan->global_identifier)); - return write_packed_cell(chan, cell); + ret = write_packed_cell(chan, cell); + + end: + /* Whatever happens, we free the cell. Either an error occured or the cell + * was put on the connection outbuf, both cases we have ownership of the + * cell and we free it. */ + packed_cell_free(cell); + return ret; } /** diff --git a/src/or/channeltls.c b/src/or/channeltls.c index e6ecc15381..023ccdefd3 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -832,6 +832,9 @@ channel_tls_write_cell_method(channel_t *chan, cell_t *cell) * * This implements the write_packed_cell method for channel_tls_t; given a * channel_tls_t and a packed_cell_t, transmit the packed_cell_t. + * + * Return 0 on success or negative value on error. The caller must free the + * packed cell. */ static int @@ -841,7 +844,6 @@ channel_tls_write_packed_cell_method(channel_t *chan, tor_assert(chan); channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); size_t cell_network_size = get_cell_network_size(chan->wide_circ_ids); - int written = 0; tor_assert(tlschan); tor_assert(packed_cell); @@ -849,18 +851,15 @@ channel_tls_write_packed_cell_method(channel_t *chan, if (tlschan->conn) { connection_buf_add(packed_cell->body, cell_network_size, TO_CONN(tlschan->conn)); - - /* This is where the cell is finished; used to be done from relay.c */ - packed_cell_free(packed_cell); - ++written; } else { log_info(LD_CHANNEL, "something called write_packed_cell on a tlschan " "(%p with ID " U64_FORMAT " but no conn", chan, U64_PRINTF_ARG(chan->global_identifier)); + return -1; } - return written; + return 0; } /** diff --git a/src/or/relay.c b/src/or/relay.c index f36fc78cda..ac91032844 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2521,17 +2521,6 @@ cell_queue_pop(cell_queue_t *queue) return cell; } -/** Insert cell as the head of the queue. */ -static void -cell_insert_head(cell_queue_t *queue, packed_cell_t *cell) -{ - tor_assert(queue); - tor_assert(cell); - - TOR_SIMPLEQ_INSERT_HEAD(&queue->head, cell, next); - ++queue->n; -} - /** Return the total number of bytes used for each packed_cell in a queue. * Approximate. */ size_t @@ -2757,8 +2746,11 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) /* this code is duplicated from some of the logic below. Ugly! XXXX */ tor_assert(destroy_queue->n > 0); cell = cell_queue_pop(destroy_queue); + /* Send the DESTROY cell. It is very unlikely that this fails but just + * in case, get rid of the channel. */ if (channel_write_packed_cell(chan, cell) < 0) { - cell_insert_head(destroy_queue, cell); + /* The cell has been freed. */ + channel_mark_for_close(chan); continue; } /* Update the cmux destroy counter */ @@ -2836,11 +2828,11 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) DIRREQ_TUNNELED, DIRREQ_CIRC_QUEUE_FLUSHED); - /* Now send the cell */ + /* Now send the cell. It is very unlikely that this fails but just in + * case, get rid of the channel. */ if (channel_write_packed_cell(chan, cell) < 0) { - /* Unable to send the cell, put it back at the start of the circuit - * queue so we can retry. */ - cell_insert_head(queue, cell); + /* The cell has been freed at this point. */ + channel_mark_for_close(chan); continue; } cell = NULL; diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 88a13862bd..c3b99d87ac 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -199,7 +199,6 @@ chan_test_write_packed_cell(channel_t *ch, if (test_chan_accept_cells) { /* Free the cell and bump the counter */ - packed_cell_free(packed_cell); ++test_cells_written; rv = 1; } @@ -846,7 +845,6 @@ test_channel_lifecycle(void *arg) done: free_fake_channel(ch1); free_fake_channel(ch2); - tor_free(p_cell); UNMOCK(scheduler_channel_doesnt_want_writes); UNMOCK(scheduler_release_channel); -- cgit v1.2.3-54-g00ecf