From 269b4561a17698732c6402e2b7a8aeb1f8e3c3bf Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 15 Apr 2024 14:24:45 -0400 Subject: conflux: Avoid noting a cell was sent on a closed circuit It turns out that circuit_package_relay_cell() returns 0 in order to drop a cell but there is a code path, if the circuit queue is full, that also silently closes the circuit and returns 0. This lead to Conflux thinking a cell was sent but actually the cell was not and the circuit was closed leading to the hard assert. And so this function makes sure that circuit_package_relay_cell() and append_cell_to_circuit_queue() returns a value that indicate what happened with the cell and circuit so the caller can make an informed decision with it. This change makes it that we do NOT enter the Conflux subsystem if the cell is not queued on the circuit. Fixes #40921 Signed-off-by: David Goulet --- changes/ticket40921 | 3 ++ src/core/or/circuitbuild.c | 6 ++-- src/core/or/conflux.c | 5 +++- src/core/or/relay.c | 51 ++++++++++++++++++++++++---------- src/core/or/relay.h | 6 ++-- src/feature/relay/circuitbuild_relay.c | 6 ++-- 6 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 changes/ticket40921 diff --git a/changes/ticket40921 b/changes/ticket40921 new file mode 100644 index 0000000000..5818b91864 --- /dev/null +++ b/changes/ticket40921 @@ -0,0 +1,3 @@ + o Minor bugfixes (conflux): + - Avoid a potential hard assert (crash) when sending a cell on a Conflux + set. Fixes bug 40921; bugfix on 0.4.8.1-alpha. diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c index d6e022e7fa..dc1912294b 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -816,8 +816,10 @@ circuit_deliver_create_cell,(circuit_t *circ, circuit_set_n_circid_chan(circ, id, circ->n_chan); cell.circ_id = circ->n_circ_id; - append_cell_to_circuit_queue(circ, circ->n_chan, &cell, - CELL_DIRECTION_OUT, 0); + if (append_cell_to_circuit_queue(circ, circ->n_chan, &cell, + CELL_DIRECTION_OUT, 0) < 0) { + return -1; + } if (CIRCUIT_IS_ORIGIN(circ)) { /* Update began timestamp for circuits starting their first hop */ diff --git a/src/core/or/conflux.c b/src/core/or/conflux.c index 677df95067..9025dcffaf 100644 --- a/src/core/or/conflux.c +++ b/src/core/or/conflux.c @@ -531,7 +531,10 @@ conflux_note_cell_sent(conflux_t *cfx, circuit_t *circ, uint8_t relay_command) } leg = conflux_get_leg(cfx, circ); - tor_assert(leg); + if (leg == NULL) { + log_fn(LOG_PROTOCOL_WARN, LD_BUG, "No Conflux leg after sending a cell"); + return; + } leg->last_seq_sent++; diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 6abe802355..8e6fddf18b 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -365,14 +365,19 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, * we might kill the circ before we relay * the cells. */ - append_cell_to_circuit_queue(circ, chan, cell, cell_direction, 0); + if (append_cell_to_circuit_queue(circ, chan, cell, cell_direction, 0) < 0) { + return -END_CIRC_REASON_RESOURCELIMIT; + } return 0; } /** Package a relay cell from an edge: * - Encrypt it to the right layer * - Append it to the appropriate cell_queue on circ. - */ + * + * Return 1 if the cell was successfully sent as in queued on the circuit. + * Return 0 if the cell needs to be dropped as in ignored. + * Return -1 on error for which the circuit should be marked for close. */ MOCK_IMPL(int, circuit_package_relay_cell, (cell_t *cell, circuit_t *circ, cell_direction_t cell_direction, @@ -430,8 +435,8 @@ circuit_package_relay_cell, (cell_t *cell, circuit_t *circ, } ++stats_n_relay_cells_relayed; - append_cell_to_circuit_queue(circ, chan, cell, cell_direction, on_stream); - return 0; + return append_cell_to_circuit_queue(circ, chan, cell, + cell_direction, on_stream); } /** If cell's stream_id matches the stream_id of any conn that's @@ -742,13 +747,24 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ, circuit_sent_valid_data(origin_circ, rh.length); } - if (circuit_package_relay_cell(&cell, circ, cell_direction, cpath_layer, - stream_id, filename, lineno) < 0) { + int ret = circuit_package_relay_cell(&cell, circ, cell_direction, + cpath_layer, stream_id, filename, + lineno); + if (ret < 0) { log_warn(LD_BUG,"circuit_package_relay_cell failed. Closing."); circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); return -1; + } else if (ret == 0) { + /* This means we should drop the cell or that the circuit was already + * marked for close. At this point in time, we do NOT close the circuit if + * the cell is dropped. It is not the case with arti where each circuit + * protocol violation will lead to closing the circuit. */ + return 0; } + /* At this point, we are certain that the cell was queued on the circuit and + * thus will be sent on the wire. */ + if (circ->conflux) { conflux_note_cell_sent(circ->conflux, circ, relay_command); } @@ -3381,8 +3397,13 @@ relay_consensus_has_changed(const networkstatus_t *ns) * The given cell is copied onto the circuit queue so the caller must * cleanup the memory. * - * This function is part of the fast path. */ -void + * This function is part of the fast path. + * + * Return 1 if the cell was successfully sent. + * Return 0 if the cell can not be sent. The caller MUST NOT close the circuit. + * Return -1 indicating an error and that the caller should mark the circuit + * for close. */ +int append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, cell_t *cell, cell_direction_t direction, streamid_t fromstream) @@ -3393,8 +3414,9 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, int32_t max_queue_size; int circ_blocked; int exitward; - if (circ->marked_for_close) - return; + if (circ->marked_for_close) { + return 0; + } exitward = (direction == CELL_DIRECTION_OUT); if (exitward) { @@ -3424,9 +3446,8 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, "Closing circuit for safety reasons.", (exitward) ? "Outbound" : "Inbound", queue->n, max_queue_size); - circuit_mark_for_close(circ, END_CIRC_REASON_RESOURCELIMIT); stats_n_circ_max_cell_reached++; - return; + return -1; } /* Very important that we copy to the circuit queue because all calls to @@ -3437,8 +3458,9 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, /* Check and run the OOM if needed. */ if (PREDICT_UNLIKELY(cell_queues_check_size())) { /* We ran the OOM handler which might have closed this circuit. */ - if (circ->marked_for_close) - return; + if (circ->marked_for_close) { + return 0; + } } /* If we have too many cells on the circuit, note that it should @@ -3462,6 +3484,7 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, /* New way: mark this as having waiting cells for the scheduler */ scheduler_channel_has_waiting_cells(chan); + return 1; } /** Append an encoded value of addr to payload_out, which must diff --git a/src/core/or/relay.h b/src/core/or/relay.h index 3a1f3b0ae5..12198ae982 100644 --- a/src/core/or/relay.h +++ b/src/core/or/relay.h @@ -76,9 +76,9 @@ void cell_queue_append_packed_copy(circuit_t *circ, cell_queue_t *queue, int exitward, const cell_t *cell, int wide_circ_ids, int use_stats); -void append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, - cell_t *cell, cell_direction_t direction, - streamid_t fromstream); +int append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, + cell_t *cell, cell_direction_t direction, + streamid_t fromstream); void destroy_cell_queue_init(destroy_cell_queue_t *queue); void destroy_cell_queue_clear(destroy_cell_queue_t *queue); diff --git a/src/feature/relay/circuitbuild_relay.c b/src/feature/relay/circuitbuild_relay.c index 5b1609a1af..ce6cbe6df4 100644 --- a/src/feature/relay/circuitbuild_relay.c +++ b/src/feature/relay/circuitbuild_relay.c @@ -579,8 +579,10 @@ onionskin_answer(struct or_circuit_t *circ, int used_create_fast = (created_cell->cell_type == CELL_CREATED_FAST); - append_cell_to_circuit_queue(TO_CIRCUIT(circ), - circ->p_chan, &cell, CELL_DIRECTION_IN, 0); + if (append_cell_to_circuit_queue(TO_CIRCUIT(circ), circ->p_chan, + &cell, CELL_DIRECTION_IN, 0) < 0) { + return -1; + } log_debug(LD_CIRC,"Finished sending '%s' cell.", used_create_fast ? "created_fast" : "created"); -- cgit v1.2.3-54-g00ecf From 612b801ea53cd0b318e3bf88da5bb5de6b507226 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 15 Apr 2024 14:45:29 -0400 Subject: conflux: Don't process a closed circuit on get packaged window Signed-off-by: David Goulet --- changes/ticket40908 | 5 +++++ src/core/or/conflux_util.c | 7 +++++++ 2 files changed, 12 insertions(+) create mode 100644 changes/ticket40908 diff --git a/changes/ticket40908 b/changes/ticket40908 new file mode 100644 index 0000000000..28cd3f0f36 --- /dev/null +++ b/changes/ticket40908 @@ -0,0 +1,5 @@ + o Minor bugfixes (conflux): + - Make sure we don't process a closed circuit when packaging data. This lead + to a non fatal BUG() spamming logs. Fixes bug 40908; bugfix on + 0.4.8.1-alpha. + diff --git a/src/core/or/conflux_util.c b/src/core/or/conflux_util.c index 31ab983f8f..4277424ec8 100644 --- a/src/core/or/conflux_util.c +++ b/src/core/or/conflux_util.c @@ -33,6 +33,13 @@ int circuit_get_package_window(circuit_t *circ, const crypt_path_t *cpath) { + /* We believe it is possible to get a closed circuit related to the + * on_circuit pointer of a connection not being nullified before ending up + * here. Else, this can lead to loud bug like experienced in #40908. */ + if (circ->marked_for_close) { + return 0; + } + if (circ->conflux) { if (CIRCUIT_IS_ORIGIN(circ)) { tor_assert_nonfatal(circ->purpose == -- cgit v1.2.3-54-g00ecf