From b32a8b024ce3a57f8865ed2937dbc3f3fd5072ee Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 Aug 2014 12:10:57 -0400 Subject: Don't send DESTROY to circID 0 when circuit_deliver_create_cell fails Cypherpunks found this and wrote this patch. Fix for 12848; fix on (I think) d58d4c0d, which went into 0.0.8pre1 --- src/or/circuitbuild.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'src/or') diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 4603de071f..11f8250934 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -584,18 +584,18 @@ circuit_deliver_create_cell(circuit_t *circ, const create_cell_t *create_cell, id = get_unique_circ_id_by_chan(circ->n_chan); if (!id) { log_warn(LD_CIRC,"failed to get unique circID."); - return -1; + goto error; } - log_debug(LD_CIRC,"Chosen circID %u.", (unsigned)id); - circuit_set_n_circid_chan(circ, id, circ->n_chan); memset(&cell, 0, sizeof(cell_t)); r = relayed ? create_cell_format_relayed(&cell, create_cell) : create_cell_format(&cell, create_cell); if (r < 0) { log_warn(LD_CIRC,"Couldn't format create cell"); - return -1; + goto error; } + log_debug(LD_CIRC,"Chosen circID %u.", (unsigned)id); + 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, @@ -619,6 +619,9 @@ circuit_deliver_create_cell(circuit_t *circ, const create_cell_t *create_cell, } return 0; + error: + circ->n_chan = NULL; + return -1; } /** We've decided to start our reachability testing. If all -- cgit v1.2.3-54-g00ecf From 981e037fd3b9e20b6e58e9c1470999a0f3a1ef0e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 Aug 2014 12:14:05 -0400 Subject: Add an extra check in channel_send_destroy for circID==0 Prevents other cases of 12848. --- src/or/channel.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'src/or') diff --git a/src/or/channel.c b/src/or/channel.c index 1270eace7d..cd55bd0dfd 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -2625,6 +2625,14 @@ channel_send_destroy(circid_t circ_id, channel_t *chan, int reason) cell_t cell; tor_assert(chan); + if (circ_id == 0) { + log_warn(LD_BUG, "Attempted to send a destroy cell for circID 0 " + "on a channel " U64_FORMAT " at %p in state %s (%d)", + U64_PRINTF_ARG(chan->global_identifier), + chan, channel_state_to_string(chan->state), + chan->state); + return 0; + } /* Check to make sure we can send on this channel first */ if (!(chan->state == CHANNEL_STATE_CLOSING || -- cgit v1.2.3-54-g00ecf From 0044d74b3c51cf5824435e76eca2a675b51a14bc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 Aug 2014 12:15:09 -0400 Subject: Fix another case of 12848 in circuit_handle_first_hop I looked for other places where we set circ->n_chan early, and found one in circuit_handle_first_hop() right before it calls circuit_send_next_onion_skin(). If onion_skin_create() fails there, then n_chan will still be set when circuit_send_next_onion_skin() returns. We should probably fix that too. --- src/or/circuitbuild.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/or') diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 11f8250934..5325eff64a 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -475,6 +475,7 @@ circuit_handle_first_hop(origin_circuit_t *circ) log_debug(LD_CIRC,"Conn open. Delivering first onion skin."); if ((err_reason = circuit_send_next_onion_skin(circ)) < 0) { log_info(LD_CIRC,"circuit_send_next_onion_skin failed."); + circ->base_.n_chan = NULL; return err_reason; } } -- cgit v1.2.3-54-g00ecf