December 2017 - tor-commits - lists.torproject.org

[tor/master] test: Add outbound channel cell test
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit fa8c98985b35cf2d0330038b8a1d455838d76af9 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 11:17:21 2017 -0500 test: Add outbound channel cell test Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 008c31b6b..fce2b3eec 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -6,8 +6,10 @@ #include "or.h" #include "channel.h" /* For channel_note_destroy_not_pending */ +#define CIRCUITLIST_PRIVATE #include "circuitlist.h" #include "circuitmux.h" +#include "circuitmux_ewma.h" /* For var_cell_free */ #include "connection_or.h" /* For packed_cell stuff */ @@ -546,6 +548,146 @@ test_channel_dumpstats(void *arg) return; } +/* Test outbound cell. The callstack is: + * channel_flush_some_cells() + * -> channel_flush_from_first_active_circuit() + * -> channel_write_packed_cell() + * -> write_packed_cell() + * -> chan->write_packed_cell() fct ptr. + * + * This test goes from a cell in a circuit up to the channel write handler + * that should put them on the connection outbuf. */ +static void +test_channel_outbound_cell(void *arg) +{ + int old_count; + channel_t *chan = NULL; + packed_cell_t *p_cell = NULL, *p_cell2 = NULL; + origin_circuit_t *circ = NULL; + cell_queue_t *queue; + + (void) arg; + + /* The channel will be freed so we need to hijack this so the scheduler + * doesn't get confused. */ + MOCK(scheduler_release_channel, scheduler_release_channel_mock); + + /* Accept cells to lower layer */ + test_chan_accept_cells = 1; + /* Use default overhead factor */ + test_overhead_estimate = 1.0; + + /* Setup a valid circuit to queue a cell. */ + circ = origin_circuit_new(); + tt_assert(circ); + /* Circuit needs an origin purpose to be considered origin. */ + TO_CIRCUIT(circ)->purpose = CIRCUIT_PURPOSE_C_GENERAL; + TO_CIRCUIT(circ)->n_circ_id = 42; + /* This is the outbound test so use the next channel queue. */ + queue = &TO_CIRCUIT(circ)->n_chan_cells; + /* Setup packed cell to queue on the circuit. */ + p_cell = packed_cell_new(); + tt_assert(p_cell); + p_cell2 = packed_cell_new(); + tt_assert(p_cell2); + /* Setup a channel to put the circuit on. */ + chan = new_fake_channel(); + tt_assert(chan); + chan->state = CHANNEL_STATE_OPENING; + channel_change_state_open(chan); + /* Outbound channel. */ + channel_mark_outgoing(chan); + /* Try to register it so we can clean it through the channel cleanup + * process. */ + channel_register(chan); + tt_int_op(chan->registered, OP_EQ, 1); + /* Set EWMA policy so we can pick it when flushing. */ + channel_set_cmux_policy_everywhere(&ewma_policy); + tt_ptr_op(circuitmux_get_policy(chan->cmux), OP_EQ, &ewma_policy); + + /* Register circuit to the channel circid map which will attach the circuit + * to the channel's cmux as well. */ + circuit_set_n_circid_chan(TO_CIRCUIT(circ), 42, chan); + tt_int_op(channel_num_circuits(chan), OP_EQ, 1); + tt_assert(!TO_CIRCUIT(circ)->next_active_on_n_chan); + tt_assert(!TO_CIRCUIT(circ)->prev_active_on_n_chan); + /* Test the cmux state. */ + tt_ptr_op(TO_CIRCUIT(circ)->n_mux, OP_EQ, chan->cmux); + tt_int_op(circuitmux_is_circuit_attached(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 1); + + /* Flush the channel without any cell on it. */ + old_count = test_cells_written; + ssize_t flushed = channel_flush_some_cells(chan, 1); + tt_i64_op(flushed, OP_EQ, 0); + tt_int_op(test_cells_written, OP_EQ, old_count); + tt_int_op(channel_more_to_flush(chan), OP_EQ, 0); + tt_int_op(circuitmux_num_active_circuits(chan->cmux), OP_EQ, 0); + tt_int_op(circuitmux_num_cells(chan->cmux), OP_EQ, 0); + tt_int_op(circuitmux_is_circuit_active(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 0); + tt_u64_op(chan->n_cells_xmitted, OP_EQ, 0); + tt_u64_op(chan->n_bytes_xmitted, OP_EQ, 0); + + /* Queue cell onto the next queue that is the outbound direction. Than + * update its cmux so the circuit can be picked when flushing cells. */ + cell_queue_append(queue, p_cell); + p_cell = NULL; + tt_int_op(queue->n, OP_EQ, 1); + cell_queue_append(queue, p_cell2); + p_cell2 = NULL; + tt_int_op(queue->n, OP_EQ, 2); + + update_circuit_on_cmux(TO_CIRCUIT(circ), CELL_DIRECTION_OUT); + tt_int_op(circuitmux_num_active_circuits(chan->cmux), OP_EQ, 1); + tt_int_op(circuitmux_num_cells(chan->cmux), OP_EQ, 2); + tt_int_op(circuitmux_is_circuit_active(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 1); + + /* From this point on, we have a queued cell on an active circuit attached + * to the channel's cmux. */ + + /* Flush the first cell. This is going to go down the call stack. */ + old_count = test_cells_written; + flushed = channel_flush_some_cells(chan, 1); + tt_i64_op(flushed, OP_EQ, 1); + tt_int_op(test_cells_written, OP_EQ, old_count + 1); + tt_int_op(circuitmux_num_cells(chan->cmux), OP_EQ, 1); + tt_int_op(channel_more_to_flush(chan), OP_EQ, 1); + /* Circuit should remain active because there is a second cell queued. */ + tt_int_op(circuitmux_is_circuit_active(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 1); + /* Should still be attached. */ + tt_int_op(circuitmux_is_circuit_attached(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 1); + tt_u64_op(chan->n_cells_xmitted, OP_EQ, 1); + tt_u64_op(chan->n_bytes_xmitted, OP_EQ, get_cell_network_size(0)); + + /* Flush second cell. This is going to go down the call stack. */ + old_count = test_cells_written; + flushed = channel_flush_some_cells(chan, 1); + tt_i64_op(flushed, OP_EQ, 1); + tt_int_op(test_cells_written, OP_EQ, old_count + 1); + tt_int_op(circuitmux_num_cells(chan->cmux), OP_EQ, 0); + tt_int_op(channel_more_to_flush(chan), OP_EQ, 0); + /* No more cells should make the circuit inactive. */ + tt_int_op(circuitmux_is_circuit_active(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 0); + /* Should still be attached. */ + tt_int_op(circuitmux_is_circuit_attached(chan->cmux, TO_CIRCUIT(circ)), + OP_EQ, 1); + tt_u64_op(chan->n_cells_xmitted, OP_EQ, 2); + tt_u64_op(chan->n_bytes_xmitted, OP_EQ, get_cell_network_size(0) * 2); + + done: + if (circ) { + circuit_free(TO_CIRCUIT(circ)); + } + tor_free(p_cell); + channel_free_all(); + UNMOCK(scheduler_release_channel); +} + /* Test inbound cell. The callstack is: * channel_process_cell() * -> chan->cell_handler() @@ -1016,6 +1158,8 @@ test_channel_id_map(void *arg) struct testcase_t channel_tests[] = { { "inbound_cell", test_channel_inbound_cell, TT_FORK, NULL, NULL }, + { "outbound_cell", test_channel_outbound_cell, TT_FORK, + NULL, NULL }, { "id_map", test_channel_id_map, TT_FORK, NULL, NULL },

1 0

[tor/master] test: Fix memleak of channel cmux
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 3ed0b28a013b846e1b787c7dee3bdc7359c5e8bf Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 11:39:33 2017 -0500 test: Fix memleak of channel cmux Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_relay.c | 4 ---- src/test/test_scheduler.c | 8 -------- 2 files changed, 12 deletions(-) diff --git a/src/test/test_relay.c b/src/test/test_relay.c index e3489627a..73c0ed558 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -67,10 +67,6 @@ test_relay_append_cell_to_circuit_queue(void *arg) pchan = new_fake_channel(); tt_assert(pchan); - /* We'll need chans with working cmuxes */ - nchan->cmux = circuitmux_alloc(); - pchan->cmux = circuitmux_alloc(); - /* Make a fake orcirc */ orcirc = new_fake_orcirc(nchan, pchan); tt_assert(orcirc); diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index d861b05b8..2cee45a49 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -440,8 +440,6 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type) /* Start it off in OPENING */ ch1->state = CHANNEL_STATE_OPENING; - /* We'll need a cmux */ - ch1->cmux = circuitmux_alloc(); /* Try to register it */ channel_register(ch1); tt_assert(ch1->registered); @@ -453,7 +451,6 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type) ch2 = new_fake_channel(); tt_assert(ch2); ch2->state = CHANNEL_STATE_OPENING; - ch2->cmux = circuitmux_alloc(); channel_register(ch2); tt_assert(ch2->registered); @@ -652,8 +649,6 @@ test_scheduler_loop_vanilla(void *arg) /* Start it off in OPENING */ ch1->state = CHANNEL_STATE_OPENING; - /* We'll need a cmux */ - ch1->cmux = circuitmux_alloc(); /* Try to register it */ channel_register(ch1); tt_assert(ch1->registered); @@ -668,7 +663,6 @@ test_scheduler_loop_vanilla(void *arg) ch2->magic = TLS_CHAN_MAGIC; tt_assert(ch2); ch2->state = CHANNEL_STATE_OPENING; - ch2->cmux = circuitmux_alloc(); channel_register(ch2); tt_assert(ch2->registered); /* @@ -819,7 +813,6 @@ test_scheduler_loop_kist(void *arg) tt_assert(ch1); ch1->magic = TLS_CHAN_MAGIC; ch1->state = CHANNEL_STATE_OPENING; - ch1->cmux = circuitmux_alloc(); channel_register(ch1); tt_assert(ch1->registered); channel_change_state_open(ch1); @@ -830,7 +823,6 @@ test_scheduler_loop_kist(void *arg) tt_assert(ch2); ch2->magic = TLS_CHAN_MAGIC; ch2->state = CHANNEL_STATE_OPENING; - ch2->cmux = circuitmux_alloc(); channel_register(ch2); tt_assert(ch2->registered); channel_change_state_open(ch2);

1 0

[tor/master] test: Add unit test for channel_check_for_duplicates()
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 36f1fb3be30f2563c7fa1eb220cb0484767adc28 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 12:46:45 2017 -0500 test: Add unit test for channel_check_for_duplicates() Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 3 +- src/or/channel.h | 2 + src/test/test_channel.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index ffa827064..5a5a7e27d 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -149,7 +149,6 @@ HT_GENERATE2(channel_idmap, channel_idmap_entry_s, node, channel_idmap_hash, channel_idmap_eq, 0.5, tor_reallocarray_, tor_free_) /* Functions to maintain the digest map */ -static void channel_add_to_digest_map(channel_t *chan); static void channel_remove_from_digest_map(channel_t *chan); static void channel_force_free(channel_t *chan); @@ -551,7 +550,7 @@ channel_listener_unregister(channel_listener_t *chan_l) * already exist. */ -static void +STATIC void channel_add_to_digest_map(channel_t *chan) { channel_idmap_entry_t *ent, search; diff --git a/src/or/channel.h b/src/or/channel.h index 5a2457faf..d88a77c9a 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -431,6 +431,8 @@ void channel_set_cmux_policy_everywhere(circuitmux_policy_t *pol); #ifdef CHANNEL_PRIVATE_ +STATIC void channel_add_to_digest_map(channel_t *chan); + #endif /* defined(CHANNEL_PRIVATE_) */ /* Channel operations for subclasses and internal use only */ diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 203fa1319..3f29d97c3 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -17,8 +17,10 @@ #include "relay.h" /* For init/free stuff */ #include "scheduler.h" +#include "networkstatus.h" /* Test suite stuff */ +#include "log_test_helpers.h" #include "test.h" #include "fakechans.h" @@ -36,6 +38,7 @@ static int test_releases_count = 0; static channel_t *dump_statistics_mock_target = NULL; static int dump_statistics_mock_matches = 0; static int test_close_called = 0; +static int test_chan_should_be_canonical = 0; static void chan_test_channel_dump_statistics_mock( channel_t *chan, int severity); @@ -426,6 +429,18 @@ scheduler_release_channel_mock(channel_t *ch) return; } +static int +test_chan_is_canonical(channel_t *chan, int req) +{ + tor_assert(chan); + (void) req; + + if (test_chan_should_be_canonical) { + return 1; + } + return 0; +} + /** * Test for channel_dumpstats() and limited test for * channel_dump_statistics() @@ -1279,6 +1294,90 @@ test_channel_state(void *arg) ; } +static networkstatus_t *mock_ns = NULL; + +static networkstatus_t * +mock_networkstatus_get_latest_consensus(void) +{ + return mock_ns; +} + +static void +test_channel_duplicates(void *arg) +{ + channel_t *chan = NULL; + routerstatus_t rs; + + (void) arg; + + setup_full_capture_of_logs(LOG_INFO); + /* Try a flat call with channel nor connections. */ + channel_check_for_duplicates(); + expect_log_msg_containing( + "Found 0 connections to 0 relays. Found 0 current canonical " + "connections, in 0 of which we were a non-canonical peer. " + "0 relays had more than 1 connection, 0 had more than 2, and " + "0 had more than 4 connections."); + + mock_ns = tor_malloc_zero(sizeof(*mock_ns)); + mock_ns->routerstatus_list = smartlist_new(); + MOCK(networkstatus_get_latest_consensus, + mock_networkstatus_get_latest_consensus); + + chan = new_fake_channel(); + tt_assert(chan); + chan->is_canonical = test_chan_is_canonical; + memset(chan->identity_digest, 'A', sizeof(chan->identity_digest)); + channel_add_to_digest_map(chan); + tt_ptr_op(channel_find_by_remote_identity(chan->identity_digest, NULL), + OP_EQ, chan); + + /* No relay has been associated with this channel. */ + channel_check_for_duplicates(); + expect_log_msg_containing( + "Found 0 connections to 0 relays. Found 0 current canonical " + "connections, in 0 of which we were a non-canonical peer. " + "0 relays had more than 1 connection, 0 had more than 2, and " + "0 had more than 4 connections."); + + /* Associate relay to this connection in the consensus. */ + memset(&rs, 0, sizeof(rs)); + memset(rs.identity_digest, 'A', sizeof(rs.identity_digest)); + smartlist_add(mock_ns->routerstatus_list, &rs); + + /* Non opened channel. */ + chan->state = CHANNEL_STATE_CLOSING; + channel_check_for_duplicates(); + expect_log_msg_containing( + "Found 0 connections to 0 relays. Found 0 current canonical " + "connections, in 0 of which we were a non-canonical peer. " + "0 relays had more than 1 connection, 0 had more than 2, and " + "0 had more than 4 connections."); + chan->state = CHANNEL_STATE_OPEN; + + channel_check_for_duplicates(); + expect_log_msg_containing( + "Found 1 connections to 1 relays. Found 0 current canonical " + "connections, in 0 of which we were a non-canonical peer. " + "0 relays had more than 1 connection, 0 had more than 2, and " + "0 had more than 4 connections."); + + test_chan_should_be_canonical = 1; + channel_check_for_duplicates(); + expect_log_msg_containing( + "Found 1 connections to 1 relays. Found 1 current canonical " + "connections, in 1 of which we were a non-canonical peer. " + "0 relays had more than 1 connection, 0 had more than 2, and " + "0 had more than 4 connections."); + teardown_capture_of_logs(); + + done: + free_fake_channel(chan); + smartlist_clear(mock_ns->routerstatus_list); + networkstatus_vote_free(mock_ns); + UNMOCK(networkstatus_get_latest_consensus); +} + struct testcase_t channel_tests[] = { { "inbound_cell", test_channel_inbound_cell, TT_FORK, NULL, NULL }, @@ -1294,6 +1393,8 @@ struct testcase_t channel_tests[] = { NULL, NULL }, { "state", test_channel_state, TT_FORK, NULL, NULL }, + { "duplicates", test_channel_duplicates, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] test: Fix channel dumpstats test
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 08d0c39b914194127497f4f2b7dc9bd85e2becbb Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 11:28:30 2017 -0500 test: Fix channel dumpstats test Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 168ffcf78..5a20dbcf2 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -435,6 +435,7 @@ static void test_channel_dumpstats(void *arg) { channel_t *ch = NULL; + cell_t *cell = NULL; packed_cell_t *p_cell = NULL; int old_count; @@ -449,7 +450,6 @@ test_channel_dumpstats(void *arg) /* Set up a new fake channel */ ch = new_fake_channel(); tt_assert(ch); - ch->cmux = circuitmux_alloc(); /* Try to register it */ channel_register(ch); @@ -492,13 +492,12 @@ test_channel_dumpstats(void *arg) /* Now make another channel */ ch = new_fake_channel(); tt_assert(ch); - ch->cmux = circuitmux_alloc(); channel_register(ch); - tt_assert(ch->registered); + tt_int_op(ch->registered, OP_EQ, 1); /* Lie about its age so dumpstats gets coverage for rate calculations */ ch->timestamp_created = time(NULL) - 30; - tt_assert(ch->timestamp_created > 0); - tt_assert(time(NULL) > ch->timestamp_created); + tt_int_op(ch->timestamp_created, OP_GT, 0); + tt_int_op(time(NULL), OP_GT, ch->timestamp_created); /* Put cells through it both ways to make the counters non-zero */ p_cell = packed_cell_new(); @@ -506,8 +505,8 @@ test_channel_dumpstats(void *arg) old_count = test_cells_written; channel_write_packed_cell(ch, p_cell); tt_int_op(test_cells_written, OP_EQ, old_count + 1); - tt_assert(ch->n_bytes_xmitted > 0); - tt_assert(ch->n_cells_xmitted > 0); + tt_u64_op(ch->n_bytes_xmitted, OP_GT, 0); + tt_u64_op(ch->n_cells_xmitted, OP_GT, 0); /* Receive path */ channel_set_cell_handlers(ch, @@ -516,12 +515,12 @@ test_channel_dumpstats(void *arg) tt_ptr_op(channel_get_cell_handler(ch), OP_EQ, chan_test_cell_handler); tt_ptr_op(channel_get_var_cell_handler(ch), OP_EQ, chan_test_var_cell_handler); - p_cell = packed_cell_new(); + cell = tor_malloc_zero(sizeof(*cell)); old_count = test_chan_fixed_cells_recved; - channel_write_packed_cell(ch, p_cell); + channel_process_cell(ch, cell); tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count + 1); - tt_assert(ch->n_bytes_recved > 0); - tt_assert(ch->n_cells_recved > 0); + tt_u64_op(ch->n_bytes_recved, OP_GT, 0); + tt_u64_op(ch->n_cells_recved, OP_GT, 0); /* Test channel_dump_statistics */ ch->describe_transport = chan_test_describe_transport; @@ -541,6 +540,7 @@ test_channel_dumpstats(void *arg) done: free_fake_channel(ch); + tor_free(cell); UNMOCK(scheduler_channel_doesnt_want_writes); UNMOCK(scheduler_release_channel); @@ -1153,8 +1153,6 @@ struct testcase_t channel_tests[] = { NULL, NULL }, { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, - - /* NOT WORKING TEST. */ { "dumpstats", test_channel_dumpstats, TT_FORK, NULL, NULL }, END_OF_TESTCASES

1 0

[tor/master] test: Fix channel lifecycle and lifecycle_2
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit bd7823b29be14953c93a6ec28a68554ddfdd2855 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 11:21:29 2017 -0500 test: Fix channel lifecycle and lifecycle_2 They were broken due to previous commit. Fixes are trivial. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index fce2b3eec..168ffcf78 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -812,8 +812,6 @@ test_channel_lifecycle(void *arg) tt_assert(ch1); /* Start it off in OPENING */ ch1->state = CHANNEL_STATE_OPENING; - /* We'll need a cmux */ - ch1->cmux = circuitmux_alloc(); /* Try to register it */ channel_register(ch1); @@ -828,14 +826,10 @@ test_channel_lifecycle(void *arg) /* Move it to OPEN and flush */ channel_change_state_open(ch1); - /* Queue should drain */ - tt_int_op(old_count + 1, OP_EQ, test_cells_written); - - /* Get another one */ +/* Get another one */ ch2 = new_fake_channel(); tt_assert(ch2); ch2->state = CHANNEL_STATE_OPENING; - ch2->cmux = circuitmux_alloc(); /* Register */ channel_register(ch2); @@ -882,11 +876,10 @@ 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); - - return; } /** @@ -920,8 +913,6 @@ test_channel_lifecycle_2(void *arg) tt_assert(ch); /* Start it off in OPENING */ ch->state = CHANNEL_STATE_OPENING; - /* The full lifecycle test needs a cmux */ - ch->cmux = circuitmux_alloc(); /* Try to register it */ channel_register(ch); @@ -941,7 +932,6 @@ test_channel_lifecycle_2(void *arg) ch = new_fake_channel(); tt_assert(ch); ch->state = CHANNEL_STATE_OPENING; - ch->cmux = circuitmux_alloc(); channel_register(ch); tt_assert(ch->registered); @@ -960,7 +950,6 @@ test_channel_lifecycle_2(void *arg) ch = new_fake_channel(); tt_assert(ch); ch->state = CHANNEL_STATE_OPENING; - ch->cmux = circuitmux_alloc(); channel_register(ch); tt_assert(ch->registered); @@ -989,7 +978,6 @@ test_channel_lifecycle_2(void *arg) ch = new_fake_channel(); tt_assert(ch); ch->state = CHANNEL_STATE_OPENING; - ch->cmux = circuitmux_alloc(); channel_register(ch); tt_assert(ch->registered); @@ -1015,7 +1003,6 @@ test_channel_lifecycle_2(void *arg) ch = new_fake_channel(); tt_assert(ch); ch->state = CHANNEL_STATE_OPENING; - ch->cmux = circuitmux_alloc(); channel_register(ch); tt_assert(ch->registered); @@ -1162,14 +1149,14 @@ struct testcase_t channel_tests[] = { NULL, NULL }, { "id_map", test_channel_id_map, TT_FORK, NULL, NULL }, - - /* NOT WORKING TEST. */ - { "dumpstats", test_channel_dumpstats, TT_FORK, - NULL, NULL }, { "lifecycle", test_channel_lifecycle, TT_FORK, NULL, NULL }, { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, + + /* NOT WORKING TEST. */ + { "dumpstats", test_channel_dumpstats, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] test: Add unit test for channel_get_for_extend()
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 8c5ed4f1503fa38a3a813ca3ca1bdc0dcb1a8ac4 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 14:15:35 2017 -0500 test: Add unit test for channel_get_for_extend() Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 164 insertions(+), 1 deletion(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 3f29d97c3..bd616218f 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -39,6 +39,8 @@ static channel_t *dump_statistics_mock_target = NULL; static int dump_statistics_mock_matches = 0; static int test_close_called = 0; static int test_chan_should_be_canonical = 0; +static int test_chan_should_match_target = 0; +static int test_chan_canonical_should_be_reliable = 0; static void chan_test_channel_dump_statistics_mock( channel_t *chan, int severity); @@ -433,7 +435,10 @@ static int test_chan_is_canonical(channel_t *chan, int req) { tor_assert(chan); - (void) req; + + if (req && test_chan_canonical_should_be_reliable) { + return 1; + } if (test_chan_should_be_canonical) { return 1; @@ -441,6 +446,18 @@ test_chan_is_canonical(channel_t *chan, int req) return 0; } +static int +test_chan_matches_target(channel_t *chan, const tor_addr_t *target) +{ + (void) chan; + (void) target; + + if (test_chan_should_match_target) { + return 1; + } + return 0; +} + /** * Test for channel_dumpstats() and limited test for * channel_dump_statistics() @@ -1378,6 +1395,150 @@ test_channel_duplicates(void *arg) UNMOCK(networkstatus_get_latest_consensus); } +static void +test_channel_for_extend(void *arg) +{ + channel_t *chan1 = NULL, *chan2 = NULL; + channel_t *ret_chan = NULL; + char digest[DIGEST_LEN]; + ed25519_public_key_t ed_id; + tor_addr_t addr; + const char *msg; + int launch; + time_t now = time(NULL); + + (void) arg; + + memset(digest, 'A', sizeof(digest)); + memset(&ed_id, 'B', sizeof(ed_id)); + + chan1 = new_fake_channel(); + tt_assert(chan1); + /* Need to be registered to get added to the id map. */ + channel_register(chan1); + tt_int_op(chan1->registered, OP_EQ, 1); + /* We need those for the test. */ + chan1->is_canonical = test_chan_is_canonical; + chan1->matches_target = test_chan_matches_target; + chan1->timestamp_created = now - 9; + + chan2 = new_fake_channel(); + tt_assert(chan2); + /* Need to be registered to get added to the id map. */ + channel_register(chan2); + tt_int_op(chan2->registered, OP_EQ, 1); + /* We need those for the test. */ + chan2->is_canonical = test_chan_is_canonical; + chan2->matches_target = test_chan_matches_target; + /* Make it older than chan1. */ + chan2->timestamp_created = chan1->timestamp_created - 1; + + /* Set channel identities and add it to the channel map. The last one to be + * added is made the first one in the list so the lookup will always return + * that one first. */ + channel_set_identity_digest(chan2, digest, &ed_id); + channel_set_identity_digest(chan1, digest, &ed_id); + tt_ptr_op(channel_find_by_remote_identity(digest, NULL), OP_EQ, chan1); + tt_ptr_op(channel_find_by_remote_identity(digest, &ed_id), OP_EQ, chan1); + + /* The expected result is chan2 because it is older than chan1. */ + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(ret_chan); + tt_ptr_op(ret_chan, OP_EQ, chan2); + tt_int_op(launch, OP_EQ, 0); + tt_str_op(msg, OP_EQ, "Connection is fine; using it."); + + /* Switch that around from previous test. */ + chan2->timestamp_created = chan1->timestamp_created + 1; + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(ret_chan); + tt_ptr_op(ret_chan, OP_EQ, chan1); + tt_int_op(launch, OP_EQ, 0); + tt_str_op(msg, OP_EQ, "Connection is fine; using it."); + + /* Same creation time, num circuits will be used and they both have 0 so the + * channel 2 should be picked due to how channel_is_better() work. */ + chan2->timestamp_created = chan1->timestamp_created; + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(ret_chan); + tt_ptr_op(ret_chan, OP_EQ, chan1); + tt_int_op(launch, OP_EQ, 0); + tt_str_op(msg, OP_EQ, "Connection is fine; using it."); + + /* For the rest of the tests, we need channel 1 to be the older. */ + chan2->timestamp_created = chan1->timestamp_created + 1; + + /* Condemned the older channel. */ + chan1->state = CHANNEL_STATE_CLOSING; + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(ret_chan); + tt_ptr_op(ret_chan, OP_EQ, chan2); + tt_int_op(launch, OP_EQ, 0); + tt_str_op(msg, OP_EQ, "Connection is fine; using it."); + chan1->state = CHANNEL_STATE_OPEN; + + /* Make the older channel a client one. */ + channel_mark_client(chan1); + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(ret_chan); + tt_ptr_op(ret_chan, OP_EQ, chan2); + tt_int_op(launch, OP_EQ, 0); + tt_str_op(msg, OP_EQ, "Connection is fine; using it."); + channel_clear_client(chan1); + + /* Non matching ed identity with valid digest. */ + ed25519_public_key_t dumb_ed_id = {0}; + ret_chan = channel_get_for_extend(digest, &dumb_ed_id, &addr, &msg, + &launch); + tt_assert(!ret_chan); + tt_str_op(msg, OP_EQ, "Not connected. Connecting."); + tt_int_op(launch, OP_EQ, 1); + + /* Opening channel, we'll check if the target address matches. */ + test_chan_should_match_target = 1; + chan1->state = CHANNEL_STATE_OPENING; + chan2->state = CHANNEL_STATE_OPENING; + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(!ret_chan); + tt_str_op(msg, OP_EQ, "Connection in progress; waiting."); + tt_int_op(launch, OP_EQ, 0); + chan1->state = CHANNEL_STATE_OPEN; + chan2->state = CHANNEL_STATE_OPEN; + + /* Mark channel 1 as bad for circuits. */ + channel_mark_bad_for_new_circs(chan1); + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(ret_chan); + tt_ptr_op(ret_chan, OP_EQ, chan2); + tt_int_op(launch, OP_EQ, 0); + tt_str_op(msg, OP_EQ, "Connection is fine; using it."); + chan1->is_bad_for_new_circs = 0; + + /* Mark both channels as unusable. */ + channel_mark_bad_for_new_circs(chan1); + channel_mark_bad_for_new_circs(chan2); + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(!ret_chan); + tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. " + " Launching a new one."); + tt_int_op(launch, OP_EQ, 1); + chan1->is_bad_for_new_circs = 0; + chan2->is_bad_for_new_circs = 0; + + /* Non canonical channels. */ + test_chan_should_match_target = 0; + test_chan_canonical_should_be_reliable = 1; + ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + tt_assert(!ret_chan); + tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. " + " Launching a new one."); + tt_int_op(launch, OP_EQ, 1); + + done: + free_fake_channel(chan1); + free_fake_channel(chan2); +} + struct testcase_t channel_tests[] = { { "inbound_cell", test_channel_inbound_cell, TT_FORK, NULL, NULL }, @@ -1395,6 +1556,8 @@ struct testcase_t channel_tests[] = { NULL, NULL }, { "duplicates", test_channel_duplicates, TT_FORK, NULL, NULL }, + { "get_channel_for_extend", test_channel_for_extend, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] test: Add unit test for channel_listener_t
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit d443a5258f2015185393470b102aab0be0892e35 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 14:51:20 2017 -0500 test: Add unit test for channel_listener_t Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index bd616218f..a2e73da56 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -41,6 +41,8 @@ static int test_close_called = 0; static int test_chan_should_be_canonical = 0; static int test_chan_should_match_target = 0; static int test_chan_canonical_should_be_reliable = 0; +static int test_chan_listener_close_fn_called = 0; +static int test_chan_listener_fn_called = 0; static void chan_test_channel_dump_statistics_mock( channel_t *chan, int severity); @@ -458,6 +460,31 @@ test_chan_matches_target(channel_t *chan, const tor_addr_t *target) return 0; } +static void +test_chan_listener_close(channel_listener_t *chan) +{ + (void) chan; + ++test_chan_listener_close_fn_called; + return; +} + +static void +test_chan_listener_fn(channel_listener_t *listener, channel_t *chan) +{ + (void) listener; + (void) chan; + + ++test_chan_listener_fn_called; + return; +} + +static const char * +test_chan_listener_describe_transport(channel_listener_t *chan) +{ + (void) chan; + return "Fake listener channel."; +} + /** * Test for channel_dumpstats() and limited test for * channel_dump_statistics() @@ -1539,6 +1566,62 @@ test_channel_for_extend(void *arg) free_fake_channel(chan2); } +static void +test_channel_listener(void *arg) +{ + int old_count; + time_t now = time(NULL); + channel_listener_t *chan = NULL; + + (void) arg; + + chan = tor_malloc_zero(sizeof(*chan)); + tt_assert(chan); + channel_init_listener(chan); + tt_u64_op(chan->global_identifier, OP_EQ, 1); + tt_int_op(chan->timestamp_created, OP_GE, now); + chan->close = test_chan_listener_close; + + /* Register it. At this point, it is not open so it will be put in the + * finished list. */ + channel_listener_register(chan); + tt_int_op(chan->registered, OP_EQ, 1); + channel_listener_unregister(chan); + + /* Register it as listening now thus active. */ + chan->state = CHANNEL_LISTENER_STATE_LISTENING; + channel_listener_register(chan); + tt_int_op(chan->registered, OP_EQ, 1); + + /* Set the listener function. */ + channel_listener_set_listener_fn(chan, test_chan_listener_fn); + tt_ptr_op(chan->listener, OP_EQ, test_chan_listener_fn); + + /* Put a channel in the listener incoming list and queue it. + * function. By doing this, the listener() handler will be called. */ + channel_t *in_chan = new_fake_channel(); + old_count = test_chan_listener_fn_called; + channel_listener_queue_incoming(chan, in_chan); + free_fake_channel(in_chan); + tt_int_op(test_chan_listener_fn_called, OP_EQ, old_count + 1); + + /* Put listener channel in CLOSING state. */ + old_count = test_chan_listener_close_fn_called; + channel_listener_mark_for_close(chan); + tt_int_op(test_chan_listener_close_fn_called, OP_EQ, old_count + 1); + channel_listener_change_state(chan, CHANNEL_LISTENER_STATE_CLOSED); + + /* Dump stats so we at least hit the code path. */ + chan->describe_transport = test_chan_listener_describe_transport; + /* There is a check for "now > timestamp_created" when dumping the stats so + * make sure we go in. */ + chan->timestamp_created = now - 10; + channel_listener_dump_statistics(chan, LOG_INFO); + + done: + channel_free_all(); +} + struct testcase_t channel_tests[] = { { "inbound_cell", test_channel_inbound_cell, TT_FORK, NULL, NULL }, @@ -1558,6 +1641,8 @@ struct testcase_t channel_tests[] = { NULL, NULL }, { "get_channel_for_extend", test_channel_for_extend, TT_FORK, NULL, NULL }, + { "listener", test_channel_listener, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] doc: Update channel.c top comments from latest
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 428ee55e5187a57b8bbc171c8b62da08209a7954 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 15:34:51 2017 -0500 doc: Update channel.c top comments from latest Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 5a5a7e27d..94d7af47b 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -5,9 +5,8 @@ * \file channel.c * * \brief OR/OP-to-OR channel abstraction layer. A channel's job is to - * transfer cells from Tor instance to Tor instance. - * Currently, there is only one implementation of the channel abstraction: in - * channeltls.c. + * transfer cells from Tor instance to Tor instance. Currently, there is only + * one implementation of the channel abstraction: in channeltls.c. * * Channels are a higher-level abstraction than or_connection_t: In general, * any means that two Tor relays use to exchange cells, or any means that a @@ -24,16 +23,28 @@ * connection. * * Every channel implementation is responsible for being able to transmit - * cells that are added to it with channel_write_cell() and related functions, - * and to receive incoming cells with the channel_queue_cell() and related - * functions. See the channel_t documentation for more information. - * - * When new cells arrive on a channel, they are passed to cell handler - * functions, which can be set by channel_set_cell_handlers() - * functions. (Tor's cell handlers are in command.c.) - * - * Tor flushes cells to channels from relay.c in - * channel_flush_from_first_active_circuit(). + * cells that are passed to it + * + * For *inbound* cells, the entry point is: channel_process_cell(). It takes a + * cell and will pass it to the cell handler set by + * channel_set_cell_handlers(). Currently, this is passed back to the command + * subsystem which is command_process_cell(). + * + * NOTE: For now, the seperation between channels and specialized channels + * (like channeltls) is not that well defined. So the channeltls layer calls + * channel_process_cell() which originally comes from the connection subsytem. + * This should be hopefully be fixed with #23993. + * + * For *outbound* cells, the entry point is: channel_write_packed_cell(). + * Only packed cells are dequeued from the circuit queue by the scheduler + * which uses channel_flush_from_first_active_circuit() to decide which cells + * to flush from which circuit on the channel. They are then passed down to + * the channel subsystem. This calls the low layer with the function pointer + * .write_packed_cell(). + * + * Each specialized channel (currently only channeltls_t) MUST implement a + * series of function found in channel_t. See channel.h for more + * documentation. **/ /*

1 0

[tor/master] test: Cleanup unused code in test_channel.c
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 04762be612bf4cbdf7aaa818883cebe4b2beb22f Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 15:03:17 2017 -0500 test: Cleanup unused code in test_channel.c Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/fakechans.h | 1 - src/test/test_channel.c | 97 ++----------------------------------------------- 2 files changed, 3 insertions(+), 95 deletions(-) diff --git a/src/test/fakechans.h b/src/test/fakechans.h index c0de430e3..ab5d8461b 100644 --- a/src/test/fakechans.h +++ b/src/test/fakechans.h @@ -20,7 +20,6 @@ void scheduler_release_channel_mock(channel_t *ch); /* Query some counters used by the exposed mocks */ int get_mock_scheduler_has_waiting_cells_count(void); -int get_mock_scheduler_release_channel_count(void); #endif /* !defined(TOR_FAKECHANS_H) */ diff --git a/src/test/test_channel.c b/src/test/test_channel.c index a2e73da56..88a13862b 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -33,7 +33,6 @@ static int test_cells_written = 0; static int test_doesnt_want_writes_count = 0; static int test_dumpstats_calls = 0; static int test_has_waiting_cells_count = 0; -static double test_overhead_estimate = 1.0; static int test_releases_count = 0; static channel_t *dump_statistics_mock_target = NULL; static int dump_statistics_mock_matches = 0; @@ -44,27 +43,6 @@ static int test_chan_canonical_should_be_reliable = 0; static int test_chan_listener_close_fn_called = 0; static int test_chan_listener_fn_called = 0; -static void chan_test_channel_dump_statistics_mock( - channel_t *chan, int severity); -static const char * chan_test_describe_transport(channel_t *ch); -static void chan_test_dumpstats(channel_t *ch, int severity); -static void chan_test_var_cell_handler(channel_t *ch, - var_cell_t *var_cell); -static void chan_test_error(channel_t *ch); -static void chan_test_finish_close(channel_t *ch); -static const char * chan_test_get_remote_descr(channel_t *ch, int flags); -static int chan_test_is_canonical(channel_t *ch, int req); -static size_t chan_test_num_bytes_queued(channel_t *ch); -static int chan_test_num_cells_writeable(channel_t *ch); -static int chan_test_write_cell(channel_t *ch, cell_t *cell); -static int chan_test_write_packed_cell(channel_t *ch, - packed_cell_t *packed_cell); -static int chan_test_write_var_cell(channel_t *ch, var_cell_t *var_cell); -static void scheduler_channel_doesnt_want_writes_mock(channel_t *ch); - -static void test_channel_dumpstats(void *arg); -static void test_channel_lifecycle(void *arg); - static const char * chan_test_describe_transport(channel_t *ch) { @@ -201,35 +179,6 @@ chan_test_get_remote_descr(channel_t *ch, int flags) return "Fake channel for unit tests; no real endpoint"; } -static double -chan_test_get_overhead_estimate(channel_t *ch) -{ - tt_assert(ch); - - done: - return test_overhead_estimate; -} - -static int -chan_test_is_canonical(channel_t *ch, int req) -{ - tt_ptr_op(ch, OP_NE, NULL); - tt_assert(req == 0 || req == 1); - - done: - /* Fake channels are always canonical */ - return 1; -} - -static size_t -chan_test_num_bytes_queued(channel_t *ch) -{ - tt_assert(ch); - - done: - return 0; -} - static int chan_test_num_cells_writeable(channel_t *ch) { @@ -240,26 +189,6 @@ chan_test_num_cells_writeable(channel_t *ch) } static int -chan_test_write_cell(channel_t *ch, cell_t *cell) -{ - int rv = 0; - - tt_assert(ch); - tt_assert(cell); - - if (test_chan_accept_cells) { - /* Free the cell and bump the counter */ - tor_free(cell); - ++test_cells_written; - rv = 1; - } - /* else return 0, we didn't accept it */ - - done: - return rv; -} - -static int chan_test_write_packed_cell(channel_t *ch, packed_cell_t *packed_cell) { @@ -346,11 +275,8 @@ new_fake_channel(void) channel_init(chan); chan->close = chan_test_close; - chan->get_overhead_estimate = chan_test_get_overhead_estimate; - chan->get_remote_descr = chan_test_get_remote_descr; - chan->num_bytes_queued = chan_test_num_bytes_queued; chan->num_cells_writeable = chan_test_num_cells_writeable; - chan->write_cell = chan_test_write_cell; + chan->get_remote_descr = chan_test_get_remote_descr; chan->write_packed_cell = chan_test_write_packed_cell; chan->write_var_cell = chan_test_write_var_cell; chan->state = CHANNEL_STATE_OPEN; @@ -409,16 +335,6 @@ scheduler_channel_doesnt_want_writes_mock(channel_t *ch) } /** - * Counter query for scheduler_release_channel_mock() - */ - -int -get_mock_scheduler_release_channel_count(void) -{ - return test_releases_count; -} - -/** * Mock for scheduler_release_channel() */ @@ -584,7 +500,8 @@ test_channel_dumpstats(void *arg) /* Test channel_dump_statistics */ ch->describe_transport = chan_test_describe_transport; ch->dumpstats = chan_test_dumpstats; - ch->is_canonical = chan_test_is_canonical; + test_chan_should_be_canonical = 1; + ch->is_canonical = test_chan_is_canonical; old_count = test_dumpstats_calls; channel_dump_statistics(ch, LOG_DEBUG); tt_int_op(test_dumpstats_calls, OP_EQ, old_count + 1); @@ -633,8 +550,6 @@ test_channel_outbound_cell(void *arg) /* Accept cells to lower layer */ test_chan_accept_cells = 1; - /* Use default overhead factor */ - test_overhead_estimate = 1.0; /* Setup a valid circuit to queue a cell. */ circ = origin_circuit_new(); @@ -768,8 +683,6 @@ test_channel_inbound_cell(void *arg) /* Accept cells to lower layer */ test_chan_accept_cells = 1; - /* Use default overhead factor */ - test_overhead_estimate = 1.0; chan = new_fake_channel(); tt_assert(chan); @@ -864,8 +777,6 @@ test_channel_lifecycle(void *arg) /* Accept cells to lower layer */ test_chan_accept_cells = 1; - /* Use default overhead factor */ - test_overhead_estimate = 1.0; ch1 = new_fake_channel(); tt_assert(ch1); @@ -965,8 +876,6 @@ test_channel_lifecycle_2(void *arg) /* Accept cells to lower layer */ test_chan_accept_cells = 1; - /* Use default overhead factor */ - test_overhead_estimate = 1.0; ch = new_fake_channel(); tt_assert(ch);

1 0

[tor/master] chan: Do not re-queue after a fail cell write
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 6120efd771928fc958b552b9f5c3e09d949e92fe Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Dec 7 15:41:09 2017 -0500 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 <dgoulet(a)torproject.org> --- 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(-) diff --git a/src/or/channel.c b/src/or/channel.c index 94d7af47b..9e79fc1a3 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 e6ecc1538..023ccdefd 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 f36fc78cd..ac9103284 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 <b>cell</b> as the head of the <b>queue</b>. */ -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 88a13862b..c3b99d87a 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);

1 0