- tor-commits - lists.torproject.org

[tor/master] simplify the #18332 changes file based on feedback on ticket
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 413b0c0ddba289da2cd90f394940f5f58a92a1a6 Author: Roger Dingledine <arma(a)torproject.org> Date: Fri Mar 11 13:28:51 2016 -0500 simplify the #18332 changes file based on feedback on ticket --- changes/ticket18332 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/changes/ticket18332 b/changes/ticket18332 index 6fb8118..9a8a6bd 100644 --- a/changes/ticket18332 +++ b/changes/ticket18332 @@ -1,7 +1,6 @@ o Removed features: - Streamline relay-side hsdir handling: when relays consider whether to accept an uploaded hidden service descriptor, they no longer - check whether directory authorities think they deserve the HSDir - flag, or whether they are one of the relays in the network that is + check whether they are one of the relays in the network that is "supposed" to handle that descriptor. Implements ticket 18332.

1 0

[tor/master] Bridges now refuse "rendezvous2" publish attempts
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit e28448a23e06e4bc997e1fdcca5af04a9d20598c Author: Roger Dingledine <arma(a)torproject.org> Date: Wed Mar 16 16:46:14 2016 -0400 Bridges now refuse "rendezvous2" publish attempts Suggested during review of ticket 18332. --- changes/ticket18332 | 4 ++++ src/or/directory.c | 7 +++++++ src/or/rendcache.c | 2 ++ 3 files changed, 13 insertions(+) diff --git a/changes/ticket18332 b/changes/ticket18332 index 9a8a6bd..d19c617 100644 --- a/changes/ticket18332 +++ b/changes/ticket18332 @@ -4,3 +4,7 @@ check whether they are one of the relays in the network that is "supposed" to handle that descriptor. Implements ticket 18332. + o Minor bugfixes: + - Bridges now refuse "rendezvous2" (hidden service descriptor) + publish attempts. Suggested by ticket 18332. + diff --git a/src/or/directory.c b/src/or/directory.c index 6f271b2..39fffc7 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3412,6 +3412,13 @@ directory_handle_command_post(dir_connection_t *conn, const char *headers, conn->base_.state = DIR_CONN_STATE_SERVER_WRITING; + if (!public_server_mode(options)) { + log_info(LD_DIR, "Rejected dir post request from %s " + "since we're not a public relay.", conn->base_.address); + write_http_status_line(conn, 503, "Not acting as a public relay"); + goto done; + } + if (parse_http_url(headers, &url) < 0) { write_http_status_line(conn, 400, "Bad request"); return 0; diff --git a/src/or/rendcache.c b/src/or/rendcache.c index 7c58442..8b2d3ce 100644 --- a/src/or/rendcache.c +++ b/src/or/rendcache.c @@ -623,6 +623,8 @@ rend_cache_lookup_v2_desc_as_dir(const char *desc_id, const char **desc) * If we have an older descriptor with the same ID, replace it. * * Return 0 on success, or -1 if we couldn't parse any of them. + * + * We should only call this function for public (e.g. non bridge) relays. */ int rend_cache_store_v2_desc_as_dir(const char *desc)

1 0

[tor/master] Merge remote-tracking branch 'arma/ticket18332-try3'
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit ddd30f966a4b186d42650e3b321da8352a19b5f5 Merge: 70024ea e28448a Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:41:23 2016 -0400 Merge remote-tracking branch 'arma/ticket18332-try3' changes/ticket18332 | 10 ++++ src/or/directory.c | 35 +++++------ src/or/rendcache.c | 49 ++++++---------- src/or/rendcache.h | 21 +++---- src/or/rendcommon.c | 69 ---------------------- src/or/rendcommon.h | 3 - src/test/test_dir_handle_get.c | 16 +---- src/test/test_rendcache.c | 129 +++++++++-------------------------------- 8 files changed, 78 insertions(+), 254 deletions(-) diff --cc src/or/rendcommon.h index 7d81976,c520c06..d67552e --- a/src/or/rendcommon.h +++ b/src/or/rendcommon.h @@@ -58,9 -57,7 +57,7 @@@ void rend_get_descriptor_id_bytes(char const char *service_id, const char *secret_id_part); int hid_serv_get_responsible_directories(smartlist_t *responsible_dirs, - const char *id); + const char *id); - int hid_serv_acting_as_directory(void); - MOCK_DECL(int, hid_serv_responsible_for_desc_id, (const char *id)); rend_data_t *rend_data_dup(const rend_data_t *data); rend_data_t *rend_data_client_create(const char *onion_address,

1 0

[tor/master] Scrub service name in introduction circuit warning
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 2c057c2833ff2ab0779cbd1e5f3747be0a232126 Author: John Brooks <special(a)torproject.org> Date: Mon Mar 21 19:22:14 2016 +0700 Scrub service name in introduction circuit warning Fixes bug 18600. --- src/or/rendservice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 09b20e3..39e5831 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2717,7 +2717,7 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) circuit->rend_data->rend_pk_digest); if (!service) { log_warn(LD_REND, "Unrecognized service ID %s on introduction circuit %u.", - serviceid, (unsigned)circuit->base_.n_circ_id); + safe_str_client(serviceid), (unsigned)circuit->base_.n_circ_id); reason = END_CIRC_REASON_NOSUCHSERVICE; goto err; }

1 0

[tor/master] Add new channel/queue_incoming unit tests; modify channel unit tests for new clarified handling of alloc/free responsibility for queued incoming cells
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 1cdc7fddb2ed10d72f4e65e15d1af4d803a1acdb Author: Andrea Shepard <andrea(a)torproject.org> Date: Tue Mar 15 03:28:04 2016 +0000 Add new channel/queue_incoming unit tests; modify channel unit tests for new clarified handling of alloc/free responsibility for queued incoming cells --- src/test/test_channel.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 116 insertions(+), 5 deletions(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index e11ac3f..b705ee5 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -25,7 +25,9 @@ extern uint64_t estimated_total_queue_size; static int test_chan_accept_cells = 0; static int test_chan_fixed_cells_recved = 0; +static cell_t * test_chan_last_seen_fixed_cell_ptr = NULL; static int test_chan_var_cells_recved = 0; +static var_cell_t * test_chan_last_seen_var_cell_ptr = NULL; static int test_cells_written = 0; static int test_destroy_not_pending_calls = 0; static int test_doesnt_want_writes_count = 0; @@ -70,6 +72,7 @@ static void test_channel_flushmux(void *arg); static void test_channel_incoming(void *arg); static void test_channel_lifecycle(void *arg); static void test_channel_multi(void *arg); +static void test_channel_queue_incoming(void *arg); static void test_channel_queue_size(void *arg); static void test_channel_write(void *arg); @@ -179,7 +182,7 @@ chan_test_cell_handler(channel_t *ch, tt_assert(ch); tt_assert(cell); - tor_free(cell); + test_chan_last_seen_fixed_cell_ptr = cell; ++test_chan_fixed_cells_recved; done: @@ -214,7 +217,7 @@ chan_test_var_cell_handler(channel_t *ch, tt_assert(ch); tt_assert(var_cell); - tor_free(var_cell); + test_chan_last_seen_var_cell_ptr = var_cell; ++test_chan_var_cells_recved; done: @@ -608,7 +611,7 @@ test_channel_dumpstats(void *arg) make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; channel_queue_cell(ch, cell); - cell = NULL; + tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, ==, old_count + 1); tt_assert(ch->n_bytes_recved > 0); tt_assert(ch->n_cells_recved > 0); @@ -819,7 +822,7 @@ test_channel_incoming(void *arg) make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; channel_queue_cell(ch, cell); - cell = NULL; + tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, ==, old_count + 1); /* Receive a variable-size cell */ @@ -827,7 +830,7 @@ test_channel_incoming(void *arg) make_fake_var_cell(var_cell); old_count = test_chan_var_cells_recved; channel_queue_var_cell(ch, var_cell); - var_cell = NULL; + tor_free(cell); tt_int_op(test_chan_var_cells_recved, ==, old_count + 1); /* Close it */ @@ -1423,6 +1426,113 @@ test_channel_queue_impossible(void *arg) } static void +test_channel_queue_incoming(void *arg) +{ + channel_t *ch = NULL; + cell_t *cell = NULL; + var_cell_t *var_cell = NULL; + int old_fixed_count, old_var_count; + + (void)arg; + + /* Mock these for duration of the test */ + MOCK(scheduler_channel_doesnt_want_writes, + scheduler_channel_doesnt_want_writes_mock); + 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.0f; + + ch = new_fake_channel(); + tt_assert(ch); + /* Start it off in OPENING */ + ch->state = CHANNEL_STATE_OPENING; + /* We'll need a cmux */ + ch->cmux = circuitmux_alloc(); + + /* Test cell handler getters */ + tt_ptr_op(channel_get_cell_handler(ch), ==, NULL); + tt_ptr_op(channel_get_var_cell_handler(ch), ==, NULL); + + /* Try to register it */ + channel_register(ch); + tt_assert(ch->registered); + + /* Open it */ + channel_change_state(ch, CHANNEL_STATE_OPEN); + tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); + + /* Assert that the incoming queue is empty */ + tt_assert(TOR_SIMPLEQ_EMPTY(&(ch->incoming_queue))); + + /* Queue an incoming fixed-length cell */ + cell = tor_malloc_zero(sizeof(cell_t)); + make_fake_cell(cell); + channel_queue_cell(ch, cell); + + /* Assert that the incoming queue has one entry */ + tt_int_op(chan_cell_queue_len(&(ch->incoming_queue)), ==, 1); + + /* Queue an incoming var cell */ + var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); + make_fake_var_cell(var_cell); + channel_queue_var_cell(ch, var_cell); + + /* Assert that the incoming queue has two entries */ + tt_int_op(chan_cell_queue_len(&(ch->incoming_queue)), ==, 2); + + /* + * Install cell handlers; this will drain the queue, so save the old + * cell counters first + */ + old_fixed_count = test_chan_fixed_cells_recved; + old_var_count = test_chan_var_cells_recved; + channel_set_cell_handlers(ch, + chan_test_cell_handler, + chan_test_var_cell_handler); + tt_ptr_op(channel_get_cell_handler(ch), ==, chan_test_cell_handler); + tt_ptr_op(channel_get_var_cell_handler(ch), ==, chan_test_var_cell_handler); + + /* Assert cells were received */ + tt_int_op(test_chan_fixed_cells_recved, ==, old_fixed_count + 1); + tt_int_op(test_chan_var_cells_recved, ==, old_var_count + 1); + + /* + * Assert that the pointers are different from the cells we allocated; + * when queueing cells with no incoming cell handlers installed, the + * channel layer should copy them to a new buffer, and free them after + * delivery. These pointers will have already been freed by the time + * we get here, so don't dereference them. + */ + tt_ptr_op(test_chan_last_seen_fixed_cell_ptr, !=, cell); + tt_ptr_op(test_chan_last_seen_var_cell_ptr, !=, var_cell); + + /* Assert queue is now empty */ + tt_assert(TOR_SIMPLEQ_EMPTY(&(ch->incoming_queue))); + + /* Close it; this contains an assertion that the incoming queue is empty */ + channel_mark_for_close(ch); + tt_int_op(ch->state, ==, CHANNEL_STATE_CLOSING); + chan_test_finish_close(ch); + tt_int_op(ch->state, ==, CHANNEL_STATE_CLOSED); + channel_run_cleanup(); + ch = NULL; + + done: + free_fake_channel(ch); + tor_free(cell); + tor_free(var_cell); + + UNMOCK(scheduler_channel_doesnt_want_writes); + UNMOCK(scheduler_release_channel); + + return; +} + +static void test_channel_queue_size(void *arg) { channel_t *ch = NULL; @@ -1666,6 +1776,7 @@ struct testcase_t channel_tests[] = { { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, { "multi", test_channel_multi, TT_FORK, NULL, NULL }, { "queue_impossible", test_channel_queue_impossible, TT_FORK, NULL, NULL }, + { "queue_incoming", test_channel_queue_incoming, TT_FORK, NULL, NULL }, { "queue_size", test_channel_queue_size, TT_FORK, NULL, NULL }, { "write", test_channel_write, TT_FORK, NULL, NULL }, END_OF_TESTCASES

1 0

[tor/master] Make sure channel_t queues its own copy of incoming cells
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit bd87d37a861c541afbeb660b4d8dd62df14d5b45 Author: Andrea Shepard <andrea(a)torproject.org> Date: Tue Mar 15 07:40:19 2016 +0000 Make sure channel_t queues its own copy of incoming cells --- src/or/channel.c | 38 ++++++++++++++++++++++++++++++++++---- src/or/channeltls.c | 11 +++++++++++ src/or/connection_or.c | 35 +++++++++++++++++++++++++++++++++++ src/or/connection_or.h | 1 + 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 21522a5..62a21be 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -2652,6 +2652,11 @@ channel_process_cells(channel_t *chan) /* * Process cells until we're done or find one we have no current handler * for. + * + * We must free the cells here after calling the handler, since custody + * of the buffer was given to the channel layer when they were queued; + * see comments on memory management in channel_queue_cell() and in + * channel_queue_var_cell() below. */ while (NULL != (q = TOR_SIMPLEQ_FIRST(&chan->incoming_queue))) { tor_assert(q); @@ -2669,6 +2674,7 @@ channel_process_cells(channel_t *chan) q->u.fixed.cell, chan, U64_PRINTF_ARG(chan->global_identifier)); chan->cell_handler(chan, q->u.fixed.cell); + tor_free(q->u.fixed.cell); tor_free(q); } else if (q->type == CELL_QUEUE_VAR && chan->var_cell_handler) { @@ -2681,6 +2687,7 @@ channel_process_cells(channel_t *chan) q->u.var.var_cell, chan, U64_PRINTF_ARG(chan->global_identifier)); chan->var_cell_handler(chan, q->u.var.var_cell); + tor_free(q->u.var.var_cell); tor_free(q); } else { /* Can't handle this one */ @@ -2701,6 +2708,7 @@ channel_queue_cell(channel_t *chan, cell_t *cell) { int need_to_queue = 0; cell_queue_entry_t *q; + cell_t *cell_copy = NULL; tor_assert(chan); tor_assert(cell); @@ -2728,8 +2736,19 @@ channel_queue_cell(channel_t *chan, cell_t *cell) U64_PRINTF_ARG(chan->global_identifier)); chan->cell_handler(chan, cell); } else { - /* Otherwise queue it and then process the queue if possible. */ - q = cell_queue_entry_new_fixed(cell); + /* + * Otherwise queue it and then process the queue if possible. + * + * We queue a copy, not the original pointer - it might have been on the + * stack in connection_or_process_cells_from_inbuf() (or another caller + * if we ever have a subclass other than channel_tls_t), or be freed + * there after we return. This is the uncommon case; the non-copying + * fast path occurs in the if (!need_to_queue) case above when the + * upper layer has installed cell handlers. + */ + cell_copy = tor_malloc_zero(sizeof(cell_t)); + memcpy(cell_copy, cell, sizeof(cell_t)); + q = cell_queue_entry_new_fixed(cell_copy); log_debug(LD_CHANNEL, "Queueing incoming cell_t %p for channel %p " "(global ID " U64_FORMAT ")", @@ -2755,6 +2774,7 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) { int need_to_queue = 0; cell_queue_entry_t *q; + var_cell_t *cell_copy = NULL; tor_assert(chan); tor_assert(var_cell); @@ -2783,8 +2803,18 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) U64_PRINTF_ARG(chan->global_identifier)); chan->var_cell_handler(chan, var_cell); } else { - /* Otherwise queue it and then process the queue if possible. */ - q = cell_queue_entry_new_var(var_cell); + /* + * Otherwise queue it and then process the queue if possible. + * + * We queue a copy, not the original pointer - it might have been on the + * stack in connection_or_process_cells_from_inbuf() (or another caller + * if we ever have a subclass other than channel_tls_t), or be freed + * there after we return. This is the uncommon case; the non-copying + * fast path occurs in the if (!need_to_queue) case above when the + * upper layer has installed cell handlers. + */ + cell_copy = var_cell_copy(var_cell); + q = cell_queue_entry_new_var(cell_copy); log_debug(LD_CHANNEL, "Queueing incoming var_cell_t %p for channel %p " "(global ID " U64_FORMAT ")", diff --git a/src/or/channeltls.c b/src/or/channeltls.c index c90f569..2a84514 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -1009,6 +1009,11 @@ channel_tls_time_process_cell(cell_t *cell, channel_tls_t *chan, int *time, * for cell types specific to the handshake for this transport protocol and * handles them, and queues all other cells to the channel_t layer, which * eventually will hand them off to command.c. + * + * The channel layer itself decides whether the cell should be queued or + * can be handed off immediately to the upper-layer code. It is responsible + * for copying in the case that it queues; we merely pass pointers through + * which we get from connection_or_process_cells_from_inbuf(). */ void @@ -1106,6 +1111,12 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) * related and live below the channel_t layer, so no variable-length * cells ever get delivered in the current implementation, but I've left * the mechanism in place for future use. + * + * If we were handing them off to the upper layer, the channel_t queueing + * code would be responsible for memory management, and we'd just be passing + * pointers through from connection_or_process_cells_from_inbuf(). That + * caller always frees them after this function returns, so this function + * should never free var_cell. */ void diff --git a/src/or/connection_or.c b/src/or/connection_or.c index a967c93..9944494 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -488,6 +488,28 @@ var_cell_new(uint16_t payload_len) return cell; } +/** + * Copy a var_cell_t + */ + +var_cell_t * +var_cell_copy(const var_cell_t *src) +{ + var_cell_t *copy = NULL; + size_t size = 0; + + if (src != NULL) { + size = STRUCT_OFFSET(var_cell_t, payload) + src->payload_len; + copy = tor_malloc_zero(size); + copy->payload_len = src->payload_len; + copy->command = src->command; + copy->circ_id = src->circ_id; + memcpy(copy->payload, src->payload, copy->payload_len); + } + + return copy; +} + /** Release all space held by <b>cell</b>. */ void var_cell_free(var_cell_t *cell) @@ -2060,6 +2082,19 @@ connection_or_process_cells_from_inbuf(or_connection_t *conn) { var_cell_t *var_cell; + /* + * Note on memory management for incoming cells: below the channel layer, + * we shouldn't need to consider its internal queueing/copying logic. It + * is safe to pass cells to it on the stack or on the heap, but in the + * latter case we must be sure we free them later. + * + * The incoming cell queue code in channel.c will (in the common case) + * decide it can pass them to the upper layer immediately, in which case + * those functions may run directly on the cell pointers we pass here, or + * it may decide to queue them, in which case it will allocate its own + * buffer and copy the cell. + */ + while (1) { log_debug(LD_OR, TOR_SOCKET_T_FORMAT": starting, inbuf_datalen %d " diff --git a/src/or/connection_or.h b/src/or/connection_or.h index 3877fd5..0bd8567 100644 --- a/src/or/connection_or.h +++ b/src/or/connection_or.h @@ -97,6 +97,7 @@ void cell_pack(packed_cell_t *dest, const cell_t *src, int wide_circ_ids); int var_cell_pack_header(const var_cell_t *cell, char *hdr_out, int wide_circ_ids); var_cell_t *var_cell_new(uint16_t payload_len); +var_cell_t *var_cell_copy(const var_cell_t *src); void var_cell_free(var_cell_t *cell); /** DOCDOC */

1 0

[tor/master] changes file for bug18570
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 72ebf4160412f64fb6ae0cd97dd89d01d89c075a Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:19:07 2016 -0400 changes file for bug18570 --- changes/bug18570 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/changes/bug18570 b/changes/bug18570 new file mode 100644 index 0000000..04f72f4 --- /dev/null +++ b/changes/bug18570 @@ -0,0 +1,7 @@ + o Minor bugfixes (correctness): + - Fix a bad memory handling bug that would occur if we had queued + a cell on a channel's incoming queue. Fortunately, we can't actually + queue a cell like that as our code is constructed today, but it's best + to avoid this kind of error, even if there isn't any code that triggers + it today. Fixes bug 18570; bugfix on 0.2.4.4-alpha. +

1 0

[tor/master] Merge branch 'bug18570_027'
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit cb3f9bc2d48e8c3f4847643c03e082d394d33168 Merge: a42938c 72ebf41 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:20:16 2016 -0400 Merge branch 'bug18570_027' changes/bug18570 | 7 +++ src/or/channel.c | 38 +++++++++++++-- src/or/channeltls.c | 11 +++++ src/or/connection_or.c | 35 ++++++++++++++ src/or/connection_or.h | 1 + src/test/test_channel.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 204 insertions(+), 9 deletions(-)

1 0

[tor/master] changes file for 18600
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 70024ea6c4df8b2fe0c07fe5519d2c9683fc833b Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:33:58 2016 -0400 changes file for 18600 --- changes/bug18600 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/changes/bug18600 b/changes/bug18600 new file mode 100644 index 0000000..bf53bfe --- /dev/null +++ b/changes/bug18600 @@ -0,0 +1,3 @@ + o Minor bugfixes (logging): + - Scrub service in from "unrecognized service ID" log messages. + Fixes bug 18600; bugfix on 0.2.4.11-alpha.

1 0

[tor/master] Merge remote-tracking branch 'special/bug18600'
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 13eb120beaff5e006c0eb9a3fc8563c5df16a328 Merge: cb3f9bc 2c057c2 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:32:39 2016 -0400 Merge remote-tracking branch 'special/bug18600' src/or/rendservice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

1 0