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/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/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 == diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 0cda6e7bf7..5841a56ffa 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); } @@ -3380,8 +3396,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) @@ -3392,8 +3413,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) { @@ -3423,9 +3445,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 @@ -3436,8 +3457,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 @@ -3461,6 +3483,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");